#launchpad-reviews 2010-02-22
<thumper> mwhudson: https://code.edge.launchpad.net/~thumper/launchpad/branch-push-permission/+merge/19834 if you have a minute
 * jpds would like https://code.edge.launchpad.net/~jpds/launchpad/fix_517839 looked into, please.
 * thumper looks
<mwhudson> thumper: done
<thumper> mwhudson: ta
 * wgrant is making use of that in another branch, and it seems good.
<thumper> wgrant: the county one?
<wgrant> thumper: Yeah.
<thumper> jpds: I have a few comments on the branch, just finishing some QA first
<wgrant> He's probably asleep.
* adeuring changed the topic of #launchpad-reviews to: on call: - || reviewing: - || queue [adeuring, jpds] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
* noodles775 changed the topic of #launchpad-reviews to: on call: - || reviewing: - || queue [adeuring, jpds, noodles775] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
* intellectronica changed the topic of #launchpad-reviews to: on call: intellectronica || reviewing: - || queue [adeuring, jpds, noodles775] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<noodles775> intellectronica: mine's a really easy one :) https://code.edge.launchpad.net/~michael.nelson/launchpad/499421-dont-grind-bm-to-a-halt/+merge/19846
<noodles775> 50 lines.
<intellectronica> noodles775: cool, i'll review it now
<noodles775> Thanks intellectronica.
<intellectronica> noodles775: r=me
<noodles775> Ta.
* intellectronica changed the topic of #launchpad-reviews to: on call: intellectronica || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<gmb> intellectronica: I have a branch that's 105-lines over the limit due to a couple of bits of refactoring (i.e. some of that 905-lines is due to moving large blocks of code). Would you be able to review it?
<intellectronica> gmb: sure, i'll review it
<gmb> intellectronica: Thanks. https://code.edge.launchpad.net/~gmb/launchpad/filebug-polling-for-jobs-bug-513193/+merge/19859.
<stub> https://code.edge.launchpad.net/~stub/launchpad/dbpolicy-syntax/+merge/19854 <- 210 lines of syntactic sugar and documentation
* intellectronica changed the topic of #launchpad-reviews to: on call: intellectronica || reviewing: gmb || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
* intellectronica changed the topic of #launchpad-reviews to: on call: intellectronica || reviewing: gmb || queue [stub] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<jtv> stub: I can review your branch after lunch
<gmb> intellectronica: I'm going to grab some lunch; feel free to ping me and I'll respond to any questions when I get back.
<intellectronica> gmb: ok. no surprises so far
<gmb> Cool.
<jtv> stub: and, typo in the doctest: "Much of our code does not know if the objects being retrieved are need"
<stub> I'm going to do future doctests in lolspeak
<jtv> stub: such as this?  "want to prove only accesses slave database resources"
<jtv> (also a double "if" in that sentence)
<stub> Go easy. I'm Australian.
<jtv> heh.  Looks like a very good branch otherwise, so I wouldn't have thought there was any need to.
<stub> Actually.... that last is technically correct I think (just clumsy and confusing)
* noodles775 changed the topic of #launchpad-reviews to: on call: intellectronica || reviewing: gmb || queue [stub, noodles775] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<noodles775> hi intellectronica, if and when you have time, this next branch is just a test-refactor, but is 700 lines:
<noodles775> https://code.edge.launchpad.net/~michael.nelson/launchpad/ppa-privatisation-test-refactor3/+merge/19864
<intellectronica> noodles775: sure. i'm just in the middle of gmb's 900 line branch review, and have stub's (smaller) branch after that, but will do yours after that.
<noodles775> Thanks.
<henninge> Oh, it's Monday!!!
<henninge> Hi intellectronica ... ;)
* henninge changed the topic of #launchpad-reviews to: on call: intellectronica, henninge || reviewing: gmb || queue [stub, noodles775] || 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: intellectronica, henninge || reviewing: gmb, - || queue [stub, noodles775] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
* abentley changed the topic of #launchpad-reviews to: on call: intellectronica, henninge, abentley || reviewing: gmb, -, stub || queue [noodles775] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
 * henninge was too slow
* henninge changed the topic of #launchpad-reviews to: on call: intellectronica, henninge, abentley || reviewing: gmb, noodles775, stub || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
* abentley changed the topic of #launchpad-reviews to: on call: intellectronica, henninge, abentley || reviewing: gmb, noodles775, henninge || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
* intellectronica changed the topic of #launchpad-reviews to: on call: intellectronica, henninge, abentley || reviewing: -, noodles775, henninge || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<abentley> henninge, your code for creating parent directories recursively seems to overlap with Transport.create_prefix.
<henninge> abentley: yes, I was wondering if that has been done elsewhere. I guess, I was too lazy to search ... ;-)
<henninge> abentley: thanks for the hint, I'll use that.
<abentley> henninge, interesting definition of lazy :-)
<henninge> abentley: well, coding is fun, isn't it? ;)
<abentley> henninge, isHidden looks like it will match on any path containing dots, such as foo/../bar
<henninge> abentley: good point ...
<henninge> abentley: I should apply normpath first, I guess.
<abentley> henninge, sure.  Alternatively, you could use bzrlib.osutils.splitpath and ensure that none of the path elements began with .
<henninge> abentley: or is ".." or "."
<abentley> henninge, yeah.  Probably normpath is simpler.
<abentley> henninge, with those changes I'd be happy with your branch.
<henninge> abentley: cool, thank you! ;)
* abentley changed the topic of #launchpad-reviews to: on call: intellectronica, henninge, abentley || reviewing: -, noodles775, - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<henninge> abentley: are you sure about create_prefix, though? It does not take a path. Maybe I don't understand "transport" correctly.
<henninge> abentley: if I have a file "foo/bar/bla.pt", how do I create that and the two directories above it?
<abentley> henninge, a transport represents a path.  So you create the transport for the path you want to exist and then call create_prefix.
<abentley> henninge, t = get_transport('foo/bar/bla.pt'); t.create_prefix(); t.put_bytes('bla.pt', 'contents')
<abentley> henninge, actually, the last would be just put_bytes('.', 'contents')
<henninge> abentley: oh cool, thanks.
<abentley> henninge, actually, I guess you're starting with a transport, so you'd do root_transport.clone('foo/bar/bla.pt')
<henninge> abentley: put_bytes does not like '.', it seems. http://paste.ubuntu.com/381669/
 * henninge tries to use basename, now.
<abentley> henninge, looking.
<henninge> abentley: the code snippet http://paste.ubuntu.com/381673/
<abentley> henninge, that's irritating; it works with mkdir.
<abentley> henninge, I suggest: https://pastebin.canonical.com/28208/
<henninge> abentley: thanks. Only it is "create_prefix". ;) You might be confusing it with ensure_base.
<abentley> henninge, okay, so ensure_base would probably be more appropriate.
<abentley> henninge, actually, no.
<henninge> abentley: no, I looked at the code and it's not recursive
<henninge> or iterative ... ;)
<abentley> henninge, ^
<henninge> right ;)
<gmb> intellectronica: What was the final outcome of your review of my branch? LP still says "pending" for your review and I haven't seen any emails about it.
<intellectronica> gmb: really? it was r=me, and i definitely approved it in the ui
<gmb> intellectronica: Hrm. Nothing showing on the mp at all. Can you re-approve it for the sake of seeing what happens?
<intellectronica> gmb: yes, approved again now. don't know what happened
<gmb> intellectronica: Thanks.
<noodles775> hi henninge, I'll be leaving soon, just wondering if you had any questions about the MP?
<abentley> BjornT, what's the status of lp:~bjornt/launchpad/form-overlay-render-by-default ?
<henninge> noodles775: sorry, I did not get  very far...
<henninge> noodles775: but no questions so far.
<noodles775> OK, thanks henninge.
<al-maisan> Hello there! I have a simple branch that fixes spurious test failures.
<al-maisan> https://code.edge.launchpad.net/~al-maisan/launchpad/fuzzy-test-525329/+merge/19885
<al-maisan> 46 lines
<al-maisan> anybody willing to review it?
* al-maisan changed the topic of #launchpad-reviews to: on call: intellectronica, henninge, abentley || reviewing: -, noodles775, - || queue [al-maisan] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<al-maisan> abentley: could you please take a look?
<abentley> al-maisan, sure.
<al-maisan> thanks!
<abentley> al-maisan, r=me
<al-maisan> abentley: thank you very much!
* abentley changed the topic of #launchpad-reviews to: on call: intellectronica, henninge, abentley || reviewing: -, noodles775, - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
* intellectronica changed the topic of #launchpad-reviews to: on call: henninge, abentley || reviewing: noodles775, - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<abentley> al-maisan, it might be profitable to allow an external time source to be used, but that would probably take more re-engineering.
<al-maisan> abentley: indeed.
<mrevell> Hey, anyone want the easiest review evah? :) It's the what's new for 10.02 https://code.edge.launchpad.net/~matthew.revell/launchpad/whatsnew-10.02/+merge/19892
 * jpds needs https://code.edge.launchpad.net/~jpds/launchpad/fix_520469/+merge/19890 looking into. :)
* jpds changed the topic of #launchpad-reviews to: on call: henninge, abentley || reviewing: noodles775, - || queue [mrevell, jpds] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
* jpds changed the topic of #launchpad-reviews to: on call: henninge, abentley || reviewing: noodles775, - || queue [mrevell] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<jpds> thumper: I fixed the issues you found on https://code.launchpad.net/~jpds/launchpad/fix_517839/+merge/19774
<thumper> jpds: cool
<thumper> jpds: done
<jpds> thumper: Can you please ec2land it for me? I don't have access to PQM.
<thumper> jpds: yep
<jpds> Awesome, thanks. :)
* abentley changed the topic of #launchpad-reviews to: on call: henninge, abentley || reviewing: noodles775, - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<thumper> https://code.edge.launchpad.net/~thumper/launchpad/no-none-subject/+merge/19902
<thumper> very trivial
<mwhudson> thumper: done
<rockstar> abentley, do you have time for my trivial bug?
<abentley> rockstar, looking
<rockstar> abentley, actually, I think it might be better to just pass the bugbranch.bug.default_bugtask into the DeletionCallable.
<rockstar> abentley, then I don't need the messy change to the tal.
<abentley> rockstar, I agree.  Actually, I'm not sure we should still be displaying it, but that needs more thought.
<rockstar> abentley, well, it's not doing any harm displaying it instead of the bug itself.
<abentley> rockstar, I'm about to EOD.
<rockstar> abentley, okay, I'll push this change so you can see it.
<abentley> Cool.
<thumper> mwhudson: ec2 land is complaining because you just approved without reviewing :)
<mwhudson> thumper: must be a bug in the ui
<mwhudson> :)
<mwhudson> thumper: done now
<thumper> mwhudson: ta
<rockstar> abentley, just waiting for lp to figure out the branch has changes now...
<rockstar> abentley, new diff there.
<abentley> rockstar, r=me.  Maybe you should change "deleted-items" into "deletion-items" since they haven't been deleted yet.
<rockstar> abentley, okay.  I'll do that.
<thumper> https://code.edge.launchpad.net/~thumper/launchpad/code-review-email-attachment-fix/+merge/19924 anyone?
 * thumper heads out for coffee
#launchpad-reviews 2010-02-23
<mwhudson> thumper: done
<thumper> mwhudson: ta
 * maxb has a merge, if someone's free: https://code.edge.launchpad.net/~maxb/launchpad/py2.6-importfascist-again/+merge/19927
