#launchpad-reviews 2010-07-26
<thumper> https://code.edge.launchpad.net/~thumper/launchpad/no-sample-data-doctest/+merge/30913
<thumper> mwhudson: diff is there
<adeuring> moin henninge,  fancy a 180 lines diff review? https://code.edge.launchpad.net/~adeuring/launchpad/security-guarded-test-object-factory-3/+merge/30917
 * flacoste is away: Gone away for now
* henninge changed the topic of #launchpad-reviews to: On call: henninge  || reviewing:  adeuring || queue: [jtvÂ³] ] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<malaria> Hi,
<malaria> Some feedback about bug #608631 ?
<_mup_> Bug #608631: Visual tag to represent narrow non-breaking spaces <Launchpad Translations:New> <https://launchpad.net/bugs/608631>
<malaria> henninge perhaps?
<henninge> malaria: I cannot comment on the matter, I am sorry. But as the bug is still new, it will be picked up and triaged these days and you should get a comment.
<malaria> ok, thanks ;-)
<henninge> adeuring: Hi!
<adeuring> hi henninge
<henninge> Argh! I was looking at the wrong MP ... :(
<adeuring> henninge: sorry if I screwed the links...
<henninge> adeuring: no, no, I had gone back to the first branch to see what remove_security_proxy_and_shout_at_engineer and such do.
<henninge> Too many tabs open ... ;)
<adeuring> ah, OK
<henninge> adeuring: so you branch is about mechanically adding ProxyFactory and remove_security_proxy_and_shout_at_engineer, right?
<adeuring> henninge: right. BTW, see also the iscussion on the mailing list
<adeuring> lifeless has some objections to the concept ;)
<henninge> adeuring: oh, I was away last week. What's the subject?
<adeuring> henninge: [Launchpad-dev] warning: we will soon have much noise in the test 	results...
<adeuring> henninge: so, I would not mind if you put the review on hold
<henninge> adeuring: let me read that discussion first.
<adeuring> sure. (and sorry for the mess... lifless responded after I wrote the MP)
* henninge changed the topic of #launchpad-reviews to: On call: henninge  || reviewing:  lunch || queue: [adeuring, jtvÂ³] ] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<wgrant> win 3
<wgrant> Argh.
<gary_poster> anyone available for a review of https://code.launchpad.net/~gary/launchpad/apidoc/+merge/30849 ?
* abentley changed the topic of #launchpad-reviews to: On call: henninge, abentley  || reviewing:  lunch || queue: [adeuring, jtvÂ³, gary_poster] ] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<abentley> gary_poster, I'm look at it, but I think I should try the rest of the queue first.
<gary_poster> abentley: fair enough, thank you
* abentley changed the topic of #launchpad-reviews to: On call: henninge, abentley  || reviewing:  lunch || queue: [jtvÂ³, gary_poster] ] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<abentley> jtv, around?
<jtv> abentley: no
* abentley changed the topic of #launchpad-reviews to: On call: henninge, abentley  || reviewing:  lunch, gary_poster || queue: [gary_poster] ] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* abentley changed the topic of #launchpad-reviews to: On call: henninge, abentley  || reviewing:  lunch, gary_poster || queue: [] ] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jtv> abentley: one of my branches got reviewed over the weekend
 * flacoste is back.
* abentley changed the topic of #launchpad-reviews to: On call: henninge, abentley  || reviewing:  lunch, - || queue: [] ] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<mars> abentley, ping, do you have time for a mostly mechanical review of the windmill test suite and lazr-js 1.0 upgrade?  https://code.edge.launchpad.net/~mars/launchpad/update-lazr-js-to-1.0/+merge/30972
<mars> abentley, I also included your xvfb-run testr fix :)
<abentley> mars, lunching, but soonish.
<mars> I'll add it to the queue then
<mars> thanks
* mars changed the topic of #launchpad-reviews to: On call: henninge, abentley  || reviewing:  lunch, - || queue: [mars] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<abentley> henninge, done for the day?
* abentley changed the topic of #launchpad-reviews to: On call: abentley  || reviewing:  mars || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<abentley> mars, the xvfb-run invocation is different from test-on-merge.  Is there a good reason for this?
<abentley> mars, it seems like it would be ideal to isolate the code for running the test suite under xvb to one location, e.g. the test runner.
<abentley> mars, then you could turn it off and on with a switch.
<mars> abentley, that takes a bit of work
<mars> you could try adding bin/test --headless, but most of the time that would be the default
<mars> bin/test --show-browser ?
<mars> nah
<mars> well, maybe
<abentley> mars, especially when you consider testr -- --show-browser
<mars> abentley, I would have to pull the xvfb-run code up and out of test_on_merge.py and into bin/test, then add the new command-line switches
<mars> and probably refactor bin/test along the way :(
<abentley> mars, I'm not saying we must do it right now, but we use xvfb-run a lot, so it seems worth doing.
<mars> yes, definitely.  I did that already with test_on_merge.py, so I guess it can't hurt to move it elsewhere
<mars> also a site-wide xvfb-run seems a bit overkill to do during every test startup
<abentley> mars, what about the invocation difference?  test-on-merge supplies "-s -screen - 1024x768x24" and testr won't.
<mars> For our code, doesn't matter unless you are planning to debug the Xvfb display directly (Which I do when I need to fix buildbot)
<abentley> mars, okay.
<abentley> mars, is there a bug for better xvfb integration?
<mars> abentley, nope
<abentley> mars, okay, I'll do one.
<mars> cool, thank you
<mars> toss it back to Foundations using the 'build-infrastructure' tag
<abentley> mars, can you avoid repeating the VISIBLE_PICKER xpath definition?
<mars> doing that right now in another branch
<abentley> cool.  r=me.
<mars> thanks!
<henninge> abentley: yes, thanks for removing my name from the topic ... ;-)
<abentley> mars, bug #610172
<_mup_> Bug #610172: xvfb-run should be integrated better <build-infrastructure> <Launchpad Foundations:New> <https://launchpad.net/bugs/610172>
<mars> cool
* abentley changed the topic of #launchpad-reviews to:  On call: -  || reviewing:  - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<rockstar> thumper, https://code.edge.launchpad.net/~rockstar/launchpad/junk-recipe-listings/+merge/30990
 * flacoste is away: Gone away for now
