/srv/irclogs.ubuntu.com/2009/11/30/#launchpad-reviews.txt

=== abentley1 is now known as abentley
=== abentley1 is now known as abentley
=== ursula is now known as Ursinha
=== mrevell is now known as mrevell-lunch
=== matsubara-afk is now known as matsubara
=== mrevell-lunch is now known as mrevell
=== sinzui changed the topic of #launchpad-reviews to: on-call: - || reviewing: - || queue [sinzui] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
=== henninge changed the topic of #launchpad-reviews to: on-call: henninge || reviewing: - || queue [sinzui] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
* henninge forgot what "Monday" means ... ;)14:16
henningesinzui: ping14:28
sinzuihi henninge14:28
=== henninge changed the topic of #launchpad-reviews to: on-call: henninge || reviewing: sinzui || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
henningesinzui: I am looking at the obsolete-styles branch.14:28
=== abentley changed the topic of #launchpad-reviews to: on-call: henninge, abentley || reviewing: sinzui, - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
henningesinzui: I am not sure about this kind of indention: http://paste.ubuntu.com/331726/14:30
sinzuiI am14:30
sinzuiI am sure that is the best indentation14:30
henningesinzui: ;)14:30
sinzuihenninge: CSS is like C, there is no indention, so we choose what we like. I like Python14:31
henningesinzui: I was about to ask if that is the goal.14:31
sinzuihenninge: That is also the indentation in style-3.0.css14:31
henningesinzui: I see14:32
henningesinzui: should style.css be fixed eventually or will it go away, anyway?14:32
sinzuiIt should go away14:33
henningeok14:33
sinzuionce this branch lands, We can move the official global styles to the new CSS. We then have to decide what to do about the locale rules in the old stylesheet.14:33
=== salgado is now known as salgado-lunch
henningesinzui: I see added vertical space between the footer and the content on branch and translations pages. It looks like this is now the same as on the overview page and I think it looks good. Was that intentional?14:48
sinzuiYes. There were a few pages that were normalised by this14:49
henningecool14:49
mrevellHey, anyone want to take a look at the what's new branch? The diff is tiny -- https://code.edge.launchpad.net/~matthew.revell/launchpad/whatsnew-3111/+merge/1542614:50
henningeabentley: ^14:51
=== abentley changed the topic of #launchpad-reviews to: on-call: henninge, abentley || reviewing: sinzui, mrevell || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
abentleymrevell: r=abentley14:53
mrevellthanks abentley!14:53
=== abentley changed the topic of #launchpad-reviews to: on-call: henninge, abentley || reviewing: sinzui, - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
henningesinzui: r=me14:56
=== henninge changed the topic of #launchpad-reviews to: on-call: henninge, abentley || reviewing: -, - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
sinzuihenninge: thanks.14:56
=== matsubara is now known as matsubara-lunch
=== danilo_ is now known as danilos
adiroibanhenninge: regarding MP reviews, when adding a new MP, should I ping someone, or it is ok just to add them in LP ?15:17
=== beuno is now known as beuno-lunch
henningeadiroiban: it is always a good idea to ask here.15:28
henningeadiroiban: the rule is: *You* are responsible for getting a review for your branch.15:29
adiroibanhenninge: ok. then I should ping the current reviewer and ask him/her to look to my MPs ?15:29
henningeadiroiban: which is me, so you already did ... ;)15:29
adiroibanyep. One of my MPs is handled by Danilo15:30
henningeadiroiban: how many are there?15:30
adiroibanhenninge: right now I got 4... for trivial bugs15:31
henningeadeuring: I see two, bug-135008 and bug-9729315:31
henningewow, these are old ;)15:31
henningeadiroiban: ^15:32
henningeadeuring: tschuldigung15:32
adeuringhenninge: kein problem :)15:32
henningeadiroiban: which of these will be reviewed by danilo?15:32
adiroibanhttps://code.edge.launchpad.net/~adiroiban/launchpad/bug-487970/+merge/1525015:33
adeuringadiroiban: sorry, I failed bady on Friday merging your branch :( I'll try it again when PQM opens again15:33
adiroibanthis one is reviewed by Adel https://code.edge.launchpad.net/~adiroiban/launchpad/bug-193750/+merge/1529215:33
adiroibanbut there are other 2 without a reviewer15:34
adiroibanhttps://code.edge.launchpad.net/~adiroiban/launchpad/bug-135008/+merge/1534715:34
adiroibanhttps://code.edge.launchpad.net/~adiroiban/launchpad/bug-97293/+merge/1539115:34
henningeadiroiban: I'll take one and slap eon on the queue. Maybe abentley will take one, too.15:34
=== henninge changed the topic of #launchpad-reviews to: on-call: henninge, abentley || reviewing: adiroiban, - || queue [adiroiban] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
henningeoh, that one is trivial15:36
adiroibanhenninge: yep. all of them are trivial :p 15:36
henningeadiroiban: are you familiar with YUI?15:38
adiroibannope15:38
abentleyhenninge: Which one are you doing?15:38
henningeabentley: 97293, bug I can do both if you are busy, they are quite small.15:39
abentleyhenninge: No, I'm not busy.15:39
henninge;)15:39
henningeadeuring: instead of document.getElementById we use Y.get( '#...')15:40
henningeadiroiban: ^15:40
adiroibanhenninge: ok. I can do that. I was just looking at selectWidget function from the same file15:41
henningeor maybe that should be Y.one in yui3?15:41
henningeadiroiban: yes, this code is older than the introduction of yui315:41
adiroibanhenninge: selectWidget15:41
henningeadiroiban: let me look at the full code ...15:43
=== salgado-lunch is now known as salgado
henningeadiroiban: yeah, this whole file would need to be converted to use yui. So never mind yui3 for now...15:48
adiroibanhenninge: i'm still learning about LP dev practice. In the future I will try to use YUI315:49
henningeadiroiban: that is good! but in this case not using it probably was the better choice, to keep the code consistent.15:50
abentleyhenninge: Do you know why there's a search_lang attribute on LanguageSet?  It doesn't seem to be part of the model.15:51
adiroibanabentley: search_lang is the HTTP_GET argument . not part of the model15:52
adiroibanabentley: it is only relevant for the form. Is the string used to match language names while doing the search15:52
adiroibanabentley: maybe I did something stupid, and „search_lang” should have been defined in another place15:53
henningeabentley: Let me look at it.15:53
abentleyadiroiban: If it is only relevant for the form, why is it on the ILanguageSet interface?15:53
abentleyhenninge: It's adiroiban's addition, I see now.15:54
adiroibanabentley: i didn't know where it needs to be defined... so i put it there15:55
adiroibanabentley: can you suggest me how it should have been implemented? I can fix that :)15:56
abentleyadiroiban: I suggest defining a new interface called ILanguageSearch in lib/lp/translations/browser/language.py15:56
adiroibanabentley: ok. you can leave all your observation as a comment in MP and I will try to solve those requests15:57
abentleyadiroiban: Since ILanguageSearch will have only the member you want, you won't need the field_names list.15:59
=== matsubara-lunch is now known as matsubara
abentleyadiroiban: What was your motivation for changing LanguageSetView into a LaunchpadFormView?16:01
=== abentley changed the topic of #launchpad-reviews to: on-call: henninge, abentley || reviewing: adiroiban, adiroiban || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
adiroibanabentley: i was thinking this is the „standard” way of adding JS autoselect entry to a form16:02
danilosadiroiban, hey, thanks for all the work, I see you are coding on steroids :) I've reviewed your branch, do note that we won't be able to land any one of them before Wednesday, and most likely not before next Monday (we are only allowing release-critical landings right now)16:04
adiroibanabentley: I looked at all the other forms that have auto-focus enabled and they were all using LaunchpadFormView16:04
abentleyadiroiban: Why do you tal:define script and use script in tal:content, instead of doing tal:content="view/focusedElementScript" ?16:05
henningeadiroiban: r=me16:05
=== henninge changed the topic of #launchpad-reviews to: on-call: henninge, abentley || reviewing: -, adiroiban || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
henningeadiroiban: you will have to wait until next week to land it, though, as this will not be considered release-critical.16:06
adiroibanabentley: at is was use both in tal:condition and tal:content16:06
henningeadiroiban: please ping danilo or me on Monday to do it for you.16:06
adiroibanabentley: as is was use both in tal:condition and tal:content. I can change it if required16:07
abentleyadiroiban: I am asking questions so I understand your patch better.  I'm not implying your choices are wrong.16:07
adiroibanabentley: np. 16:08
abentleyadiroiban: So the only problem I see is the first one: putting search_lang on ILanguageSet.  The rest looks fine, now that I understand it.16:09
adiroibanabentley: ok. I will fix that.16:10
bachenninge, abentley: could one of you have a look at https://code.edge.launchpad.net/~bac/launchpad/ec2_attached/+merge/1527216:11
henningebac: looking16:11
abentleyadiroiban: The Interfaces in our "interfaces" directories are meant to correspond with model objects.  ILanguageSet shouldn't specify anything that LanguageSet can't provide, which is why search_lang didn't belong on ILanguageSet.  Does that make sense?16:16
adiroibanabentley: yep. I was not aware of that policy16:17
=== henninge changed the topic of #launchpad-reviews to: on-call: henninge, abentley || reviewing: bac, adiroiban || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
henningebac: stray blank line at line 8 of the diff.16:17
daniloshenninge, adiroiban: re bug-97293, there's a jslint warning: was that there before as well? if it was, never mind it, but if it wasn't, you should fix it first :)16:17
henningedanilos: oops, missed that16:18
adiroibandanilos: it was there16:18
adiroibani did not touched that line 16:18
=== abentley changed the topic of #launchpad-reviews to: on-call: henninge, abentley || reviewing: bac, - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
danilosadiroiban, ok, if you can figure out how to fix it, that'd be great, if not, never mind :)16:18
adiroibandanilos: I have no idea how to fix it 16:18
danilosadiroiban, ok, then just ignore it (or perhaps file a bug against launchpad-foundations)16:19
bachenninge: gone16:19
adiroibandanilos: i can read about JS closures and try to fix it ... if I will fail, will file a bug16:20
henningebac: I am surprised there are not more parameters to the Option constructor but I guess boolean values defaulting to False are the default, right?16:21
danilosadiroiban, I think this is best left for whoever made it such, the code where it happens does look ugly with a debugging function being defined in a loop16:21
bachenninge: i can double-check that16:21
henningebac: the Option 'force' above that looks the same ...16:22
bachenninge: yes, that was my example16:22
danilosadiroiban, it's not really a function definition, it's a YUI3 way of doing things, so I just wouldn't worry about it :)16:22
henningedanilos: I agree.16:22
adiroibandanilos: ok. then no need to file a bug?16:22
danilosadiroiban, you can if you want to, but don't spend time trying to fix it :)16:23
adiroibandanilos: ok16:23
henningebac: where does run get called? I see the extension of the parameter list but would expect that to happen at the call site, too.16:23
henningebac: oh, it's from EC2Command16:25
bachenninge: let me look.  it's part of the 'commandant' machinery16:25
henningebac, yes, I just just figured.16:25
henningebac: r=me16:26
bacthanks henninge.16:27
=== henninge changed the topic of #launchpad-reviews to: on-call: henninge, abentley || reviewing: -, - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
adiroibanabentley: regarding the addition of LanguageSearch interface. Should I put it in lp.translations.interfaces.language rather than lp.services.worlddata.interfaces.language ?16:30
abentleyadiroiban: As you explained, it has no purpose outside the context of the view.  Therefore, I suggest putting it next to the view.  I don't think it has value as a public API.16:32
adiroibanabentley: next to view means putting the interface in the same file ?16:33
abentleyadiroiban: Yes.16:34
adiroibanabentley: :) ok16:34
abentleyI specified that file here and in the review.16:35
adiroibanabentley: I have pushed the required changes. What would be the next step?16:48
abentleyadiroiban: I'll have a look.16:49
adiroibanabentley: what is the normal LP process. Should I add a comment, ping you... or both ?16:50
abentleyadiroiban: Normally, you'd add a comment saying you've made the requested changes.16:51
abentleyadiroiban: Why have you added ILanguageSetSearch to lib/lp/services/worlddata/interfaces/language.py ?16:51
adiroibanabentley: it should be an error.16:52
abentleyadiroiban: I don't understand.16:52
adiroibanabentley: it's just me being stupid . I'll fix that right away16:52
abentleyadiroiban: Okay.16:53
abentleyadiroiban: This looks good to me now.17:00
adiroibanabentley: sorry for messing it 17:00
abentleyadiroiban: np17:00
adiroibanabentley: ok. Should i ping you on Monday, or everthing is ok?17:03
abentleyadiroiban: Please ping me on Monday.17:04
adiroibanabentley: ok. Henning  told me I responsible for getting a review for your branch. I am also responsible for getting it merged into devel ?17:05
abentleyadiroiban: I'm not sure.  This is the first patch I've handled from a non-committer.17:06
adiroibanabentley: ok.17:06
abentleyadiroiban: It would be simpler if this wasn't week 4.17:07
henningeabentley, adiroiban: I'd say you are responsible as long as non of us says "I'll handle it from here on" ;)17:10
henningeadiroiban: So get back to us about it in a week and we'll take the landing off your hand.17:11
adiroibanhenninge: ok. no hurry to have them accepted/merged. I just wanted to learn the workflow17:11
henningeabentley: you can just land it, it will be credited to Adi.17:11
abentleyadiroiban: Have you signed the contributor agreement?17:12
henningeadiroiban: we are in feature-freeze this week because of the roll-out, that is the only reason for not going straight ahead.17:12
adiroibanabentley: yep17:12
adiroibanabentley: you can check here https://edge.launchpad.net/~contributor-agreement-canonical17:12
henningeadiroiban: It *may* be best to have one person land all your branches.17:13
henningebut I am not sure.17:13
adiroibanhenninge: ok. in that case, what is the next action I will have to do ?17:13
henninge_adiroiban: The next step is run the full test suite on it (on ec2) and land the branch17:15
henninge_adiroiban: oh, and since a week will pass, wou will also have to merge devel again to check for conflicts.17:15
adiroibanhenninge_: hm...  17:15
adiroibanhenninge_: i can do the devel merge... but not sure about ec2 testing17:16
henninge_adiroiban: once that is done (and resolved), ping me and I'll do the testing and landing.17:16
henninge_adiroiban: no, you cannot.17:16
adiroibanhenninge_ ok.17:17
henninge_adiroiban: you *could* run the test suite locally (make check), that'll take a few hours, but there is no need for that.17:17
adiroibanhenninge_: on my computer it will take days :)17:18
adiroibanhenninge_: but I'm running the translation test suite17:18
adiroibanand also testing for specific stories17:18
henninge_cool17:19
henninge_adiroiban: I will go home now. ttyl17:19
adiroibanhenninge_: have a nice evening17:20
=== henninge_ changed the topic of #launchpad-reviews to: on-call: abentley || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
henninge_adeuring: you too!17:20
adeuringhenninge: ...and you ;)17:21
=== beuno-lunch is now known as beuno
=== abentley is now known as abentley-lunch
=== deryck is now known as deryck[lunch]
=== abentley-lunch is now known as abentley
=== abentley1 is now known as abentley
=== deryck[lunch] is now known as deryck
deryckabentley, I'm about to go EOD, but I have a really simple css fix.  care to take a look, or can you do an offline review later?20:07
abentleyderyck: I'll do an offline review later.20:13
thumperderyck: I've grabbed the branch20:14
thumperderyck: and happy to look at it20:14
deryckthumper, excellent, thanks!20:14
deryckthanks anyway, abentley (since thumper has it now)20:14
deryckthumper, I've got to bail for the day.  Will check back later about the review.20:36
bacabentley: i've got a branch coming up which is targeted to the next cycle.  would you have time to look at it?21:17
abentleybac: probably.  on a call.21:17
bacabentley: ok.  i'll paste it when it shows up.  no rush.  thanks.21:18
=== salgado is now known as salgado-afk
bacabentley: when you have time: https://code.edge.launchpad.net/~bac/launchpad/bug-429802/+merge/1544921:28
=== matsubara is now known as matsubara-afk
=== abentley changed the topic of #launchpad-reviews to: on-call: abentley || reviewing: bac || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
marsabentley, rc-ping, would you feel comfortable reviewing the JavaScript for https://code.edge.launchpad.net/~mars/launchpad/fix-picker-result-parsing-487975/+merge/15436?  39 lines.21:55
* thumper looks21:57
abentleymars: There seems to be an extra space before the = in line 9 of the diff.22:00
abentleymars: Aside from that, it looks fine to me.22:01
marsabentley, nice catch.  Thanks for the review.22:02
abentleymars: np22:02
=== abentley changed the topic of #launchpad-reviews to: on-call: - || reviewing: bac || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
thumperabentley: heh, I said exactly the same thing :)22:04
marsthumper, thank you too.  I'll send off this branch, then finish up your reply22:09
thumpermars: ok22:09
marsthumper, the question I had about creating a new wrapper was the commit_message_listener function on line 25.22:11
thumpermars: I still feel like I'm missing something22:12
marsthumper, that has a green and a red flash on the link.  The current editor wrapper flashes the message itself.  I take it this is an extra, additional bit of flash?22:12
thumpermars: ah22:12
thumpermars: this is flashing the menu link not the editor itself22:13
thumpermars: if the commit message ends up being empty22:13
thumpermars: we hide the multiline editor22:13
thumpermars: show the link again, and flash the link22:13
marsah22:13
marsok22:13
thumpermars: it would be nice to be able to use widget.on('save')22:14
thumpermars: rather than widget.editor.on('save')22:14
thumpermars: is this possible, because at the moment it doesn't work22:14
marsyep22:14
marsit didn't work in PR2.  It probably works fine in 3.0 final.22:14
marsrather, that feature didn't work in PR2 - I tried to code it already, and gave up :)22:15
flacostemars: what is that?22:15
flacostethat's parsing the HTML representaion?22:15
flacostewhy is it doing that22:15
flacostewe shouldn't be doing that22:15
marsflacoste, one of many approaches I take it.22:15
flacosteyou should be requesting the field HTML output directly22:15
flacostethat's a deprecated approach22:16
marsthis is an original widget22:16
flacostewe talked about making that view a 40422:16
flacosteto fix that broken behaviour22:16
flacosteand it's the third time that somebody "fixes" it22:16
marsgood thing you didn't then :)22:16
flacostethat's crazy22:16
thumpermars: it doesn't work in 3.0 right now with the widgets we have22:17
marsthumper, yep, I know.  I would have to add it back into lazr-js, then publish that new version in LP before you could use it.22:17
thumperok22:17
marsthumper, so maybe in a release or two.  Your branch did show some serious API problems though.  I like that.22:18
marsflacoste, is there a bug for "Don't use the XHTML view" I can reference with an XXX?22:19
flacosteprobably, but it will have been marked as 'Fix released'22:19
marsflacoste, IIRC publishing instructions for the Right Way To Do It is on the UI Action items list22:19
marsok, I'll exhume the bug22:20
marsflacoste, I can't find the bug number for XHTML representations.  I'll just file a new one and mark it as an XXX.22:36
marsflacoste, anything else needed for an r-c?22:38
flacostemars: nope, just updated the m-p22:39
marsthanks22:39
thumpermars: so want to approve the js review? https://code.edge.launchpad.net/~thumper/launchpad/edit-commit-msg-link/+merge/1526022:42
marsthumper, doing so now22:42
thumpermars: thanks22:42
thumpersalgado-afk (or flacoste): https://code.edge.launchpad.net/~thumper/launchpad/edit-commit-msg-link/+merge/15260 rc?22:43
marsthumper, two small fixes, r=mars22:48
thumpermars: ok, ta22:48
marsthumper, to clarify the code, a quick comment with the same reason you gave me on IRC should be enough.22:51
flacostethumper: rc=me23:38
thumperflacoste: ta23:46

Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!