<mwhudson> maxb: done
<mwhudson> maxb: want to set a commit message?
<maxb> oh, I always forget that
<maxb> any reason not to just copy the commit message of the single revision, for single revision branches?
<mwhudson> i guess that would be a sensible default
<maxb> I've set that
<mwhudson> thumper: hi, can you review a simple branch?
<mwhudson> thumper: found a problem in the incremental import stuff, luckily it's super easy
<mwhudson> maxb: your branch is in ec2 btw
<mwhudson> (has been for a while)
<thumper> mwhudson: sure
<mwhudson> thumper: https://code.edge.launchpad.net/~mwhudson/launchpad/incremental-oops/+merge/19931
<mwhudson> thumper: otherwise the incremental stuff seems to be going ok
<thumper> mwhudson: testing on staging?
<mwhudson> although the setup part before it starts importing revisions is a bit long
<thumper> mwhudson: done
<mwhudson> so maybe we should up the number of revisions we import
<mwhudson> hooray for QA i guess
<mwhudson> maxb: actually i lied there
<mwhudson> trying again...
<noodles775> Hi henninge, did you get a chance to start/finish my MP? If not, I'll ask todays ocr to take a look.
<henninge> noodles775: I did start and I will finish it this morning ...;-)
<noodles775> Great, thanks henninge !
<henninge> noodles775: review sent.
* henninge changed the topic of #launchpad-reviews to: on call: - || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<noodles775> Thanks henninge.
* gmb changed the topic of #launchpad-reviews to: on call: gmb || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
* bigjools changed the topic of #launchpad-reviews to: on call: gmb || reviewing: - || queue [bigjools] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<bigjools> an easy soyuz branch in the queue for ya gmb :)
<gmb> bigjools: Ta.
<bigjools> https://code.launchpad.net/~julian-edwards/launchpad/publish-copy-archives-bug-520520-getPubConfig-proc-accept/+merge/19955
* gmb changed the topic of #launchpad-reviews to: on call: gmb || reviewing: bigjools || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<bigjools> sorry for the terse cover note
<bigjools> I wrote a long one then my mail client crashed
<bigjools> I was too depressed to re-create it
<bigjools> so I am here to answer your questions
<intellectronica> noodles775: can i bother you for a code and ui review for a small lazr-js change? 20 lines of code, and the ui review is quite trivial too
<noodles775> intellectronica: sure.
<intellectronica> noodles775: lovely. sending an MP your way
<intellectronica> noodles775: https://code.edge.launchpad.net/~intellectronica/lazr-js/choiceedit-position/+merge/19956
<noodles775> intellectronica: thanks, I'll just finish off a branch for review, and then start it.
<intellectronica> cheers
<gmb> bigjools: No answers needed; it actually made sense for a change... r=me with some minor coding-style fixes
<bigjools> gmb: for a change? :)
<bigjools> thanks for the review
<gmb> np
<bigjools> blame vim's python mode for the closing brace positioning :/
* noodles775 changed the topic of #launchpad-reviews to: on call: gmb || reviewing: bigjools || queue [noodles775] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<noodles775> Hi gmb, this one is pure test refactoring, and is just over 800lines: https://code.edge.launchpad.net/~michael.nelson/launchpad/ppa-privatisation-test-refactor4/+merge/19961
<gmb> Eeesh.
<gmb> noodles775: Okay, I'll take a look
* gmb changed the topic of #launchpad-reviews to: on call: gmb || reviewing: noodles775 || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<gmb> seems like the week of the long branches.
<noodles775> Yeah, and it's the 4th pipe in the test refactoring! Thanks gmb, let me know if you want me to move some of the changes to the next branch to make it more manageable.
<gmb> noodles775: I like how "just over" means "+11%" ;)
<gmb> noodles775: But no, it's fine. I'll take care of it.
<noodles775> gmb: thanks.
<gmb> noodles775: r=me
* gmb changed the topic of #launchpad-reviews to: on call: gmb || reviewing: lunch || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<noodles775> Thanks gmb
<noodles775> intellectronica: I can see from the code what your lazr-js branch is meant to do, but when testing it in a browser, I can find a way to show the difference between trunk and your branch?
<intellectronica> noodles775: i'm not sure i understand the question. do you mean that it behaves in the same way the version in trunk behaves?
<intellectronica> noodles775: and if so, could it be a cache issue?
<noodles775> intellectronica: yes, with the two side-by-side, I haven't found a way to ...
<noodles775> ah...
<noodles775> intellectronica: http://people.canonical.com/~michaeln/tmp/intellectronica.ogv
<noodles775> intellectronica: forget that...
<noodles775> I just saw the issue (my end, too many tabs)
<intellectronica> noodles775: looks like it's working correctly. some of the entries you choose cause the box to go out of screen, and that's why the position is not as expected. as i explained in the MP, i can't think of a better way of handling that situation
<intellectronica> i wish i could cause the mouse pointer to move
<noodles775> intellectronica: yeah, I was just thinking the same thing ;)
<noodles775> intellectronica: code-wise, any reason for not using boundingBox.query('span.current') instead of iterating through all the spans?
<intellectronica> noodles775: no, i just didn't think of that. let me give that a try.
<intellectronica> noodles775: yeah, that works fine. changing.
<noodles775> Great, r=me.
<intellectronica> noodles775: thanks!
<jpds> Does anyone know if ec2test is having problems?
<noodles775> jpds: Twice in the past few weeks I've had an ec2 run not come back (gets stalled during windmill and then I have to terminate it). But I've just rerun them and it worked fine.
 * noodles775 -> lunch
<jpds> Two branches of mine were approved yesterday and still haven't landed. :/
* bac changed the topic of #launchpad-reviews to: on call: gmb,bac || reviewing: lunch || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
* bac changed the topic of #launchpad-reviews to: on call: gmb,bac || reviewing: lunch,- || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<bac> jpds: do you need me to re-submit them or will your reviewer do it?
<jpds> bac: They're https://code.launchpad.net/~jpds/launchpad/fix_517839/+merge/19774 and https://code.launchpad.net/~jpds/launchpad/fix_520469/+merge/19890
<bac> jpds: ok
<leonardr> bac: trivial branch: https://code.edge.launchpad.net/~leonardr/lazr.restful/prep-for-release/+merge/19968
<bac> leonardr: ok, i'll get right on it
* bac changed the topic of #launchpad-reviews to: on call: gmb,bac || reviewing: lunch,leonard || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
* bac changed the topic of #launchpad-reviews to: on call: gmb,bac || reviewing: lunch,- || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<bac> leonardr: that really was trivial
<leonardr> bac: yes, but gary dings me if i commit to lazr.* without an r=
<leonardr> and i agree with him
<bac> glad to help
<bac> jpds: both sent off.  cross your fingers
<jpds> bac: "What could possibly go wrong?"
<bac> nothing i can think of!
<adeuring> gmb, bac: could one of you do review this mp: https://code.edge.launchpad.net/~adeuring/launchpad/bug-513382/+merge/19970 (should be really simple)
<adeuring> noodles775: could you ui--review this ^^^^branch?
<bac> adeuring: yes
<adeuring> bac: thanks
* bac changed the topic of #launchpad-reviews to: on call: gmb,bac || reviewing: lunch,abel || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
* gmb changed the topic of #launchpad-reviews to: on call: gmb,bac || reviewing: - , abel || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<leonardr> intellectronica, if you're around, i need one more review of https://code.edge.launchpad.net/~leonardr/launchpad/multiversion-integration
<intellectronica> leonardr: ok
<leonardr> intellectronica: i'm preparing an incremental diff now
<intellectronica> leonardr: thanks
<leonardr> intellectronica: http://pastebin.ubuntu.com/382273/
<leonardr> writing up the changes now
<intellectronica> leonardr: why the change in doc/menus.txt?
<intellectronica> oh, ok, i'll wait then :)
<leonardr> intellectronica:https://code.edge.launchpad.net/~leonardr/launchpad/multiversion-integration/+merge/19346
<intellectronica> leonardr: so my question (above) still stands
<leonardr> intellectronica: what's missing from the explanation i gave?
<leonardr> oh, menus.txt
<intellectronica> yes, i don't understand why the error changed
<leonardr> sorry, i thouht you were talking about folder.txt
<leonardr> i'll write another comment
<intellectronica> cool
<leonardr> intellectronica: added
<intellectronica> ah, got it
<intellectronica> leonardr: r=me
<leonardr> ok, time to test it and hopefully see no more failures
<noodles775> Hi adeuring, I can, but it'd be great to first request a UI review from one of the people with an asterisk at: https://dev.launchpad.net/UI/Reviews - I can then do the second UI review.
<noodles775> This'll help us build up more ui reviewers.
<noodles775> But if it's urgent, I'll do it straight away.
<adeuring> noodles775: sure, but this one is really trivial...
<noodles775> adeuring: ok, I'll do it straight away then :)
<bac> adeuring: wow, the reviews today keep getting more trivial
* bac changed the topic of #launchpad-reviews to: on call: gmb,bac || reviewing: - , - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<noodles775> adeuring: I think you've got the wrong bug linked on your MP ;) ui=me, thanks!
<noodles775> adeuring: I think you've got the wrong bug linked on your MP ;) ui=me, thanks!
<adeuring> noodles775: thanks! and right, the bug is 513380...
* bac changed the topic of #launchpad-reviews to: on call: gmb,bac || reviewing: - , jtv || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<al-maisan> hello gmb, here's a branch that remedies the merge conflict between stable and db-devel, https://code.edge.launchpad.net/~al-maisan/launchpad/merge-conflict/+merge/19981
<al-maisan> Could you please have a look at it?
<gmb> al-maisan: Sure
<al-maisan> thanks!
* gmb changed the topic of #launchpad-reviews to: on call: gmb,bac || reviewing: al-maisan, jtv || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<gmb> al-maisan: Is there some extra data in the diff? It looks like there's other changes in there besides conflict resolutions.
<al-maisan> gmb: I actually merged stable into db-devel and then resolved the conflicts .. that's what you're seeing..
<gmb> Ah.
<gmb> al-maisan: Looks fair enough. r=me
<al-maisan> gmb: thank you very much!
<gmb> np
* gmb changed the topic of #launchpad-reviews to: on call: bac || reviewing: jtv || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<adeuring> bac, noodles775: could you please re-review my MP https://code.edge.launchpad.net/~adeuring/launchpad/bug-513382/+merge/19970 ?
<bac> adeuring: i will.  i'm in the middle of a long one and my lunch is fast approaching.  i'll do it later if that's ok
<adeuring> bac: sure, thanks
<noodles775> adeuring: I'll do it first thing tomorrow (as I'm off for dinner in a tick).
<adeuring> noodles775:  thanks!
<leonardr> rockstar, do you want to review https://code.edge.launchpad.net/~leonardr/lazr.restful/register-operations-when-version-list-is-known/+merge/19985 ?
<rockstar> leonardr, well, I'm a bit tied up at the moment.  You might be better served having bac look at it.
<leonardr> rockstar: sure, just wanted to give you right of first refusal since it's multiversion
<leonardr> bac: -^
<jtv> bac: sorry for the extensive drive-by damage :)
<al-maisan> hello bac, this branch resolves the "stable -> db-devel" merge conflicts, https://code.edge.launchpad.net/~al-maisan/launchpad/merge-conflicts-2/+merge/19994
<al-maisan> Can you please take a look?
<bac> al-maisan: is this different from the similar branch you did earlier today that gmb looked at?
<al-maisan> it is similar in the sense that it resolves the merge conflict
<al-maisan> hrmpff .. the diff generation takes ages
<al-maisan> bac: the diff is available now
<bac> al-maisan: the diff isn't available here yet.  propogation delay
<bac> and now i have it
<al-maisan> great!
<bac> al-maisan: done
<bac> thanks muharem
<al-maisan> bac: thank you!
<EdwinGrubbs> bac: can you review https://code.edge.launchpad.net/~edwin-grubbs/launchpad/bug-526001-edit-packaging-oops/+merge/19997
<bac> EdwinGrubbs: yes.  please add to the queue
* EdwinGrubbs changed the topic of #launchpad-reviews to: on call: Edwin || reviewing: jtv || queue [adiroiban(bug-509252-take-2), Edwin(bug-526001)] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<bac> leonardr: i didn't see your request.  i'll put it on the queue where it would've landed
* bac changed the topic of #launchpad-reviews to: on call: Edwin || reviewing: - || queue [adiroiban(bug-509252-take-2), leonard, Edwin(bug-526001)] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
* bac changed the topic of #launchpad-reviews to: on call: bac || reviewing: abel || queue [adiroiban(bug-509252-take-2), leonard, Edwin(bug-526001)] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<bac> EdwinGrubbs: if you have a moment could you look at https://code.edge.launchpad.net/~adeuring/launchpad/bug-513382/+merge/19970 since it involves sprites?
<bac> adiroiban: i'd prefer to let henninge re-review your changes
* bac changed the topic of #launchpad-reviews to: on call: bac || reviewing: leonard || queue [adiroiban(bug-509252-take-2), leonard, Edwin(bug-526001)] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
* bac changed the topic of #launchpad-reviews to: on call: bac || reviewing: leonard || queue [adiroiban(bug-509252-take-2), Edwin(bug-526001)] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
* bac changed the topic of #launchpad-reviews to: on call: bac || reviewing: edwin || queue [adiroiban(bug-509252-take-2)] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<bac> EdwinGrubbs: you didn't add a test case to show an obsolete productseries
<EdwinGrubbs> bac: I can add that.
<bac> EdwinGrubbs: thanks
<thumper> rockstar: https://code.edge.launchpad.net/~thumper/launchpad/descriptions-for-merge-proposals/+merge/19564
<abentley> thumper, could you review https://code.launchpad.net/~abentley/launchpad/restricted-diffs/+merge/19999 ?
<thumper> oooh 19999
<thumper> abentley: yes
<abentley> thumper, thanks.
* bac changed the topic of #launchpad-reviews to: on call: - || reviewing: - || queue [adiroiban(bug-509252-take-2)] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
* sinzui changed the topic of #launchpad-reviews to: on call: - || reviewing: - || queue [adiroiban(bug-509252-take-2), sinzui] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<EdwinGrubbs> bac: looking at adeuring's branch, the only info that couldn't be displayed if he switched it to sprites would be the alt tag on the image. The size of the sprite is no problem.
<bac> EdwinGrubbs: would you mind adding that to the MP?
<EdwinGrubbs> bac: no problem
<mwhudson> thumper: around to review a branch?
<mwhudson> https://code.edge.launchpad.net/~mwhudson/launchpad/incremental-save-all-revs/+merge/20020 if you or anyone else fancies it
 * thumper looks