<rockstar> thumper, my code review plz?
<thumper> rockstar: ack
<thumper> rockstar: done
#launchpad-reviews 2010-07-27
<spiv> Want codebrowse to generate OOPS reports?  Then please review https://code.edge.launchpad.net/~spiv/launchpad/codebrowse-oops-84838/+merge/31007 :)
 * mwhudson looks 
<mwhudson> spiv: er
<mwhudson> + Strictly speaking this isn't 100% correct WSGI middleware, because it
<mwhudson> 99	+ doesn't respect the return value from start_response (the 'write'
<mwhudson> 100	+ callable), but using that return value is deprecated and nothing in our
<mwhudson> 101	+ codebrowse stack uses that.
<mwhudson> loggerhead in fact uses that callable
<spiv> mwhudson: heh
<spiv> mwhudson: so I just noticed!
<spiv> mwhudson: see latest push
<spiv> mwhudson: for some reason that didn't show up until my last-minute-before-I-push cleanups :(
<spiv> Sorry for the noise.
<mwhudson> heh
<spiv> There's also something funny I'm seeing with the very first request I make to the server after starting it giving a weird HTTP error, but I'm not sure that's related to this branch.
<spiv> I'll confess to thinking this isn't the most beautiful code I've ever written :)
<thumper> mwhudson: I have a couple of small branches if you have a few minutes
<thumper> https://code.edge.launchpad.net/~thumper/launchpad/fix-mirror-failure-story/+merge/31010
<mwhudson> spiv: heh heh
<thumper> https://code.edge.launchpad.net/~thumper/launchpad/stop-factory-mirroring/+merge/31011
<spiv> I feel so wonderfully old-school with all these mutable variables destined to be mutated by nested functions, though!
<mwhudson> you've been wsgi-ed!
<spiv> Possibly there's enough going on there that I can profitably refactor out an actual object.  Initially it was just the one little variable, but well, wsgi...
<thumper> I plan one day soon to take more of a look at loggerhead
<mwhudson> spiv: blurgh
<mwhudson> spiv: we shouldn't return a 200 when we're reporting an error
<spiv> mwhudson: ah ok, I wondered about that, I meant to ask
<spiv> I'm happy to ape whatever lpnet does here, if you happen to know :)
<spiv> I was hoping to avoid spelunking through the layers of zope publication internals more than necessary
 * mwhudson attempts to trigger a timeout
<mwhudson> spiv: this timeout has a http 503
<spiv> I'm happy to change it to 500 Internal Server Error.
<mwhudson> spiv: please
<spiv> I guess if some IE users get a boilerplate "IE noticed that the server broke, but I won't show you any helpful details the server may have sent about that" page then that's their problem ;)
<mwhudson> yeah, i think that's a potential issue
<mwhudson> but i agree with your conclusion
<mwhudson> spiv: i reviewed the branch btw
<spiv> Thanks!
<thumper> mwhudson: another relatively brainless review https://code.edge.launchpad.net/~thumper/launchpad/bzr-format-matching-refactoring/+merge/31014
<thumper> mwhudson: although more thought needed than the other two :)
<mwhudson> thumper: looking
<mwhudson> thumper: "# The default branch format is unrecognized."
<mwhudson> thumper: this caused mental jarring; the default branch format is 2a, surely?
<thumper> mwhudson: ah
<thumper> mwhudson: hmm...
<thumper> the idea is that if we don't recognise it, it is unrecognized
<thumper> a rename perhaps
<thumper> so for the test methods s/default/unrecognized/
<mwhudson> thumper: +1
<thumper> mwhudson: last for the day... https://code.edge.launchpad.net/~thumper/launchpad/merge-directive-handling-no-request-mirror/+merge/31019
 * mwhudson waits for the diff
<mwhudson> thumper: done
<thumper> mwhudson: thanks
 * thumper kicks off 4th ec2 land run
<jelmer> wgrant, hi
<wgrant> jelmer: Evening.
<jelmer> wgrant: Any particular reason for returning None rather than raising an exception in your refactor-nuf-creation branch?
<wgrant> jelmer: Hm, not really.
<wgrant> Not sure which exception is appropriate, though.
<jelmer> Probably a new one. ValueError would work too, but is perhaps a bit too generic.
<wgrant> Right. Is it worth creating a new exception for that?
<jelmer> Personally I think so, it'd be clearer if people come across that error. I can see the argument either way though.
<jelmer> s/it'd/it'll/
<jelmer> bac`: hi
* jtv changed the topic of #launchpad-reviews to: On call: -  || reviewing:  - || queue: [jtvÂ²] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* mars changed the topic of #launchpad-reviews to: On call: mars  || reviewing:  - || queue: [jtvÂ²] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<bigjools> mars: I just send an MP mail, can I add myself to your queue please
* bigjools changed the topic of #launchpad-reviews to: On call: mars  || reviewing:  - || queue: [jtvÂ², bigjools] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<bac> hi jelmer
<jelmer> bac: 'morning!
<jelmer> bac: I was wondering if you'd seen my follow-up to https://code.edge.launchpad.net/~jelmer/launchpad/600153-qafix/+merge/30790
<bac> jelmer: done
<bac> thanks for the reminder
<jelmer> bac: thanks for the review :-)
 * flacoste is back.
* mars changed the topic of #launchpad-reviews to: On call: mars  || reviewing:  jtv || queue: [bigjools] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jtv> hi mars, thanks for reviewing me
<mars> np
<gary_poster> benji: I'll see how far I get.  To start:
<gary_poster> - I'd suggest making a local release of this release of batchnavigator and integrate it with Launchpad.  Then you can see if the change to the endNumber behavior is problematic
<gary_poster> - in lines 55 and 59 of the diff, why do you call len(batch) outside of the assertions?
<benji> re. local release: good idea
<gary_poster> - I don't see the value of the behavior described in lines 68-74 of the diff
<gary_poster> shouldn't prevBatch try again, or at least puke?
<gary_poster> as it is, it seems like an odd edge case designed to bite people unexpectedly :-)
<gary_poster> - I like the new endNumber behavior.  I also like the new comments.
<benji> re. len: that's cruft that shouldn't be there; removing
<gary_poster> cool
<gary_poster> also cruft: line 115 (#print "_get_length called (EXPENSIVE)")
<benji> re. 68-74: me neither, I just added comments that describe what is being testsed; I don't think the behavior is useful, but I don't believe that strongly enough to remove the tests
<gary_poster> - line 120: the Lazy decorator is supposed to mean that you just "return self._get_length(self.list)"
<gary_poster> you shouldn't have to also set the _listlength value
<gary_poster> 68-74: but...that's new test for this branch
<gary_poster> maybe this is something that leonard added for this branch?
<gary_poster> if so, my question goes to him
<benji> the reason we set _listlength is so we can use it if available; avoiding calling listlength if not
<benji> let me look at 68-74 again
<gary_poster> _listlength: ah ok.  Maybe remove the @Lazy then, since I suspect it adds processing cost for the reader for little to no advantage for speed?
<benji> gary_poster: right, Leonard added the test, I added the comment
<gary_poster> ok.  do you want me to ping him, or do you want to pursue on your own so you feel you understand it first?
<gary_poster> (where "pursue on your own" == "you ping Leonard" in my book, but as you wish :-) )
<benji> re. _listlength: +1
<gary_poster> cool
<benji> I'll pursue
<gary_poster> cool thanks
<gary_poster> I don't understand "return len(self._sliced_list) - 1" yet: why -1?  Maybe I'll see soon...
<gary_poster> (line 139)
<gary_poster> - another cruft but: #print "sliced_list called (EXPENSIVE)" (line 206)
<gary_poster> s/but/bit/
* henninge changed the topic of #launchpad-reviews to: On call: mars  || reviewing:  jtv || queue: [bigjools, henninge] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<henninge> mars: Hi, I added myself to the queue and it would be great if you could get to my branch but I will also have to leave in 30 min. So I don't mind it waiting around till tomorrow, either.
<henninge> ;-)
<henninge> oops, confilcts ...
* mars changed the topic of #launchpad-reviews to: On call: mars  || reviewing:  bigjools || queue: [henninge] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* mars changed the topic of #launchpad-reviews to: On call: mars  || reviewing:  lunch || queue: [henninge] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* mars changed the topic of #launchpad-reviews to: On call: mars  || reviewing:  henninge || queue: [lunch] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* mars changed the topic of #launchpad-reviews to: On call: mars  || reviewing:  henninge || queue: [lunch] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* mars changed the topic of #launchpad-reviews to: On call: mars  || reviewing:  - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* mars changed the topic of #launchpad-reviews to: On call: mars  || reviewing:  wgrant || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* mars changed the topic of #launchpad-reviews to: On call: mars  || reviewing: wgrant || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* mars changed the topic of #launchpad-reviews to: On call: mars  || reviewing: danilos || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* mars changed the topic of #launchpad-reviews to: On call: -  || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
#launchpad-reviews 2010-07-28
<thumper> anyone? https://code.edge.launchpad.net/~thumper/launchpad/fix-upgrade-branch-failure/+merge/31114
<thumper> lifeless: are you technically a reviewer?
<lifeless> I've never left; I was one of the very first reviewers.
<lifeless> and I spoke with bac about whether he wanted any action to stop being emeritus, he didn't
<lifeless> other than poppig back in the roster etc - which I volunteered for anyhow
<lifeless> that said, I'm fucked
<lifeless> is it trivial ?
<lifeless> it looks ok except on line 45 you have 2 lines of VWS, in a class only one line between methods please
 * wgrant loves the Vim plugin which points that out.
<thumper> lifeless: no it isn't trivial
<thumper> lifeless: even if we had trivial, I don't think this should be any more
<thumper> lifeless: also, just to rerun failing tests, use testr run --failing ?
<lifeless> I don't mean [trivial], I meant 'small'
<lifeless> I agree that your patch wouldn't have been [trivial]!
<lifeless> just small-enough-for-fucked-brain to review
<lifeless> yes, testr run --failing
<thumper> lifeless: I'm trying to make as many branches as I can to be trivial to review
<thumper> No .testr.conf config file
<thumper> ?
<thumper> lifeless: I did a testr init; zcat ... | testr load
<lifeless> thumper: it looks in . not in ., .., ../.. etc
<thumper> lifeless: I'm in .
<lifeless> hmm
<lifeless> is there a .testr.conf ?
<thumper> no
<thumper> what creates it?
<lifeless> its in the lp tree
<lifeless> has been for a while
<lifeless> its versioned
<thumper> oh
<thumper> so my rm was prolly bad then
<lifeless> yes
<lifeless> .testrepository - safe to delete
<lifeless> .testr.conf - do nae touch
<thumper> bzr revert to the rescue
<thumper> mwhudson: https://code.edge.launchpad.net/~thumper/launchpad/stop-mirroring-hosted/+merge/31116
<thumper> mwhudson: this is the last of the request mirror branches
 * mwhudson looks
<thumper> mwhudson: the one that actually raises
<mwhudson> yay
<mwhudson> thumper: approved
<mwhudson> with a minor note
<thumper> mwhudson: ta
* jelmer changed the topic of #launchpad-reviews to: On call: jelmer || reviewing: - || queue: [] ||  This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jelmer> EdwinGrubbs: I was looking at your +needs-packaging branch.
<jelmer> EdwinGrubbs: The MP mentions a URL on staging that should timeout - is your branch already on staging?
<jelmer> or this for after it has landed?
<EdwinGrubbs> jelmer: no, it's not on staging, but you are unlikely to be able to trigger a timeout on launchpad.dev.
<jelmer> Ah, of course - makes sense
<mars> jelmer, ping, have time for a simple bit of test suite refactoring?
<mars> as a review
<mars> :)
<jelmer> mars: Sure!
<jelmer> refactoring ftw
<mars> ok, It's on +activereviews
* mars changed the topic of #launchpad-reviews to: On call: jelmer || reviewing: - || queue: [mars] ||  This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<mars> thanks jelmer
* jelmer changed the topic of #launchpad-reviews to: On call: jelmer || reviewing: mars || queue: [mars] ||  This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* jelmer changed the topic of #launchpad-reviews to: On call: jelmer || reviewing: mars || queue: [] ||  This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jelmer> mars: Is it ok that the Mocker object is static and doesn't get cleaned up between tests?
<mars> jelmer, yes, I think it is a singleton Facade to mocker itself
<mars> That is what mocker.reset() is for I believe
<mars> had to experiment a bit to find that out
<mars> jelmer, you know, it could probably become a test case member as well
<mars> the docs don't really have a best practice there, short of using the MockerTestCase base class
<jelmer> Ah, ok - that makes sense
<jelmer> I guess the overhead of creating a Mocker when importing that module also isn't too bad
<jelmer> Is this the first place where we're using mocker?
<mars> it is
<mars> ideal case for it too
<mars> WindmillClient uses dynamic methods, so there is no base class you can inherit and override
<mars> you have to manually stub out every single method, or use mocks
<jelmer> mars: r=me, see some minor comments in the MP
<mars> jelmer, great, thanks for the review
<mars> I'll have another review, a one-liner, coming along in a few minutes
<mars> jelmer, time for one more?  2 lines edited:  https://code.edge.launchpad.net/~mars/launchpad/activate-xvfb-error-log/+merge/31154
<mars> ugh, diff updates are slow today
 * jelmer kicks xchat for not highlighting
<jelmer> mars: r=me
<mars> jelmer, thanks!
<bac> EdwinGrubbs: are you reviewing?
<EdwinGrubbs> bac: yes
* EdwinGrubbs changed the topic of #launchpad-reviews to: On call: jelmer, Edwin || reviewing: mars || queue: [] ||  This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<bac> EdwinGrubbs: would you mind looking at my private membership destroyer branch?  sadly it is huge...almost 1400 lines
<bac> https://code.edge.launchpad.net/~bac/launchpad/bug-602773/+merge/31162
<EdwinGrubbs> bac: sure
<EdwinGrubbs> bac: it doesn't look like validate_person does anything besides check IPerson.providedBY(). Is there something important it should be doing, or could it just be removed?
<EdwinGrubbs> bac: did you see my comment above?
* jelmer changed the topic of #launchpad-reviews to: On call: Edwin || reviewing: mars || queue: [] ||  This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<abentley> EdwinGrubbs, could you please review https://code.launchpad.net/~abentley/launchpad/reject-private-recipe/+merge/31171 ?
<EdwinGrubbs> abentley: I can do that after lunch.
<abentley> EdwinGrubbs, thanks
* EdwinGrubbs changed the topic of #launchpad-reviews to: On call: Edwin || reviewing: - || queue: [abentley] ||  This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* abentley changed the topic of #launchpad-reviews to: On call: Edwin || reviewing: mars || queue: [abentley] ||  This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* abentley changed the topic of #launchpad-reviews to: On call: Edwin || reviewing: mars || queue: [] ||  This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* mars changed the topic of #launchpad-reviews to: On call: Edwin || reviewing: - || queue: [] ||  This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<bac> hi Edwin-lunch, sorry i didn't see your comment before i went to lunch.
<abentley> Edwin-lunch, lifeless reviewed the one I asked you to, so please review https://code.edge.launchpad.net/~abentley/launchpad/new-score/+merge/31178 instead.
* abentley changed the topic of #launchpad-reviews to:  On call: Edwin || reviewing: - || queue: [abentley] ||  This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<bac> Edwin-lunch: back?
<EdwinGrubbs> bac: I'm here
<bac> hi EdwinGrubbs...otp for a sec
<bac> EdwinGrubbs: yeah, i was unsure about keeping that person validator.  you think it is useless?
<bac> i'm unclear how a non-person could get that far
<EdwinGrubbs> bac: I doubt that a nonperson could get that far.
<bac> yeah, i think that test was just in there to protect the remaining checks in the old validator.  probably overly defensive
<bac> thanks for the review.  i'll remove that validator and land via ec2, as always
<EdwinGrubbs> bac: ok, I'll mark it approved then.
<bac> thanks, EdwinGrubbs
<james_w> Hi EdwinGrubbs, would you have time for https://code.edge.launchpad.net/~james-w/launchpad/copy-archive-job/+merge/28439 ?
<EdwinGrubbs> james_w: I can work on that a little bit later today.
<james_w> great, thanks
* EdwinGrubbs changed the topic of #launchpad-reviews to:  On call: Edwin || reviewing: abentley || queue: [james_w] ||  This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<EdwinGrubbs> abentley: what is the queue_record.lastscore exactly?
<abentley> EdwinGrubbs, it is the score that has been assigned to the build to determine its priority.  "last" is hysterical raisins.
<bac> EdwinGrubbs: i've got a 37 line branch.  may i add it to your queue?
<EdwinGrubbs> bac: sure
* bac changed the topic of #launchpad-reviews to:  On call: Edwin || reviewing: abentley || queue: [james_w, bac] ||  This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<bac> thanks.  https://code.edge.launchpad.net/~bac/launchpad/bug-607733/+merge/31195
<EdwinGrubbs> abentley: r=me
<abentley> EdwinGrubbs, thanks.
<bac> EdwinGrubbs: you said earlier you would update https://code.edge.launchpad.net/~bac/launchpad/bug-602773/+merge/31162 -- could you do that now?
<EdwinGrubbs> bac: oops, done.
* EdwinGrubbs changed the topic of #launchpad-reviews to:  On call: Edwin || reviewing: james_w || queue: [bac] ||  This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<EdwinGrubbs> james_w: review sent
* EdwinGrubbs changed the topic of #launchpad-reviews to:  On call: - || reviewing: - || queue: [] ||  This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<james_w> thanks
#launchpad-reviews 2010-07-29
<bdmurray> Is there anybody reviewing now?
<jelmer> bdmurray: I don't think there is an on call reviewer at the moment, though I can do a review if you have something non-huge.
<bdmurray> jelmer: I think it counts as non-huge - https://code.edge.launchpad.net/~brian-murray/launchpad/bug-605340/+merge/31221
<jelmer> bdmurray: That's fine, I'll review.
<bdmurray> jelmer: great, thanks!
<jelmer> bdmurray: Is there any particular reason you're setting the event_creator attribute? It doesn't appear to be used somewhere.
<bdmurray> jelmer: its used to add the header for X-Launchpad-Bug-Modifier in bugnotificationbuilder
<jelmer> bdmurray: That's done from the argument to the constructor, not using the attribute.
<bdmurray> jelmer: are you referring to setting it to None?
<jelmer> bdmurray: No, to line 46 of your patch. You set self.event_creator to event_creator (the argument) there but don't use the attribute afterwards as far as I can tell.
<bdmurray> jelmer: got it now - thanks. I was on a call.
<mwhudson> gary_poster: hey, want to review some of my vostok-* branches? :-)
<mwhudson> you've seen most of it before i think
<gary_poster> mwhudson: :-) I'm happy to do it tomorrow if you like.  booked today
<mwhudson> gary_poster: ok, no real hurry
<mwhudson> gary_poster: happy to chase someone else if you can think of someone to review this sort of thing
<mwhudson> i guess thumper knows the code after battling to write test_traverse...
<gary_poster> ok cool, mwhudson.  send me a note with the branches and I'll look at em.  Well, benji would be a good choice once he is a reviewer, but no help yet.  yeah, thumper might be good, or sinzui.
<mwhudson> gary_poster: ok
<rockstar> thumper, https://code.edge.launchpad.net/~rockstar/launchpad/recipe-no-signer/+merge/31339
 * thumper looks
<rockstar> thumper, no diff yet.  :(
<rockstar> I can has RabbitMQ?
<thumper> yes
<thumper> yes you can
<thumper> one day
<rockstar> Oh, if only that one day could have been yesterday.
<rockstar> Diff!
#launchpad-reviews 2010-07-30
<thumper> rockstar: approved with tweaks
<mwhudson> thumper: morning
<thumper> hi mwhudson
<mwhudson> thumper: did you see my discussion with gary earlier?
<thumper> not really
<thumper> I saw my nick flash by
<thumper> but didn't read
<thumper> whazzup?
<mwhudson> thumper: i have a series of branches up for review, could you look at them?
<thumper> how long are they?
<mwhudson> thumper: https://code.edge.launchpad.net/~mwhudson/launchpad/+activereviews
<thumper> heh
<thumper> I just typed that in
<mwhudson> 200-300 lines each
<thumper> I see you have two reviews to do :)
<thumper> vostok?
<thumper> soyuz precursor?
<mwhudson> thumper: yeah
<thumper> mwhudson: the vostok apache config has /var/tmp/vostok-archive
<thumper> mwhudson: but I don't see that path in the makefile
<thumper> should it be there?
<mwhudson> thumper: i guess so
 * mwhudson didn't write that config
<mwhudson> thumper: what is __used_for__ for?
<thumper> I don't know...
<thumper> just seen it lots
<thumper> perhaps it is no longer used
<thumper> in which case, add a docstring instead of pass :)
<mwhudson> it's set a bunch of times in the codebase but never accesses
<mwhudson> accessed rather
<thumper> don't bother setting then...
<thumper> I'm not sure if zope itself cares
<thumper> if it did, we probably override that behaviour anyway
<mwhudson> i grepped and googled and i think it's no longer used
<thumper> ok
<mwhudson> https://mail.zope.org/pipermail/zope3-users/2005-May/000563.html
<thumper> just a docstring then...
<mwhudson>     """The view for the Vostok root object."""
<mwhudson> ?
<thumper> yeah, I guess
<thumper> one step better than pass
<mwhudson> thumper: also i can't find get_initialized_view
<mwhudson> i know there's something like that, but what's it called?
<thumper> from lp.testing.views import create_initialized_view
<thumper> sorry
<mwhudson> thanks
<thumper> mwhudson: BTW, did a count earlier, almost a third of the total tests start with lp.code :)
<thumper> that skips all doctests as they don't have a module or path prefix
<mwhudson> thumper: heh heh
<thumper> mwhudson: I'm afk for a while, I'll check on your responses when I get back
<mwhudson> thumper: thanks!
<mwhudson> thumper: which Makefile target should create /var/tmp/vostok-archive ?
<mwhudson> it's not like there's any pattern to follow about this sort of thing :(
<thumper> hmm...
<thumper> do we not create other dirs?
<mwhudson> thumper: clean_codehosting creates a few
<mwhudson> this doesn't seem to fit there :-)
<thumper> actually I think most of clean_codehosting should go into build:
<thumper> and the rest into clean
<mwhudson> can i not do this now? :-)
<mwhudson> i'll add a mkdir to build?
<mwhudson> or compile i guess
<thumper> yeah, just add it to one of those
<mwhudson> thumper: did you have any more comments on https://code.edge.launchpad.net/~mwhudson/launchpad/vostok-add-root/+merge/31239 ?
<mwhudson> thumper: i'm working on removing the need to do the launchbag thing
<thumper> just one comment on the XXX comment
<thumper> other than that, it is good
<mwhudson> thumper: thanks
<thumper> mwhudson: done your new review
<mwhudson> thumper: thanks
<mwhudson> thumper: i made some changes to -main-template and -traverse-distro can you look again?
<thumper> ok
<thumper> mwhudson: for -traverse-distro, can you add to the redirect test to test that the canonical url contains the vostok vhost?
<thumper> so we know
<mwhudson> thumper: ah yes, good idea
<thumper> other than that it looks good
<mwhudson> thumper: i'm surprised that test passes
<mwhudson> i guess the testrequest must still be current in some sense
<thumper> what do you mean?
<mwhudson> thumper: the test calls canonical_url on the distro, and that must be returning the url on the vostok vhost
<mwhudson> or
<mwhudson> hm
<mwhudson> bah
 * thumper waits holding breath
<mwhudson> the redirection is good in 'make run'
<mwhudson> but not in the tst
<mwhudson> test
<mwhudson> oh
<mwhudson> the test isn't using VostokTestRequest
<mwhudson> nope, not that
<mwhudson> /me boggles
<mwhudson> even using test_traverse the redirect target is wrong
<mwhudson> but in the webapp it's correct
<mwhudson> aaaaaargh
<mwhudson> hm
<thumper> pizza and movie night is starting
<thumper> I'll check in later
<mwhudson> good call
* adeuring changed the topic of #launchpad-reviews to: On call: adeuring || reviewing: - || queue: [] ||  This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* bac changed the topic of #launchpad-reviews to: On call: adeuring,bac || reviewing: - || queue: [] ||  This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<bac> hi adeuring.  staying busy?
<adeuring> hi bac, up to now, I had no review request
<bac> i see a few on +activereviews.  i'll grab some of those
<jelmer> hi adeurin, bac
<jelmer> sorry, hi adeuring
<jelmer> Any chance I can add a branch to the queue?
<adeuring> jelmer: sure -- hadn't yet reviewed today...
<adeuring> jelmer: where is your branch?
<jelmer> adeuring: https://code.edge.launchpad.net/~jelmer/launchpad/521110-use-python-debian-debversion/+merge/31268
<jelmer> I think you reviewed a version of that branch a while ago (4 months?)
<adeuring> jelmer: yes, I remember -- I was a bit paranoid at that time ;)
<jelmer> https://code.edge.launchpad.net/~jelmer/launchpad/noversion/+merge/21059 was the MP from back then
* adeuring changed the topic of #launchpad-reviews to: On call: adeuring,bac || reviewing: jelmer, - || queue: [] ||  This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jelmer> There was one particular test that failed with older versions of python-apt. I've now landed an updated version of python-debian that works around that bug in the PPA.
<adeuring> ok
<bac> jelmer: your branch has conflicts: https://code.edge.launchpad.net/~jelmer/launchpad/buildd-trivial/+merge/30808
<jelmer> bac: I'll fix - thanks
<jelmer> I guess the smart debian/changelog merger isn't running on lp yet :-)
 * jelmer is waiting for somebody to come up with a smart 3-way python merger
<adeuring> jelmer: the new symlinks lib/deb822.py and lib/debian point to nowhere on my machine.
<jelmer> adeuring: Ah, sorry. I need to remove those as we ended up putting python-debian in the PPA rather than in sourcedeps.conf.
<adeuring> jelmer: ok
<EdwinGrubbs> james_w: ping
<james_w> hi EdwinGrubbs
<jelmer> bac: The changelog conflict should be fixed now.
<EdwinGrubbs> james_w: I'm not sure what was intended by the merge, but in my mind, the target archive could have a package that the source archive does not have, and then after the merge, the target archive is a union of both archives. The test just seems to update the target archive so that it is a mirror of the source archive. If that is all that it will be used for, there is no reason for more testing. I am just curious about the
<EdwinGrubbs>  purpose of merge.
<bac> jelmer: you rock and i take back all of those nasty things i thought about you yesterday
* bac changed the topic of #launchpad-reviews to: On call: adeuring,bac || reviewing: jelmer,jml - || queue: [] ||  This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<adeuring> jelmer: in Version.__init__(), is it OK if self.epoch or self.debian_version are None?
<james_w> EdwinGrubbs: that is what a merge is, correct, but that behaviour is tested elsewhere. This was just intended to be a test that a merge happened. I'm happy to make the test more complex, but I'm not sure there is any value in showing that aspect of a merge at this level.
<EdwinGrubbs> james_w: ok, if that's tested elsewhere, don't worry about it. land away.
<james_w> EdwinGrubbs: thanks
<jelmer> bac: Yep, though it should be None rather than ""
<jelmer> sorry, s/bac/adeuring/
<jelmer> bac: :-)
<adeuring> jelmer: ok, thanks
<adeuring> jelmer: r=me, but a few nitpicks
* adeuring changed the topic of #launchpad-reviews to: On call: adeuring,bac || reviewing: -, jml  || queue: [] ||  This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jelmer> adeuring: Thanks!
<bac> jelmer: i am going to pass on reviewing https://code.edge.launchpad.net/~jelmer/launchpad/buildd-trivial/+merge/30808
<jelmer> bac: ok
<bac> jelmer: it all looks reasonable, but i don't know enough about packagin
<bac> jelmer: perhaps bigjools can look
<bigjools> lamont should review it
<bigjools> he owns the code
<jelmer> bigjools: Ah, ok - I'll wait for him then.
<jelmer> bac, bigjools: Thanks
<bigjools> jelmer: hilariously, I'm sure wgrant recently removed some of the files you're re-adding
<jelmer> bigjools: Unfortunately there's a gazillion alternatives for doing some things in Debian packages :-/
<bigjools> yay
<bac> jelmer: have you pinged lamont to review that branch?
<jelmer> bac: yeah, he's subscribed but on holiday at the moment
<bac> ok
<jelmer> bac: There's no hurry though, these were just some tech-debt fixes.
<bac> jelmer: right
* adeuring changed the topic of #launchpad-reviews to:  On call: bac || reviewing: jml  || queue: [] ||  This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jelmer> anybody around to review a one-liner?
<jelmer> bac: could I add it to your queue?
<bac> yep
<jelmer> it's at https://code.edge.launchpad.net/~jelmer/launchpad/jelmer-ec2-image-update/+merge/31388
* jelmer changed the topic of #launchpad-reviews to:  On call: bac || reviewing: jml  || queue: [jelmer] ||  This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jelmer> bac: Thanks for the review, have a good weekend!
* bac changed the topic of #launchpad-reviews to:  On call: bac || reviewing: -  || queue: [jelmer] ||  This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<sinzui> hi bac: I am preparing a short branch to make the testrunner stop shouting at me for things I did not right. Will you have time to review it?
<bac> yes
<sinzui> 'right' is mispelled, yet I know I was 'right' in how I wrote the tests.
* bac changed the topic of #launchpad-reviews to:  On call: bac || reviewing: -  || queue: [sinzui] ||  This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<abentley> rockstar, I can has review? https://code.edge.launchpad.net/~abentley/launchpad/detailed-build-emails/+merge/31413
<rockstar> abentley, can you verify that there are no conflicts in your branch?
<rockstar> (Launchpad claims there is)
<abentley> rockstar, there weren't, but now there are.
<abentley> rockstar, pushing a fix.
<abentley> rockstar, this is what I get for fixing lint.
<bac> sinzui: is your branch ready?
<sinzui> bac: in 10 minutes. I am waiting for a test to complete
<bac> ok
<bac> ping me when it is available?
<abentley> rockstar, updated.
<rockstar> abentley, huh.
<rockstar> abentley, I can hear you, but you can't hear me...
<rockstar> abentley, one sec.
<sinzui> bac: I am stalled. I fixed a found a typo on the naked object. this broke the test. I am perplexed because I cannot see how the test passed before I fixed it
<bac> NGDGU
<sinzui> bac: I found the mistake. the assertion was wrong. I will take a moment to explain we were expecting a "demotion" in the cover letter
<sinzui> bac: My MP will be visible in a few minutes
<bac> ok
<sinzui> bac https://code.launchpad.net/~sinzui/launchpad/stop-shouting-0/+merge/31423 when you have time
<bac> looking now
<wgrant> jelmer: I actually just removed lots of non-packaging cruft, and was leaving the package cleanups for a subsequent branch.
<jelmer> wgrant: Ah, great.. so our work probably doesn't overlap then
<wgrant> No.
<jelmer> I just cleaned up some lintian warnings and added a 'prepare' target to debian/rules so I could use a custom builder
* bac changed the topic of #launchpad-reviews to:  On call: - || reviewing: -  || queue: [-] ||  This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
#launchpad-reviews 2010-07-31
<EdwinGrubbs> abentley, bac, jml, rockstar, mars: are any of you around to review a one-line buildbot testfix? https://lpbuildbot.canonical.com/builders/lp/builds/1113/steps/shell_7/logs/summary
<rockstar> EdwinGrubbs, fire away
<bac> EdwinGrubbs: lost by a hair
<bac> better reviewer coverage than most weekdays...
<abentley> rockstar, enjoy :-P
<rockstar> Man, look at all yous guys that ain't out enjoyin' your weekends!  :)
<EdwinGrubbs> rockstar: oops, I didn't realize I pasted the wrong url. https://code.edge.launchpad.net/~edwin-grubbs/launchpad/need_from_future_import_with_statement/+merge/31458
<rockstar> EdwinGrubbs, r=me, but please file a bug and send out an email about this.  abentley broke the build, but it's not really his fault...
<rockstar> I can't believe we're running buildbot in a different version of python than we're running on production.
<EdwinGrubbs> rockstar: ok, thanks
<abentley> rockstar, oh geez, really?  I thought we were all switched over.
<rockstar> abentley, not according to the test failure.  EdwinGrubbs had to do "from future import with_statement"
<abentley> rockstar, I couldn't understand what was going on in the test failure.
<mars> rockstar, everything in the build system *except* developer boxes is still on Python 2.5
<rockstar> mars, expand "developer boxes" because technically, my machine is a developer box.
<mars> And it is really really easy to use awesome 2.6 features and forget that "you can't land that"
<rockstar> Does that mean all our boxes in the datacenter?
<mars> rockstar, nope, just your local machine I believe
<mars> In the data center we have one buildbot slave running 2.6/lucid
<mars> the others are all 2.5/hardy
<mars> devpad is 2.5/hardy
<rockstar> mars, but production is running 2.6, right?
<mars> rockstar, hmm, I don't know, but I would think not.  The next "big thing" is to get staging ported to Lucid/2.6.  IIRC then the app servers were next in line.
<rockstar> mars, okay, so the buildbots prevent us from deploying code that can't be run.  That makes a bit more sense.
<rockstar> Still kinda hairy, but makes sense.
<mars> hehe
<mars> yeah, I thought about putting a Py2.5 pyflakes check in the linter, but the changeover is happening
<mars> a pyflakes-2.5 check would have caught this, but I think it is only the second time this has happened
<mars> and people aren't using things like namedtuple yet
<mars> the 2.5 feature set appears to be very heavily ingrained in our minds
#launchpad-reviews 2010-08-01
<jelmer> wgrant: r=me. Thanks for the style fixes, too.
<jelmer> wgrant: Shall I ec2 land as well?
<wgrant> jelmer: Well, the vim plugin makes them conveniently hard to ignore.
<wgrant> Thanks.
<wgrant> Please ec2.
<jelmer> Of course.