#launchpad-reviews 2010-02-24
<thumper> mwhudson: done
<thumper> rockstar: I'm back if you are ready to talk
<mwhudson> thumper: danke
<rockstar> thumper, I think this diff is good, just a few comments.
<thumper> rockstar: skype me up
<rockstar> thumper, hold on, Mo just got home.
<thumper> rockstar: https://code.edge.launchpad.net/~thumper/launchpad/use-last-rev-id/+merge/20023
<mwhudson> thumper: ta
<mwhudson> oh, i already said that
<thumper> :)
<mwhudson> thumper: just threw a very simple branch at you
<thumper> ok
<thumper> mwhudson: done
<mwhudson> thumper: ta
<mwhudson> thumper: really?
<thumper> mwhudson: via email
<thumper> mwhudson: patience
<mwhudson> thumper: ok
<mwhudson> :-)
<mwhudson> thumper: hey, one more review if you're around: https://code.edge.launchpad.net/~mwhudson/launchpad/pull-mirror-branches-separately-bug-520107/+merge/20032
<mwhudson> thumper: hm, initial email is probably wrong, hadn't pushed some revs
* jtv changed the topic of #launchpad-reviews to: on call: jtv || reviewing: - || queue [adiroiban(bug-509252-take-2), sinzui] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<bigjools> jtv: still reviewing?
<jtv> bigjools: yes, chuck it on the queue.  otp.
* bigjools changed the topic of #launchpad-reviews to: on call: jtv || reviewing: - || queue [adiroiban(bug-509252-take-2), sinzui, bigjools] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<bigjools> ta
<noodles775> Hi adeuring, I do realise you were thinking about accessibility when you made the decision, I'm just not sure I understand what benefit an img+alt has over a span+title+content? Is it specifically that the alt will be read, but the title attribute won't?
<adeuring> noodles775: yes; screnreader should present images via their alt tag content
<adeuring> s/screnreader/screenreaders/
<noodles775> adeuring: yes, but not the title? I always *assumed* (perhaps wrongly), that titles were read by default too. Hrm.
<adeuring> noodles775: I am also not 100% sure, but I think it is good to follow standards ;)
<adeuring> noodles775: also, the content and purpose of the title and alt attribute are, while similar, different in theier detailed meaning
<noodles775> adeuring: yes, theoretically, sure, but in practise I've usually seen people define both for images, and title's only for other elements (as they're just wanting the tool-tip).
<adeuring> noodles775: right, but "tooltip" is a good summary for the spec of the title attribute I quote in the MP. But that's different from a textual representation of an image
<adeuring> i.e, from the alt atttribute
<noodles775> yep, it's not an accessibility feature.
<noodles775> adeuring: so the way I was thinking about the issue, the "3 out of 4 heat flames" would be the textual content (displayed by default when no styles are used etc.), but switched to a graphical representation if stylesheets are enabled. Yes, it's not perfect (ie. content hidden by stylesheet), but it would have a title and content, and allow us to use our sprites infrastructure.
<noodles775> But I'll butt-out of the code discussion and leave that to bac/deryck and you to sort out.
<adeuring> noodles775: that works fine for browsers like Lynx. but many blind people use grapical software together with screenreaders. In this case your approach does not work: A screenreader should present only the visible elements of an htML page
<noodles775> adeuring: yes, I've used screenreaders before (a few years ago), and it was always just a setting, but I can't remember what the default was.
<noodles775> (eg. JAWS)
* jtv changed the topic of #launchpad-reviews to: on call: jtv || reviewing: - || queue [sinzui, bigjools] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
* jtv changed the topic of #launchpad-reviews to: on call: jtv || reviewing: sinzui || queue [bigjools] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
* jtv changed the topic of #launchpad-reviews to: on call: jtv || reviewing: bigjools || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<jtv> bigjools: conflict in your branch
<bigjools> bah
<jtv> blame noodles775
<bigjools> one sec
<jtv> it's yesterday's testfix branch
<bigjools> jtv: seems ok, I just merged with none
<jtv> hmm
<bigjools> just pushing up a new rev
<bigjools> see if it fixes
<jtv> bigjools: moving right along... is the "copy" in --copy-archive as a verb?
<bigjools> branch scanner may be having a spaz
<bigjools> noun
<bigjools> as in ArchivePurpose.COPY
<jtv> that's what I figured... makes the option name a bit misleading
<jtv> bigjools: is there any particular reason why process-accepted.py explicitly chooses read-committed isolation rather than default to it?
<jtv> I believe serializable used to be the default, so maybe this is just historic.
<bigjools> jtv: are you looking at the right branch?
<bigjools> the p-a change is already reviewed
<bigjools> https://code.edge.launchpad.net/~julian-edwards/launchpad/publish-copy-archives-bug-520520-publish-distro/+merge/19987
<bigjools> is the one I am waiting on :)
<jtv> bigjools: https://code.edge.launchpad.net/~julian-edwards/launchpad/publish-copy-archives-bug-520520-publish-distro/+merge/19987 is the one I'm looking at.
<jtv> This bit is all the way at the end.
 * bigjools looks again
<bigjools> errr that diff is total crap
<jtv> no it isn't... I need to search my desktops for a second review now
<bigjools> wtf - it has translations stuff in it
<jtv> Yup... did you branch off devel and then propose for merging into db-devel?
<bigjools> nope
<jtv> This MP is for db-devel.  Doesn't sound like that's what you wanted for this change...
<bigjools> GAH
<bigjools> let me re-submit it
<jtv> I'll take that as a "yes."
<bigjools> https://code.edge.launchpad.net/~julian-edwards/launchpad/publish-copy-archives-bug-520520-publish-distro/+merge/20049
<jtv> bigjools: now all of a sudden the "this is a simple branch" makes a lot more sense.
<bigjools> no kidding
<bigjools> I suck
<jtv> bigjools: I'll refrain from commenting on that.  There is, however another small matter.
<bigjools> haha
 * jtv breathes sigh in relief that his joke wasn't taken seriously
<jtv> 42	+        if archives.count() == 0:
<jtv> 43	+            raise LaunchpadScriptFailure("Could not find any COPY archives")
<jtv> Use "if archives.is_empty()" instead.
<bigjools> can't
<bigjools> it's sqlobj
<jtv> grrr
<bigjools> otherwise I'd use bool()
<bigjools> but that's got a bug in it since FOREVAH
<bigjools> this inconsistency between our Storm and SQLObj objects is really annoying now
<jtv> Stormicate getArchivesForDistribution?
<bigjools> yeah that'd be nice
<bigjools> but out of scope for this branch
<bigjools> I need to get this shit working by Friday
<jtv> Okay, then the idiomatic way of doing this nowadays is to fetch one arbitrary row and check if you get None.
<jtv> The implementation there is still crap in that it still sorts the query, but that's not your fault and may go away at some point.
<jtv> bigjools: then there's this in the test docstring: "It should only publish copy archives."  Come on, a bit more confidence!
<jtv> Something like "In this mode, it only publishes copy archives."
<bigjools> it's a unit test, not a doc test
<bigjools> I am telling you what it's testing
<bigjools> not what it will do
<jtv> Good answer :)
<bigjools> :)
<jtv> I see now that the option name is much clearer in context.  So I withdraw my objection to that.
<bigjools> good :)_
<bigjools> brb in 5, I depserately need more coffee
<jtv> you and me both, bro
<bigjools> ah Java
<jtv-afk> bigjools: did you find a more satisfactory solution for checking for an empty result?
<bigjools> nope
<bigjools> it's not the first time this came up
<jtv> hmm... there is a __nonzero__ on the sqlobject compatibility wrapper
<bigjools> yes, it's fucked
<jtv> oh :)
<bigjools> there's a bug about it
<bigjools> get a nice traceback when using it
<jtv> Then can I trouble you to make your comment a XXX, referring to either that bug or a new one for stormicating the query?
<bigjools> bug 246200
<mup> Bug #246200: SQLObjectResultSet.__nonzero__() implementation does not strip result ordering. <oops> <tech-debt> <Soyuz:New> <Storm:New> <https://launchpad.net/bugs/246200>
<bigjools> sure :)
<jtv> then with that, r=me
<bigjools> grassy ass
<bigjools> I just realised I need to tweak that script some more but I'll do a separate branch
<jtv> de nutter
<intellectronica> who wants to review a small and boring branch?
* intellectronica changed the topic of #launchpad-reviews to: on call: jtv || reviewing: bigjools || queue [intellectronica] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<intellectronica> jtv-eat: if you're still reviewing when you're back maybe you can have a look? mp is https://code.edge.launchpad.net/~intellectronica/launchpad/use-max-heat/+merge/20054
<al-maisan_> hello jtv-eat, you must be suffering withdrawal symptoms since you haven't reviewed Soyuz branches for a long time ;)
<al-maisan_> Here's one to make your day: https://code.edge.launchpad.net/~al-maisan/launchpad/oops-526969/+merge/20056
* al-maisan_ changed the topic of #launchpad-reviews to: on call: jtv || reviewing: bigjools || queue [intellectronica, al-maisan] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<leonardr> intellectronica, i'm going to need yet another review of that launchpad integration branch
<intellectronica> leonardr: hey, how about we trade reviews?
<leonardr> intellectronica, sure
<intellectronica> excellent
<intellectronica> leonardr: here's mine: https://code.edge.launchpad.net/~intellectronica/launchpad/use-max-heat/+merge/20054
<intellectronica> leonardr: can you maybe prepare an inc. diff for your changes?
<leonardr> intellectronica, i'm working on it
<leonardr> i need a few minutes to write a new comment
<al-maisan> intellectronica: maybe you should remove yourself from the queue then?
* intellectronica changed the topic of #launchpad-reviews to: on call: jtv || reviewing: bigjools || queue [al-maisan] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<intellectronica> al-maisan: good point
<intellectronica> leonardr: cool
<leonardr> intellectronica: https://code.edge.launchpad.net/~leonardr/launchpad/multiversion-integration/+merge/19346
<leonardr> intellectronica: it seems like having a MAX_HEAT in bugtask.py would still be useful. you still use it in two different files
<intellectronica> leonardr: r=me
<leonardr> intellectronica: apart from that, r=me
<intellectronica> leonardr: i use it in a test. i use the value as a default, but it will be gone with a following branch which will make sure we have max_heat populated
<leonardr> intellectronica: oh, one other question. why can't the bugheatview be a view on the bugtask?
<leonardr> (assuming views are desirable in general)
<intellectronica> leonardr: it's just a case of yagni. it wasn't such a brilliant idea to use a view to begin with.
<intellectronica> all we ever did with the view is initialize manually and call it
<leonardr> ok. i don't like views so i approve
<intellectronica> :)
* al-maisan changed the topic of #launchpad-reviews to: on call: - || reviewing: - || queue [al-maisan] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
* jtv changed the topic of #launchpad-reviews to: on call: jtv || reviewing: - || queue [al-maisan] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<jtv> al-maisan: your branch looks good, but one question: your tests do things to job.status to show how it affects dispatch_time_estimate_available, but it'd be nice to see buildstate affect it as well... is that in there somewhere?  If not, is it doable?
* jtv changed the topic of #launchpad-reviews to: on call: jtv || reviewing: al-maisan || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<al-maisan> jtv: I can add a test that manipulates the buildstate as you suggested above
<jtv> al-maisan: something simple would do.  I'm mostly worried about protecting my fascist reputation.  ;-)
<al-maisan> :-)
<al-maisan> jtv: actually there are quite a few pre-existing tests involving builds in states other than NEEDSBUILD in xx-build-record.txt
<al-maisan> and they ascertain that no dispatch estimate is requested and/or shown on the +build page
<jtv> al-maisan: then that answers my first question.  You're good to go.
<jtv> (BTW the code is not just functionally better this way but the TAL is cleaner, too.  Me like.)
<al-maisan> jtv: man, I love these "soft" fascists who even listen to what other people say ;)
<al-maisan> jtv: thanks :)
<jtv> al-maisan: careful now... there _will_ be a next review and you want me in a good mood.  :-)
<al-maisan> oh-oh
<al-maisan> sorry massa
<al-maisan> ;)
<jtv> al-maisan: btw if you request a "code" review (I set the type myself in this case) and set a commit message, you can combine the headless testing and the landing with a mere "./utilities/ec2 land <merge-proposal-url>"
<al-maisan> jtv: great!
<al-maisan> will try that
<jtv> No need to spell out "[r=myfavoritereviewer][ui=thisisnotwindows][bug=123456][listening-to=Pet Shop Boys Best Slayer Covers] This branch fixes a bug."
<al-maisan> jtv: very nice indeed.
<bdmurray> could somebody review my branch for bug 527174?
<mup> Bug #527174: logo_link missing from "team" object in API <Launchpad Registry:In Progress by brian-murray> <https://launchpad.net/bugs/527174>
<bdmurray> al-maisan: ?
<al-maisan> bdmurray: I am sorry but I am just about to finish the day.
<al-maisan> jtv: are you still around?
<jtv> al-maisan: yes
<jtv> got another one?
<al-maisan> bdmurray: has one ^^
<al-maisan> Good night folks!
<jtv> bdmurray: I'll get to it in a moment
<jtv> bdmurray: can you put yourself on the queue?
<bdmurray> jtv: okay, thanks
<bdmurray> jtv: queue?
* jtv changed the topic of #launchpad-reviews to: on call: jtv || reviewing: - || queue [bdmurray] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<bdmurray> ah! that's not too complicated
<jtv> bdmurray: wehre's your merge proposal?
<bdmurray> jtv: apparently I missed that step - https://code.edge.launchpad.net/~brian-murray/launchpad/api-export-person-logo_link/+merge/20074
<jtv> bdmurray: there are no tests!
<jtv> on call: jtv || reviewing: bdmurray || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
* jtv changed the topic of #launchpad-reviews to: on call: jtv || reviewing: bdmurray || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<jtv> oh, I see it.
<jtv> bdmurray: looks simple enough... can't think of anything that might go wrong, but who else did you discuss the change with?
<jtv> (Sorry to be a stickler, but I'm suspicious of things that can't possibly go wrong :-)
<bdmurray> jtv: I haven't discussed it with anyone.  Should I do that with someone on the registry team?  Additionally, are the steps to contribute to Launchpad detailed anywhere?
<jtv> bdmurray: they should be somewhere on dev.launchpad.net.  We normally require a pre-implementation call where you discuss your approach with another developer as a basic sanity check.
<jtv> You know, someone who might say "you can't do that!  Someone is relying on this method 1,000Ã per second and including an image makes it too slow!"
<jtv> bdmurray: ah, found it... https://dev.launchpad.net/PatchSubmission
<jtv> (This is tailored to outside contributors, of course)
<jtv> Normally we also write cover letters that explain the change to the reviewer (and future historians), say which tests to run, and what "make lint" had to say about the branch.
<bdmurray> I didn't see that in the wiki page.  This process seems really complicated to me.
<jtv> It is.  We have to be careful with the codebase or things can go really horribly wrong.  But in this case tbh I can't think of any additional effort that might be useful.
<jtv> Most changes aren't this easy, or they'd be riding along on the back of a larger branch.
<jtv> bdmurray: I've approved it.
<jtv> bdmurray: can you land it yourself or would you like me to do it for you?
<bdmurray> jtv: okay, thanks.  What happens next then?  I'd like to run the ec2 test if possible - I was trying to set that up also.
<jtv> bdmurray: several ways to do that.
<jtv> When you've pushed all your changes, you can run "./utilities/ec2 test --headless"
<bdmurray> right with -s "[r=jtv] ...?"
<jtv> No, this is when you just want to run a test, before you've got approval to land.
<jtv> It can be useful if you're expecting to wait a while before review, and you want to use the time to have EC2 figure out what tests you just broke.
<jtv> Second, you can also test-and-land: "./utilities/ec2 test --headless -s '[r=jtv][ui=none][bug=1235] Frumbil the xeniogarb.'
<jtv> That runs the tests and sends your branch off to pqm as soon as it's done.
<jtv> But third, if you've met a few conditions, you can just "./utilities/ec2 land <URL to merge proposal>"
<jtv> (Where --headless is implied)
<jtv> This runs your tests, and if successful, composes the "[r=..." commit string and sends the branch to PQM for landing.
<jtv> The conditions you need to meet are:
<jtv>  * You must have an Approve vote on the mp.
<jtv>  * The mp must be in Approved state.
<jtv>  * The Approve vote must have a review type of "code"âleaving it blank won't do, so try to request this specific review type if you remember to.
<jtv>  * The commit message on the MP must be set; write a short single-line description of the branch there, like I just did for you.
<jtv> <eol>
<bdmurray> jtv: okay, thanks for all that information
<jtv> bdmurray: no worries...  I think you have rights to land this; if not, let one of us know and we can do it for you.
<bdmurray> jtv: okay, I've started the test and will let you / someone know
<jtv> bdmurray: good luck, and thanks for the patch!
<jtv> Doesn't always feel like people are grateful when you hit a process like this, I know.
* jtv changed the topic of #launchpad-reviews to: on call: - || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<henninge> jtv: I just pushed my pottery-to-slave branch. I thought you might be interested ... ;-)
<henninge> jtv: bug I am also about to EOD, so we can discuss it in the morning.
<jtv> henninge: cool
<henninge> jtv: pbuilder was really helpful when trying it out.
<jtv> henninge: what work does it take out of your hands?
<henninge> jtv: it builds a base hardy (or whatever you ask) chroot, keeps it nicely in a tarball.
<jtv> oh, nice!
<henninge> jtv: I just go pbuilder --login and get a shell in the chroot.
<jtv> very nice
<henninge> after exit, all is cleaned up. All changes forgotten (unless you ask for it not to).
<jtv> henninge: anyway, my OCR day is done.
<henninge> jtv: see you tomorrow!
<jtv> bis morgen!
<henninge> ;-)
 * henninge has to learn these things in Dutch.
* EdwinGrubbs changed the topic of #launchpad-reviews to: on call: Edwin || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<abentley> EdwinGrubbs, could you review this merge proposal, please? https://code.edge.launchpad.net/~abentley/launchpad/job-db-user/+merge/20081
* EdwinGrubbs changed the topic of #launchpad-reviews to: on call: Edwin || reviewing: abentley || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<EdwinGrubbs> abentley: sure
<abentley> EdwinGrubbs, thanks.
<EdwinGrubbs> abentley: TwistedJobRunner.runFromSource() has an error_utility parameter that JobRunner.runFromSource() does not have, so I'm wondering why the dbuser parameter was added to JobRunner.runFromSource() despite that class ignoring that argument.
<EdwinGrubbs> abentley: it seems like it would be good for JobRunner.runFromSource() to at least warn the user when they are passing in an argument that is ignored.
<abentley> I added the dbuser parameter to allow the JobRunners to support polymorphism.  Otherwise, we would have to switch how we called runFromSource according to the jobRunner class.
<abentley> EdwinGrubbs, we always pass in the dbuser argument, so warning the user would result in endless warnings.
<EdwinGrubbs> abentley: why wasn't the error_utility parameter added to JobRunner then?
<abentley> EdwinGrubbs, it's not currently used.
<EdwinGrubbs> abentley: but it seems like that would fall under the same polymorphism argument. All the JobRunner class' runFromSource() should have the same parameters.
<abentley> EdwinGrubbs, I don't thing that it is true for optional parameters.
<abentley> EdwinGrubbs, s/thing/think/
<EdwinGrubbs> abentley: I don't buy that. If someone is trying to call runFromSource() without checking whether the class supports the dbuser parameter, they could just as easily do that with the error_utility parameter. Otherwise, it will raise an exception when you pass in too many arguments.
<abentley> EdwinGrubbs, the difference is that dbuser is a mandatory parameter.  You need it for the TwistedJobRunner to work, which means you must supply it to all JobRunners.
<abentley> EdwinGrubbs, however, I can remove the error_utility parameter.  It just seems out of scope for the change I'm making.
<EdwinGrubbs> abentley: If you just add error_utility=None to JobRunner, it will be optional and the parameter lists will match, so you won't have to check which subclass of JobRunner you have before calling removeFromSource().
<abentley> EdwinGrubbs, I don't have to check which subclass of JobRunner I have anyhow!
<abentley> EdwinGrubbs, it's an unused parameter.
<abentley> EdwinGrubbs, there are no callsites that supply that parameter, so there are no callsites that care about whether it's supported.
<EdwinGrubbs> abentley: ok, that's fine too. I just didn't know if there were some callsites passing in an ignored error_utility, which would mean more work for you than a single line of code change.
<EdwinGrubbs> abentley: you read my mind
<abentley> EdwinGrubbs, I'm confused.  What would you like me to do.
<EdwinGrubbs> abentley: feel free to remove the error_utility. I just wanted the method parameters to match.
<EdwinGrubbs> abentley: the only other change I would like is some info about the dbuser parameter in the docstring about whether it is ignored or not.
<EdwinGrubbs> abentley: r=me
<abentley> EdwinGrubbs, roger wilco.
<abentley> EdwinGrubbs, changes pushed.
<EdwinGrubbs> abentley: I don't need to see them. feel free to land it.
<abentley> EdwinGrubbs, sure.  Was just letting you know.
<jelmer__> jtv, hi
#launchpad-reviews 2010-02-25
* EdwinGrubbs changed the topic of #launchpad-reviews to: on call: - || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<noodles775> BjornT: Hi! Can you do a CP review for me? https://code.edge.launchpad.net/~michael.nelson/launchpad/499421-dont-grind-bm-to-a-halt/+merge/19846
<noodles775> I've added QA info to the MP there, and Julian's request to have it CP'd is on the linked bug.
<BjornT> noodles775: the bug that you linked to in the XXX is marked as fix released
<noodles775> Yes, we thought the issue had gone, but it's happened again twice in the last two days (we're uncertain of the cause, this CP is just to ensure the buildd-manager doesn't die when it happens).
 * noodles775 re-opens the bug.
<noodles775> ie. this CP isn't fixing the bug, just improving the situation when it occurs.
<BjornT> noodles775: right. i just wanted to know more about the issue, and wasn't sure whether the bug status was wrong, or you linked to the wrong bug
<BjornT> noodles775: changing the summary and description of the bug would be good as well. the way it reads now, that CP does fix the bug, since the build farm doesn't grind to a halt anymore
<noodles775> BjornT: Yep, I thought I should wait until it was CP'd before doing that, but I can do it now.
<BjornT> noodles775: well, ideally there should be one bug for this CP, and one bug for the XXX. re-defining the meaning of bugs should be avoided, since if you go back to look at this CP later, you will get confused.
<noodles775> BjornT: True, so this bug is really the CP (ie. ensuring it doesn't grind to a halt), and I'll create another bug for the on-going issue (I'll have to update the XXX in devel, but that's easy enough). Would that work for you?
<BjornT> noodles775: yeah, thanks
<noodles775> Thanks BjornT.
<al-maisan> Hello noodles775, I prepared a branch that merely updates the sample data in db-devel, https://code.edge.launchpad.net/~al-maisan/launchpad/sampledata-update/+merge/20112
<al-maisan> Could you please take a look?
<noodles775> al-maisan: great! I'll be there soon...
<al-maisan> noodles775: thanks!
* noodles775 changed the topic of #launchpad-reviews to: on call: noodles775 || reviewing: sinzui || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<salgado> noodles775, can I add one to the queue?
<noodles775> salgado: please do!
* salgado changed the topic of #launchpad-reviews to: on call: noodles775 || reviewing: sinzui || queue [salgado] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<salgado> noodles775, thanks.  I'm just wrapping it up and the mp should be ready soon
* noodles775 changed the topic of #launchpad-reviews to: on call: noodles775 || reviewing: salgado || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<noodles775> hrm... no MP yet.
<adeuring> noodles775: I have another comment on hiding text via a CSS rule in https://code.edge.launchpad.net/~adeuring/launchpad/bug-513382/+merge/19970 ;)
<noodles775> adeuring: ok, I'll take a look soon.
<salgado> noodles775, the mp should be up now
* noodles775 changed the topic of #launchpad-reviews to: on call: noodles775 || reviewing: salgado || queue [noodles775] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<noodles775> intellectronica: can I tempt you to a UI review (should be straight-forward): https://code.edge.launchpad.net/~michael.nelson/launchpad/444988-remove-reference-to-karmic-on-ppa-page/+merge/20114
<intellectronica> noodles775: sure, but first i have to finish a review i'm doing for deryck
<noodles775> intellectronica: thanks, there's no rush.
<noodles775> salgado: you mention this is a temporary thing, what will happen to all the account records that are created during the window before the refactoring of the Account table?
<intellectronica> noodles775: i like your change. i think it really improves the experience. ui*=me.
<salgado> noodles775, we're going to move the openid_identifiers to a separate table and then drop Account*
<noodles775> intellectronica: ta.
<noodles775> salgado: ok, so none of it will be needed. Great.
<bigjools> noodles775: crack bash for you in the queue :)
<noodles775> niice :)
* noodles775 changed the topic of #launchpad-reviews to: on call: noodles775 || reviewing: salgado || queue [noodles775, bigjools] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
* noodles775 changed the topic of #launchpad-reviews to: on call: noodles775[lunch] || reviewing: - || queue [noodles775, bigjools] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
* bigjools changed the topic of #launchpad-reviews to: on call: noodles775[lunch] || reviewing: - || queue [noodles775, bigjools*2] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<leonardr> bac, i'm ready to land a big launchpad branch but i just realized i never got anyone to sanity-check my changes to download-cache. can you do it? it should only take a minute
<leonardr> http://paste.ubuntu.com/383669/
 * bac looks
<bac> hurrah for updating launchpadlib.  i hope 1.5.5 is a real release.  r=bac
<bac> btw leonardr, have you played with thekorn's ilaunchpad-shell?  it's pretty cool.
<leonardr> bac: i haven't played with it. but 1.5.5 is a real release
* noodles775 changed the topic of #launchpad-reviews to: on call: noodles775 || reviewing: bigjools || queue [noodles775, bigjools] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<noodles775> Hi sinzui, would you have time to do a second UI review for https://code.edge.launchpad.net/~michael.nelson/launchpad/444988-remove-reference-to-karmic-on-ppa-page/+merge/20114 ?
<sinzui>  noodles775: yes
<noodles775> Thanks.
* noodles775 changed the topic of #launchpad-reviews to: on call: noodles775 || reviewing: thumper || queue [noodles775] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<sinzui> noodles775: adeuring: bac: are all of you aware of the invisible-link that that are links use to generate real text for text browsers, visually impaired users, spiders, and our own test browser?
<adeuring> sinzui: well, bac said that basically
 * adeuring needed this head-upo
<adeuring> erm, heads-up
<sinzui> Our text checks in the test suite did not break when we switched to sprites because the text is still rendered
<noodles775> sinzui: nope...
<adeuring> sinzui: but the point here is that we create new text
<sinzui> adeuring: konquerer and webkit may want the format to be:
<sinzui>     <span class="sprite heat-3">&nbsp;<span class="invissible-link">accessible test</span></span>
<sinzui> adeuring: You will see similar markup if you visit a page with a disabled link, or even an edit icon next to a value
<adeuring> sinzui: my point is that the invisible text for the heat icons will appera 50 times on bug search pages
<adeuring> this makes a search for "heat" really confusing
<sinzui> person icons an milestone icons also appear many times
<adeuring> sinzui: right. But in this case, we _want_ to show the person's/milestone's name, don't we?
<sinzui> adeuring: bug heat is not worthy of an exception given that subscribers are a greater example that we do not think is a problem
<sinzui> and I want to know the heat of each item
* adiroiban changed the topic of #launchpad-reviews to: on call: noodles775 || reviewing: thumper || queue [noodles775,adiroiban(bug-359180-take-2)] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<adeuring> sinzui: well, we don't want to remove the heat icon entirely.
<adeuring> the question is about the right way to present it on the page
<bac> sinzui: i think adeuring's point is a search for a name will rightly bring up pages where she is a subscriber.  but a search for 'heat' will bring up every bug page since every bug page has a heat display and indexable invisible text
<bac> hmm, is that true?
<sinzui> that is true for the terms launchpad and bug
<bac> ah, a stronger counter-point
<adeuring> bac: you found another isuue with invisible text ;) my point was: Do a simple browser search  (crtl-f for firefox) on a bug listing
<sinzui> I can point you to a bug in blueprint where the complaint is that you cannt search for "user" and get a short list of blueprints about users
<adeuring> bac: but thanks for finding the search engine issue!
 * sinzui may have closed it
<bac> adeuring: if i created a new novel argument it is because i misunderstood your original.
<sinzui> adeuring: you are engineering a exception which is costly to build and maintain. Need I remind everyone that the link/image exceptions in answers and bugs caused months of bug fixes!
<adeuring> bac: that the best way how misunderstandung can out, right ;)?
<adeuring> sinzui: can you explain a bit?
<sinzui> adeuring: bac: we should do this the standard way, feel free to report an bug about a *universal* problem where ambient text from icons cause bad search results.
<sinzui> adeuring: we choose sprites because page load times are the *most* common problem for our users. Other issue are not as important
<sinzui> If we change our rules for icon/images again, someone needs to fix all the exceptions in the pages. We really do want to use our standards to ensure we can adapt quickly
<adeuring> sinzui: OK,  I understand this point. But tehre is a difference between images that are used for "decorative" purposes, like the bug icons with different colours on bug linstsing, and images like the heat icons. The incofrmation encoded in the colour of the heat icon is "importance", and that is shon in its own column. People icons are just a decoration for a person's name, which is displayed indenedently and delibrately.
<adeuring> while the bug heat icon is unque (foer each row of a listing)
<adeuring> tehre is not other element carrying theis data
<sinzui> So is the bug status and badges
<adeuring> so, we can completely drop the alt attribute of of a person icon without causing any accessibiliy problems
<adeuring> or of a milestone
<jtv> noodles775: got one for you as well
<noodles775> jtv: great, pop it in the queue (I did see it earlier and was planning to get to it when the queue emptied..., but it hasn't yet).
<jtv> noodles775: ok, thanks
* jtv changed the topic of #launchpad-reviews to: on call: noodles775 || reviewing: thumper || queue [noodles775,adiroiban(bug-359180-take-2),jtv] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<henninge> noodles775: is it ok if I add mine to the queue, too?
<noodles775> salgado: what was it that I need to add to be able to log in locally now? add testopenid.dev to /etc/hosts?
<noodles775> henninge: please do, I'm doubtful that I'll get to it, but the next OCR will :)
<henninge> noodles775: ok, understood ;-)
 * noodles775 hopes to have a second OCR person around next week!
* henninge changed the topic of #launchpad-reviews to: on call: noodles775 || reviewing: thumper || queue [noodles775,adiroiban(bug-359180-take-2),jtv, henninge] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<noodles775> salgado: nm, that worked.
<salgado> noodles775, yeah, just adding that to /etc/hosts should do it
<salgado> noodles775, it's done by rocketfuel-setup as well, in case you use it
<henninge> noodles775: who is doing your branch? ;)
<noodles775> henninge: no one... yet :)
<henninge> noodles775: done, r=me ;)
* henninge changed the topic of #launchpad-reviews to: on call: noodles775 || reviewing: thumper || queue [adiroiban(bug-359180-take-2),jtv, henninge] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<noodles775> henninge: thankyou sir!
<noodles775> jtv: would it be safer to forbid the running of the script unless LPCONFIG was dev?
<jtv> noodles775: it's not set by default, and I don't want to add _too_ much safety or it'll just break needlessly.
<jtv> We have a belt; this is suspenders.
<jtv> (One suspender is the id check, the other the config check)
* noodles775 changed the topic of #launchpad-reviews to: on call: - || reviewing: - || queue [adiroiban(bug-359180-take-2), henninge] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<jtv> noodles775: thanks for the review!
<jtv> noodles775: I'm a bit worried though that being this strict would stop people from playing with customized configs.
<jtv> I realize it's always a tradeoff, but there's also the "do we have a really large production-like table" test.
<noodles775> jtv: well, if they're playing with customized configs, they're not going to have a problem spending 10 seconds to adjust the script if they want to.
<jtv> noodles775: true, but this script already has another check to ensure it's not running on production; how many scripts do we have (without ever any trouble) that are just as dangerous without anyone ever adding checks like this?
<jtv> I mean, if I hadn't added the check, would the average reviewer have thought of adding it?
<noodles775> jtv: which scripts? make schema?
<jtv> noodles775: I guess, though I've no idea whether that has any checks.
<noodles775> jtv: yeah, I'm not sure. All the other scripts that I can think of *add* info, not delete.
<jtv> launchpad-database-setup
 * noodles775 looks
<jtv> mock-code-import ("warning! run make schema first!")
<jtv> We also have a script now, apparently, to remove private data.  There may be more.
<noodles775> Yep, you're right.
<jtv> So I don't want to spend my time guarding against the admin who accidentally goes through the rigmarole for running scripts against a production db, with the --force option added.
<noodles775> yeah, I agree. I was more worried about the situation where a person runs it against a smaller DB with a different config.
* rockstar changed the topic of #launchpad-reviews to: on call: rockstar || reviewing: - || queue [henninge] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<jtv> noodles775: there's a good chance that the script might fail, and I'd estimate the risk of them running "make schema" by accident considerably larger.
<jtv> (from shell history, f'rinstance)
<noodles775> jtv: I just thought it would have been a simpler implementation, not more difficult (ie. LPCONFIG == 'development'), but yep, I'm updating the MP now.
<jtv> noodles775: thanks for being open to my rants.  :)
 * jtv goes for a quick bite
* jelmer changed the topic of #launchpad-reviews to: rockstar || reviewing: - || queue [henninge,jelmer]  || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
* jelmer changed the topic of #launchpad-reviews to: ON CALL: rockstar || reviewing: - || queue [henninge,jelmer]  || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
* jelmer changed the topic of #launchpad-reviews to: on call: rockstar || reviewing: - || queue [henninge,jelmer]  || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<jelmer> argh, sorry!
* bdmurray changed the topic of #launchpad-reviews to: on call: rockstar || reviewing: - || queue [henninge,jelmer,bdmurray]  || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<henninge> Hi rockstar!
<rockstar> henninge, hi
<henninge> rockstar: I will have to leave on top of the hour. Do you think you can look at my branch soon, so I can answer any questions you might have?
<rockstar> henninge, I'm in the LP production meeting.  When that's over, I'll start my reviews for the day.
<henninge> rockstar: great! ;-)
<henninge> oh, it's still early in Colorado ...
* rockstar changed the topic of #launchpad-reviews to: on call: rockstar || reviewing: henninge || queue [jelmer,bdmurray]  || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<al-maisan> hello rockstar, could you please have a quick peek at this db-devel testfix branch: https://code.edge.launchpad.net/~al-maisan/launchpad/disable-mintime-tests/+merge/20143 ?
<rockstar> al-maisan, r=me
<al-maisan> rockstar: thanks!
<al-maisan> rockstar: sorry for the bother but I overlooked one tests that needed to be disabled as well: http://pastebin.ubuntu.com/383778/
<rockstar> al-maisan, it's good, land it.
<al-maisan> rockstar: good man! Thanks!
<rockstar> henninge, did lp.translations.pottery.build_slave change to lp.translations.pottery.buildd in a previous branch?
<henninge> no, that's in here.
<henninge> rockstar: ^
<rockstar> henninge, I don't see it in the diff
<henninge> rockstar: line 87
 * rockstar facepalms
<rockstar> Thanks.
<rockstar> henninge, r=rockstar
<rockstar> bdmurray, where's your merge proposal?
<henninge> rockstar: cool, thanks
<bdmurray> rockstar: well, it's actually been approved.  I ran it through ec2test and it passed but it hasn't been merge so I'm uncertain if it is a permissions thing or something else
<rockstar> bdmurray, who ran it through ec2 for you?
<bdmurray> rockstar: I ran the ec2 test myself
<rockstar> bdmurray, ah, and you don't have pqm access?
<rockstar> bdmurray, who reviewed the branch?
<bdmurray> rockstar: that's likely the problem - jtv reviewed it
<bdmurray> https://code.edge.launchpad.net/~brian-murray/launchpad/api-export-person-logo_link/+merge/20074
<rockstar> bdmurray, usually, you'd want jtv to land it for you.
<jtv> rockstar: I checked but he was in what I thought was the right team.  bdmurray: I let you try it yourself first, but apparently that didn't work.  Sorry for that; I can now land it for you as per yesterday's Plan B.
<bdmurray> jtv: okay, thanks!
<rockstar> jtv, you are awesome.
 * jtv checks
<rockstar> jtv, you don't have to check. You are awesome.
<rockstar> :)
<rockstar> I thought you had EOD'd already, so was going to volunteer.
<jtv> rockstar: you exaggerate, but pleasantly.  But EC2 is firing up.
<jtv> rockstar: I'm at fuzzy EOD.  Kick off some landings first for a pleasant morning.  :)
<rockstar> jelmer, is this diff up to date? Does it have a dependent branch?
* rockstar changed the topic of #launchpad-reviews to: on call: rockstar || reviewing: jelmer || queue []  || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<jelmer> rockstar: the bug471148 one? That's current now
<rockstar> jelmer, okay.
<jelmer> rockstar: It's a fair bit smaller now that Muharems prerequisite branch has landed.
<rockstar> jelmer, you should set the prerequisite branch on the mp anyway, because it's informational.
<jelmer> rockstar: Even if that branch has already landed?
<rockstar> jelmer, yeah, it's still a good idea to say "I based my work off of this work."
<rockstar> Not a necessity, but for history's sake.
<jelmer> rockstar: Ah, that makes sense. I'll keep that in mind for next time.
<rockstar> jelmer, your tests need comments/documentation
<rockstar> lib/lp/soyuz/tests/test_processor.py and lib/lp/soyuz/tests/test_publishing.py
<rockstar> Actually, it looks like some of the tests have docstrings, so you'll probably just need to add the docstrings to the ones that lack them.
<jelmer> ok
* sinzui changed the topic of #launchpad-reviews to: on call: rockstar || reviewing: jelmer || queue [sinzui]  || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<rockstar> jelmer, good on you for adding factory methods for your new work.
<sinzui> rockstar: bac: EdwinGrubbs: can one of you review my branch to fix a timeout: https://code.launchpad.net/~sinzui/launchpad/portlet-package-summary-timeout/+merge/20147
<rockstar> sinzui, I can when I'm done with jelmer.
<jelmer> rockstar: Thanks
<rockstar> jelmer, other than the missing docstrings, this looks like very good work.
* rockstar changed the topic of #launchpad-reviews to: on call: rockstar || reviewing: sinzui || queue []  || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<jelmer> rockstar: Thanks for the review :-)
<rockstar> sinzui, what specifically is the portlet you're talking about?
<rockstar> sinzui, ?
<sinzui> https://edge.launchpad.net/ubuntu/lucid <-- Upstream packaging
<rockstar> sinzui, ah, okay, thanks.
<rockstar> sinzui, did you cherry pick to staging or were you running raw queries?
<sinzui> rockstar: I ran raw queries comparing the oops query to the modified one
<gary_poster> bac, I have a gigantic diff for review (1984 lines), but the size mostly comes from the same mechanical change over and over again.  You might be interested because it addresses bug 496705, which you entered after we talked.  It also has some other changes that I think are generally interesting.  If you are not interested; I'll try for a salespitch on rockstar. :-)
<gary_poster> https://code.edge.launchpad.net/~gary/launchpad/bug491705/+merge/20156
<mup> Bug #496705: Single entry point for site-wide initialization needed <Launchpad Foundations:In Progress by gary> <https://launchpad.net/bugs/496705>
<rockstar> gary_poster, at 1984 lines, it better be one hell of a salespitch.  :)
<rockstar> sinzui, okay.
<gary_poster> rockstar: heh
<rockstar> sinzui, this looks good. r=rockstar
<sinzui> rockstar: thanks
* rockstar changed the topic of #launchpad-reviews to: on call: rockstar || reviewing: || queue []  || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<gary_poster> rockstar: um, so, 1600 lines are the following repetition:
<gary_poster> -#!/usr/bin/python2.5
<gary_poster> 988	+#!/usr/bin/python2.5 -S
<gary_poster> :-D
<gary_poster> please?
<rockstar> gary_poster, wow.  That's a lot of python.
<gary_poster> oh yeah
* jelmer changed the topic of #launchpad-reviews to: on call: rockstar || reviewing: || queue [jelmer/al-maisan] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<gary_poster> bac, I'm now changing my begging to rockstar.  rockstar, if you can't stand it, lemme know, and I'll go beg on the foundations channel. :-)
<rockstar> gary_poster, looking at it now.  It hasn't been too bad.
<gary_poster> rockstar: thank you
<rockstar> gary_poster, this seems to kill a lot of hacks we had.
<rockstar> (hacks I didn't even know about)
<gary_poster> rockstar: heh, yeah.  that was a definite goal. :-)
<rockstar> gary_poster, okay, cool.
<rockstar> gary_poster, also, it looks like you killed a lot of trailing whitespace issues.  Was that on purpose, or is your editor doing that?
<gary_poster> rockstar: on purpose because I tell my editor to do it. :-)  If you want me to separate I will.  I felt it was relatively small, but I'm obviously not in much of a bargaining position there. ;-)
<rockstar> gary_poster, well, it seems more effort to separate them than it's worth.
<gary_poster> ack
<gary_poster> thanks
<rockstar> gary_poster, however, while that does need to be done, it can cause spurious conflicts, so do so sparingly in the future plz.
<gary_poster> rockstar: ack, will do
<rockstar> I'm mostly impressed with the changes in buildout-templates/_pythonpath.py.in
<rockstar> You replaced 110 lines of code with 14 lines.  :)
<rockstar> gary_poster, these are pretty invasive changes, and we're getting nigh to closing PQM time.
<rockstar> gary_poster, are you confident these changes aren't going to have explosive bugs?
<gary_poster> rockstar: ack, see last para of cover letter.  Going to insert as soon as branch opens.
<rockstar> gary_poster, ah, hadn't seen that.
<rockstar> gary_poster, r=rockstar
<gary_poster> thanks rockstar!
<rockstar> gary_poster, the diff was surprisingly deceptive of the actual changes.  :)
* rockstar changed the topic of #launchpad-reviews to: on call: rockstar || reviewing: <no-one-because-rockstar-needs-a-break> || queue [jelmer/al-maisan] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<gary_poster> rockstar: yeah. :-)  I tried to talk about em in the cover letter, but still
<rockstar> gary_poster, the cover letter was actually more helpful than the diff in this case.  :)
<gary_poster> :-) good.  thanks again.
<abentley> rockstar, could you please review https://code.edge.launchpad.net/~abentley/launchpad/qa-ready/+merge/20162?
<rockstar> abentley, sure.
* abentley changed the topic of #launchpad-reviews to: on call: rockstar || reviewing: <no-one-because-rockstar-needs-a-break> || queue [jelmer/al-maisan/abentley] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<rockstar> abentley, I thought there was a page that had only the revno on it.  Did that not pan out?
<abentley> rockstar, I don't think there's any such page.
<rockstar> abentley, okay.
<rockstar> abentley, the use of a bzr transport to screen scrape confuses me.  What does it provide that a regular urllib or something doesn't?
<rockstar> Also, it sure would be nice if we had a successful-updates.txt for edge.
<rockstar> (not your problem though)
<abentley> rockstar, it respects all the configuration data that bzr does, so if you have proxies or things set up for bzr, those will apply to the transport also.
<rockstar> abentley, ah, okay.
<abentley> rockstar, also, it's familiar, and I don't see any reason *not* to use it.
<rockstar> abentley, that's the reason I would have suspected, but the former is also a very good reason.
<rockstar> s/suspected/expected/
<rockstar> abentley, was there a technical reason is_present appears after main?
<abentley> rockstar, no.
<rockstar> abentley, so, it's not a big deal, but I usually expect functions to be defined before I see the calls.  It's still valid python, but hard to follow.  Would you mind just bumping is_present above main?
<abentley> rockstar, sure.
<rockstar> abentley, r=rockstar
<abentley> rockstar, thanks.
* rockstar changed the topic of #launchpad-reviews to: on call: rockstar || reviewing: jelmer || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
* rockstar changed the topic of #launchpad-reviews to: on call: rockstar || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<rockstar> abentley, could I get you to do a small javascript branch?
<rockstar> s/do/review
<rockstar> EdwinGrubbs, could I get you to take a look at a branch to fix a bug you filed?
<EdwinGrubbs> rockstar: sure
<rockstar> EdwinGrubbs, https://code.edge.launchpad.net/~rockstar/launchpad/code-js-reorg/+merge/20170
<rockstar> EdwinGrubbs, it's run the windmill gambit and everything is hunky dory.
<rockstar> EdwinGrubbs, wait, it looks like I didn't get all the revisions I needed.  Please hold for a new diff.
<rockstar> EdwinGrubbs, new revision pushed, should update the diff in a few minutes.
* sinzui changed the topic of #launchpad-reviews to: on call: rockstar || reviewing: - || queue [sinzui] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<sinzui> rockstar: I have a short documentation fix: lp:~sinzui/launchpad/ds-get-source-package into lp:launchpad/devel
<rockstar> sinzui, what's the mp url?
<sinzui> rockstar: https://code.launchpad.net/~sinzui/launchpad/ds-get-source-package/+merge/20172
<rockstar> sinzui, easy. r=rockstar
<sinzui> thanks
<rockstar> EdwinGrubbs, howgoesit?
<EdwinGrubbs> rockstar: I'm just starting on it now.
<rockstar> EdwinGrubbs, great, thanks.
<EdwinGrubbs> rockstar: the module for javascript/code/branchlinks.js should be "code.branchlinks". The only thing that needed to be changed was the namespace variable which you did.
<rockstar> EdwinGrubbs, why should it be code.branchlinks?
<EdwinGrubbs> rockstar: there already is a javascript/lp/ directory, so the files in there are in the lp module.
<rockstar> EdwinGrubbs, :/
<rockstar> EdwinGrubbs, what happens when canonical/launchpad/javascript doesn't exist anymore?
<rockstar> At least, canonical/launchpad/javascript/code won't exist much longer.
<EdwinGrubbs> rockstar: well, lib/lp/code/javascript could be the new home for the "code" module. If you want lp to prefix everything, you will have to bring it up at the reviewer meeting.
<rockstar> EdwinGrubbs, okay, I withdraw my proposal then, and I'll bring it up at the next reviewer meeting.
<abentley> rockstar, sorry, was on lunch.  Sounds like you got a better review anyhow.
<EdwinGrubbs> rockstar: ok, sorry for the confusion. I thought the docs were clear.
 * rockstar afks to the shop
<leonardr> rockstar, i'm leaving soon but if you get some free time today or tomorrow, would you take a look at https://code.edge.launchpad.net/~leonardr/lazr.restful/mutators-are-not-named-operations ?
* rockstar changed the topic of #launchpad-reviews to: on call: rockstar || reviewing: leonardr || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
#launchpad-reviews 2010-02-26
* sinzui changed the topic of #launchpad-reviews to: on call: rockstar || reviewing: leonardr || queue [sinzui] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<sinzui> rockstar: if you are still reviewing: https://code.launchpad.net/~sinzui/launchpad/something-and-nothing/+merge/20191
* rockstar changed the topic of #launchpad-reviews to: on call: - || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<rockstar> sinzui, looking
* jelmer changed the topic of #launchpad-reviews to: on call: - || reviewing: - || queue [al-maisan/jelmer] || This channel is logged: http://irclogs.ubuntu.com ||
* adeuring changed the topic of #launchpad-reviews to: on call: adeuring || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<al-maisan> Good morning adeuring, could you please take a look at https://code.edge.launchpad.net/~al-maisan/launchpad/restricted-ui/+merge/20145 ?
<adeuring> hi al-maisansure, I'll look
<al-maisan> the diff is not quite right, this is all the branch does: http://pastebin.ubuntu.com/384297/
<al-maisan> adeuring: ^^
<adeuring> sl-thanks!
* bigjools changed the topic of #launchpad-reviews to: on call: adeuring || reviewing: - || queue [bigjools] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<bigjools> adeuring: simple branch for ya
<adeuring> bigjools: sure; i just started a review for al-maisan, after that one?
<bigjools> np
<al-maisan> do we need a database review for a branch that modifies database/schema/security.cfg ?
<adeuring> al-maisan: r=me; 1 nitpick
<al-maisan> adeuring: thanks :)
<al-maisan> adeuring: I'll take care of the nitpick
<adeuring> bigjools: r=me
<bigjools> adeuring: thanks very much
<bigjools> no nitpicks? :)
<noodles775> Hi adeuring, I've got an 820 line pure test refactoring branch needing review, but if you'd prefer, I can move the last change out and into the next pipe: https://code.edge.launchpad.net/~michael.nelson/launchpad/ppa-privatisation-test-refactor5/+merge/20207
<jelmer> hi adeuring
<jelmer> adeuring: When you have a chance, can you have a look at https://code.edge.launchpad.net/~jelmer/launchpad/bug471148-ui/+merge/20208 ?
<jelmer> Hmm, today seems to be soyuz review day :-)
<bigjools> we're productive dudes
<adeuring> noodles775: na... that's fine
* noodles775 changed the topic of #launchpad-reviews to: on call: adeuring || reviewing: - || queue [bigjools, noodles775, jelmer] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<adeuring> jelmer: sure, i can look.
<noodles775> Thanks adeuring !
<jelmer> adeuring: Thanks
<gmb> adeuring: Okay if I add another branch to the queue?
* adeuring changed the topic of #launchpad-reviews to: on call: adeuring || reviewing: noodles775, || queue [jelmer] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
* gmb changed the topic of #launchpad-reviews to:  on call: adeuring || reviewing: - || queue [jelmer, gmb] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
* gmb changed the topic of #launchpad-reviews to:  on call: adeuring || reviewing: noodles775 || queue [jelmer, gmb] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<adeuring> gmb: sure, but I just strted a somewhat longer review for noodles775
<gmb> adeuring: Okay, I'll see if I can find someone else to reviw my branch.
<salgado> gmb, I'll start my shift shortly and I'll have plenty of time to review yours
<gmb> salgado: allenap's kindly agreed to review it in the meantime; sorry, I forgot to remove it from the queue. Thanks anyway.
* gmb changed the topic of #launchpad-reviews to:  on call: adeuring || reviewing: noodles775 || queue [jelmer] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<salgado> no worries
* adeuring changed the topic of #launchpad-reviews to: on call: adeuring || reviewing: jelmer || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<noodles775> Thanks adeuring .
* bigjools changed the topic of #launchpad-reviews to: on call: adeuring || reviewing: jelmer || queue [bigjools] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<bigjools> adeuring: I have a further tweak to the change you just reviewed, it's on the queue.
<adeuring> bigjools: ok
<bigjools> I have to run out to give a presentation on FLOSS to my son's school now!
<jtv-afk> bigjools: run, julian, run!
<adeuring> jelmer: could you add doc strings to _get_arm_builds_enabled() and _get_arm_builds_disabled()?
<adeuring> jelmer: sorry... i mean _set_arm_builds_enabled() instead of the "disabled"
<jelmer> adeuring: ok
<adeuring> jelmer: there is an "elif" in set_arms_builds_enabled() without a final "else". Could you add such an "else: pass", together with a comment why nothing needs to be done in this case?
<adeuring> jelmer: and (sorry about all these nitpick for one method...): The comment "a link is required but it is present" is a bit enigmatic, at least for me. What about sonething like "Arms builds are already enabled"?
<jelmer> adeuring: Sure, that's more sensible indeed.
<adeuring> jelmer: getPubSource() in test_publishing.py needs a doc string
<adeuring> jelmer: other than these nitpicks, r=me
<jelmer> adeuring: Thanks!
<adeuring> jelmer: welcome
* adeuring changed the topic of #launchpad-reviews to: on call: adeuring || reviewing: bigjools || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
* adeuring changed the topic of #launchpad-reviews to: on call: adeuring || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<noodles775> Hi again adeuring ! I've got the final test refactoring branch ready if you've time: https://code.edge.launchpad.net/~michael.nelson/launchpad/ppa-privatisation-test-refactor6/+merge/20227
<adeuring> noodles775: could you ask salgado? I have a somewhat urgent branch myself...
<noodles775> Sure, salgado?
* salgado changed the topic of #launchpad-reviews to: on call: adeuring,salgado || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<salgado> noodles775, I'm working on a critical issue, but if gary_poster can't take it I will
<gary_poster> noodles775, salgado: looking
* gary_poster changed the topic of #launchpad-reviews to: on call: adeuring,gary_poster || reviewing: ?,noodles775 || queue [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<gary_poster> salgado: changed topic taking you off reviewers
<salgado> thanks gary_poster
<gary_poster> noodles775: why did you remove test_login.py?
<noodles775> gary_poster: I didn't... a local diff between this pipe and the prev shows everything but the test_login.py.
<noodles775> I was just trying to figure out what was going on with the MP.
<gary_poster> noodles775: ok, I'll just clarify in the review that merging that removal is not ok, and leave you to handle the mechanism :-)
<noodles775> gary_poster: I'll merge RF into the initial pipe (there are 7 in total) and push it through and see if it's still there when the diff regenerates on the MP.
<gary_poster> noodles775: ack, ok
<salgado> gary_poster, noodles775, in my openid branch I added a test_new_login.py file and later removed test_login.py.  then on a subsequent branch I renamed test_new_login.py to test_login.py.  that, together with pipes, might have confused  bzr?
<noodles775> salgado: thanks for the info - I thought it may have been related to your branch I reviewed yesterday. If that's the case, pumping a fresh RF through my pipes should resolve the issue. (I hope)
<gary_poster> noodles775: in xx-private-builds.txt, why is it OK to remove diff line 375ff? ("Now visit the same page as an admin we can see that there are four more...")
<gary_poster> thanks salgado
<gary_poster> noodles775: also, in the previous section of the same file, why did the result change (5 of 17 results, and different values)?  If explaining it takes too long, I'll accept "because of changes that have previously passed review"
<gary_poster> but I'd like reassurance that you are confident of the change, at least :-)
<gary_poster> hm, I could understand an increase of 1, since we added the private p3a archive, but I don't understand 14 -> 17
<noodles775> sorry gary_poster, I'll be there in a minute or two.
<gary_poster> np noodles775
<stub> gary_poster: You already have looked at https://code.edge.launchpad.net/~stub/launchpad/memcache/+merge/20226 earlier. Its finished now.
<noodles775> gary_poster: so, regarding your first question, 375ff, the test for authorised viewers seeing the builds in the history was moved to just below with the Frog builder.
<noodles775> It looks like a lot less test-code because the Frog builder only has the one build in its history (it's not polluted with sample data :)).
<stub> (it won't land this cycle though if you need to prioritize)
<gary_poster> noodles775: ah ok cool thanks
<noodles775> gary_poster: and your second question is related, because previously cprov's ppa was switched to private (which is no longer allowed), and had a bunch of publishings.
<gary_poster> stub: cool, I'll take it, unless flacoste, who was just complimenting the branch, tells me that he'll take it.  But I'll shepherd it one way or the other
<noodles775> So the 14 -> 17 is because there are 3 packages in cprov's public archive (when it was private ,they did not show up).
<gary_poster> stub: yeah, I'll take it :-)
<flacoste> stub: it's not a review, but heh, nice work! i like the syntax a lot!
<stub> ta :)
* sinzui changed the topic of #launchpad-reviews to: on call: adeuring,gary_poster || reviewing: ?,noodles775, sinzui || queue [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<noodles775> s/packages/builds of course.
<gary_poster> noodles775: we dupe the private setup in xx-source-package-publishing.txt and xx-binary-package-publishing.txt (">>> from lp.registry.interfaces.distribution import IDistributionSet...").  You considered and rejected a helper function?  Two copies is arguable, maybe.  Three, much less so. :-)
<gary_poster> So if you considered and rejected then I'm OK, but otherwise, please consider :-)
<sinzui> bac: EdwinGrubbs: are either of you available to review https://code.launchpad.net/~sinzui/launchpad/timeout-portlet-package-summary-2/+merge/20228
<noodles775> gary_poster: no I didn't consider it - sounds good.
<gary_poster> ok cool noodles775
<bac> sinzui: i can do it later.  i just made a break through on my testing so i'd prefer not to interrupt that effort ATM
<sinzui> rock
<allenap> adeuring, gary_poster: Are either of you close to be able to review another branch? If not I'll see if I can get a quick review elsewhere.
<gary_poster> allenap: I'd be able to do it in about...30-45 min, I'd guess
<allenap> gary_poster: Okay, I'll go and prod someone else. (I don't have much time left to work today, that's the only reason.) Thank you though :)
<gary_poster> allenap: cool, np :-)
<gary_poster> noodles775: I'm finishing the review, but merge-conditional on not landing the deletion of test_login.py
<adeuring> allenap: I can review it in a few miuntes
<gary_poster> I mean, I'm writing it up
<gary_poster> but it is r=gary with that
<allenap> adeuring: deryck is doing it now, but thank you anyway.
<noodles775> gary_poster: thanks. Actually, if you re-load the MP you'll see that it's no longer there (pumping a fresh RF through seems to have helped)
<gary_poster> noodles775: confirmed, awesome :-)
<noodles775> Hi gary_poster, would you object if I land that earlier branch without creating the pagetest helper? I need to leave 20mins ago and would like to send it off to ec2.
<gary_poster> noodles775: that's fine
<noodles775> gary_poster: ta.
* sinzui changed the topic of #launchpad-reviews to: on call: adeuring,gary_poster || reviewing: ?,noodles775 || queue [sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
* adeuring changed the topic of #launchpad-reviews to: on call: gary_poster || reviewing: noodles775 || queue [sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<sinzui> bac: EdwinGrubbs: can either of you review my branch: https://code.launchpad.net/~sinzui/launchpad/timeout-portlet-package-summary-2/+merge/20228 If we cannot land it, we need to RC and remove code next week
<bac> sinzui: yes
<bac> sinzui: is there a bug to put the 'needs linking' section back in?
<bac> assuming we can solve the query timeouts
<sinzui> no there is not
<sinzui> bac: I assume we need a model change and job to do daily updates
<bac> sinzui: am i right in assuming we want to restore it?  i just don't want to rip it out and then forget about it.
<sinzui> bac: yes we should restore it after the next two blueprints
<sinzui> I can report a bug and add an XXX
<bac> cool
<bac> done
<sinzui> bac: I was able to restore the template. I changed view/needs_linking to return None and left an XXX. We can re-enable the template by uncommenting a line
<bac> ah, sweet
<intellectronica> can anyone take a small branch for review?
<intellectronica> i see that there's a bit of a queue, and it's getting late for me, so if anyone other than gary wants to snatch it now i'll be grateful
<intellectronica> only 115 lines
<intellectronica> rockstar: how about you? you're being suspiciously quiet
<intellectronica> EdwinGrubbs: maybe you fancy a short review?
<EdwinGrubbs> intellectronica: I should able to do a review in about 30 minutes.
<intellectronica> EdwinGrubbs: wow, you're amazing. thanks so much. i'm creating an MP now
<intellectronica> EdwinGrubbs: https://code.edge.launchpad.net/~intellectronica/launchpad/heat-decay-complete/+merge/20250
<gary-lunch> sinzui: do you have a branch you need me to look at?
<sinzui> gary, no, sorry, bac reviewed it for me
* sinzui changed the topic of #launchpad-reviews to: on call: gary_poster || reviewing: noodles775 || queue [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<gary_poster> cool
* gary_poster changed the topic of #launchpad-reviews to: on call: gary_poster || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<bac> gary_poster: you ditched us last night!
<gary_poster> bac: :-P sorry.  talked later with chris rossi at the office and then decided I had had my "drink".  I'll stay later next time...esp. when it is closer to home. :-)
<gary_poster> bac, do you thnk you will do any of the hack nights?
<bac> yes, probably.  especially if i make any progress on my side project, which hasn't gotten off the ground yet
<EdwinGrubbs> intellectronica: review sent
<intellectronica> EdwinGrubbs: great, thanks a million!
<bdmurray> intellectronica: so I've figured out what was wrong with export bug linked branches branch
<intellectronica> bdmurray: oh yeah? what was it?
<bdmurray> intellectronica: I believe we were importing and using the wrong thing.  But now I'm uncertain how to proceed with a new or modified merge proposal
<intellectronica> bdmurray: excellent. i'm just about to sign off, but i'll be back tomorrow, so if you have something ready i can review and land it for you
<bdmurray> intellectronica: okay, cool.  Should I just make a whole new merge proposal against devel and reference the old one?
<intellectronica> bdmurray: sure, that works
* gary_poster changed the topic of #launchpad-reviews to: on call: - || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
#launchpad-reviews 2010-02-27
<EdwinGrubbs> anybody around?
<EdwinGrubbs> I can trade reviews.
#launchpad-reviews 2010-02-28
<al-maisan> Hello there, anybody there to take a look at https://code.edge.launchpad.net/~al-maisan/launchpad/conflict-resolution-20100228/+merge/20301 ?
<al-maisan> BjornT or gmb maybe?
#launchpad-reviews 2011-02-21
<stub> lifeless: distro.all_distro_archive_ids issues a db query. Did you consider inlining that?
<lifeless> what do you mean by inlining it ?
<lifeless> Doing it as part of the query ?
<stub> 'if not len(series_clauses)' seems an odd way of spelling 'if not series_clauses'
<stub> lifeless: Yes. Embedding that logic in the SQL
<lifeless> stub: I was assuming that other things will hit that cachedproperty
<stub> It would depend if that cached_property is being evaluated elsewhere in the pages that use this I guess
<lifeless> so that inlining it would be of little benefit, and perhaps a (tiny) pessimism
<lifeless> but I have no data
<lifeless> happy to revisit when we have the next round of scaling data
<stub> lifeless: WHERE (single_item = 'foo') is valid, so you can simplify the OR generation for combined_clause
<lifeless> ok, cool
<stub> lifeless: The helpers we have are what Storm gives us, but I think it would be worse to mix and match Storm & raw SQL here.
<lifeless> yeah
<lifeless> I thought about that and cried
<stub> This could all be Storm code, but I personally don't believe that it would be more readable or maintainable that way.
<stub> esp. since you would be jumping to undocumented apis like 'group by'
<lifeless> agreed
<stub> lifeless: So you have a GROUP BY calculating the MAX(spph.id) for each spr.id, but you don't use it.
<lifeless> yeah we do
<lifeless> oh hmmm
<lifeless> let me review
<stub> I don't think that query does what you think that query does
<lifeless> perhaps
<lifeless> I think the max is redundant
<lifeless> wgrant: what do you think, I think you touched the query shape right before me ?
<lifeless> stub: so the query is transplanted; I'm pretty sure the multi-series change is good; if the query was already faulty, thats perhaps a separate problem ?
<stub> SELECT DISTINCT ON (spr.id) spr.*
<stub> FROM
<stub>     SourcePackageRelease AS spr,
<stub>     SourcePackagePublishingHistory AS spph,
<stub>     DistroSeries
<stub> WHERE
<stub>     spph.sourcepackagerelease = spr.id
<stub>     AND spph.distroseries = DistroSeries.id
<stub>     AND spph.status IN (1,2,3)
<stub>     AND (
<stub>         (spr.sourcepackagename IN (1,2,3 )
<stub>             AND spph.archive IN (1,2,3)
<stub>             AND DistroSeries.distribution = 1
<stub>         )
<stub>         )
<stub> Bah
<stub> Forgot the order by
<stub> SELECT DISTINCT ON (spph.sourcepackagerelease) spr.*
<stub> FROM
<stub>     SourcePackageRelease AS spr,
<stub>     SourcePackagePublishingHistory AS spph,
<stub>     DistroSeries
<stub> WHERE
<stub>     spph.sourcepackagerelease = spr.id
<stub>     AND spph.distroseries = DistroSeries.id
<stub>     AND spph.status IN (1,2,3)
<stub>     AND (
<stub>         (spr.sourcepackagename IN (1,2,3 )
<stub>             AND spph.archive IN (1,2,3)
<stub>             AND DistroSeries.distribution = 1
<stub>         )
<stub>         )
<stub> ORDER BY spph.sourcepackagerelease DESC, spph.id DESC;
<lifeless> AIUI we can only get one spph back anyway because of the status check
<stub> So it is working by accident?
<stub> Only one published with that status in that distroseries... guess not accident.
<lifeless> stub: I'd be delighted to file a bug to review this query to make it more efficient
<lifeless> you'll note the two implementations were different (because one had been tuned more recently) - it was that one that I extended
<stub> If you file a bug, you need an XXX: in there stating the query is obviously broken
<lifeless> I want to consult the soyuz folk first
<stub> But I'd rather it got fixed now given we have diagnosed it.
<lifeless> I'll land a trivial with an XXX if there is an issue
<lifeless> stub: so you're proposing what you pasted?
<stub> The query is broken, we know that.
<stub> If you are correct about the status, my query is unnecessarily complicated.
<lifeless> stub: line 132 in the diff in the mp has the old 2 generations ago query
<stub> Right. So either the 'latest spph' is unnecessary due to the status field, or someone broke it.
<lifeless> ok
<stub> This becomes fairly neat Storm code too...
<wgrant> lifeless: I don't think I've touched that code recently/ever.
<wgrant> stub: There will be more than one Published occasionally.
<wgrant> In the middle of a publisher run.
<stub> lifeless: So it seems you transplanted the getCurrentSourcePackages query for a distroseries into your new getCurrentSourcePackages for a distribution
<lifeless> stub: I did
<lifeless> stub: and adjusted it
<lifeless> wgrant: so the distroseries code I started with was in fact bong; the max(id) is needed in there as well as the status constraints ?
<stub> But that breakage we identified may not matter for a distroseries, but could matter a great deal for a distro.
<lifeless> stub: distros are already broken
<lifeless> stub: because they return the most recent for the distro, not the most recent for the distro development focus - so backports can show up
<stub> So we are making broken code break faster :)
<lifeless> in a manner of speaking
<lifeless> I'd like to delete distro{,Set}.getCurrentSourceReleases
<lifeless> but thats a different task
<lifeless> its not my intent to break the semantics of the functions with this patch
<lifeless> and I'm glad you noticed
<stub> But we are breaking semantics. DistroSet.getCurrentSourceReleases used to return the 'current' source release across all distroseries, including backports (which you say is a bug). The new implementation returns a random sourcepackagerelease with a particular status across all distroseries
<stub> So you could get hardy's most recent spr when you asked for Ubuntu's most recent spr.
<lifeless> 1,2 is pending and published
<lifeless> right, agreed
<lifeless> so as you say, its broken
<stub> Suckiest thing is this indicates the tests are not sufficient ;)
<lifeless> well
<lifeless> I hadn't run all the tests
<stub> reduce(lambda _, series:milestones.extend(series.milestones), self.series_list, [])
<stub> You missed your calling as a Perl programmer?
<stub> Or maybe lisp?
<stub> lifeless: approved with minor comments - https://code.launchpad.net/~lifeless/launchpad/bug-717394/+merge/50541
<lifeless> stub: huh, I think that goes in the next branch
<lifeless> stub: sorry about the diff in the -2 branch
<lifeless> I forgot to give it a prereq
<stub> lifeless: Can you resub easily?
<lifeless> let me see
<stub> Or did the other review get everything and this one is redundant?
<lifeless> it used to end badly doing htat
<lifeless> https://code.launchpad.net/~lifeless/launchpad/bug-717394-2/+merge/50557
<lifeless> stub: ^
<lifeless> stub: diff is there
<stub> and reviewed
<lifeless> stub: that leaves just https://code.launchpad.net/~lifeless/launchpad/bug-717394-4/+merge/50548
<lifeless> stub: did you have a fixed query for bug 636158 ?
<_mup_> Bug #636158: BugTask:+index times out with many bug tasks/nominations (eg. Bug #1) <lp-bugs> <pg83> <timeout> <Launchpad itself:Triaged> < https://launchpad.net/bugs/636158 >
<stub> lifeless: What query needs fixing in that bug?
<lifeless> stub: thats the getcurrentsourcereleases one
<lifeless> that you were fiddling with
<lifeless> which I broke
<lifeless> I thought you had a working one I could slot in
<stub> The ones I pasted before?
<lifeless> ah
<lifeless> I was'nt sure that that was complete
<stub> SELECT DISTINCT ON (spph.sourcepackagerelease) spr.*
<stub> FROM
<stub>     SourcePackageRelease AS spr,
<stub>     SourcePackagePublishingHistory AS spph,
<stub>     DistroSeries
<stub> WHERE
<stub>     spph.sourcepackagerelease = spr.id
<stub>     AND spph.distroseries = DistroSeries.id
<stub>     AND spph.status IN (1,2,3)
<stub>     AND (
<lifeless> it seemed to have different spph.status ;)
<stub>         (spr.sourcepackagename IN (1,2,3 )
<stub>             AND spph.archive IN (1,2,3)
<stub>             AND DistroSeries.distribution = 1
<stub>         )
<stub>         )
<stub> ORDER BY spph.sourcepackagerelease DESC, spph.id DESC;
<lifeless> cool, I'll slot that in
<lifeless> and similar for DistoSeries I presume ?
<stub> Yes, you need to replace my placeholders :)
<lifeless> thank you
<stub> Yes, although we might be able to use a simpler query if the status is sufficient for getting 'current package'
<stub> Although from what wgrant said earlier, that might very occasionally return two 'current packages'
<lifeless> yes
<lifeless> belts and bracers for now
<stub> wgrant: That looks sane to you? ^^
<wgrant> Looking.
<wgrant> If it does return two packages, I wouldn't worry too much.
<wgrant> The callsites are just informational.
<wgrant> Just pick one.
<stub> lifeless: https://code.launchpad.net/~lifeless/launchpad/bug-717394-4/+merge/50548 reviewed
<lifeless> stub: hi
<stub> yo
<lifeless> stub: I don't see how that query actually helps
<lifeless> spph.sourcepackagerelease isn't going to be distinct if there are two candidates
<lifeless> and the point is to get sprs for different names at the same time
<stub> spph.sourcepackagerelease isn't distinct, which is why the DISTINCT ON is there.
<lifeless> hardy and lucid have different spr's for their most recent spph
<lifeless> we want only one of those two sprs
<lifeless> a window function can do what we need
<stub> So we get all (spph, spr) records matching, ordered by (spph.sourcepackagerelease DESC, spph.id DESC) and disgard all but the first matching one
<lifeless> where do we discard ?
<stub> DISTINCT ON
<stub> Not DISTINCT
<lifeless> I get that
<lifeless> but I think spph.sourcepackagerelease is different than you think it is
<lifeless> distinct on spr.sourcepackagename might do it
<lifeless> will the order by take the first item reliably ?
<stub> oic. we are getting the latest out of all spns....
<stub> you get an error if the order by doesn't match what is needed by the ORDER BY
<lifeless> right, we want multiple sprs, one spr per spn
<lifeless> I think I have it, trying now
<stub> SELECT DISTINCT ON (spr.sourcepackagename) spr.*
<stub> FROM
<stub>     SourcePackageRelease AS spr,
<stub>     SourcePackagePublishingHistory AS spph,
<stub>     DistroSeries
<stub> WHERE
<stub>     spph.sourcepackagerelease = spr.id
<stub>     AND spph.distroseries = DistroSeries.id
<stub>     AND spph.status IN (1,2,3)
<stub>     AND (
<stub>         (spr.sourcepackagename IN (1,2,3 )
<stub>             AND spph.archive IN (1,2,3)
<stub>             AND DistroSeries.distribution = 1
<stub>         )
<lifeless> yes, thats what I have though stormisified
<stub> You got DISTINCT ON in Storm syntax?
<lifeless> theres a hack
<lifeless> grep for DISTINCT ON in lib
<lifeless> stub: so, thats wrong
<lifeless> its (spr.sourcepackagename, distroseries.distribution) in the distinct
<lifeless> *that* gets use unique tuples matching the input parameter
