#launchpad-reviews 2009-11-23
<mwhudson> thumper: pls review 
<mwhudson> https://code.edge.launchpad.net/~mwhudson/launchpad/ec2-test-shutdown-if-setup-fails/+merge/15138
 * thumper looks
<mwhudson> no diff yet :/
 * thumper waits
<mwhudson> thumper: it's there now
<thumper> mwhudson: done
<mwhudson> thumper: thanks
<thumper> mwhudson: still around?
<mwhudson> thumper: slightly
<mwhudson> thumper: i can review that branch we were talking about if you want
<thumper> mwhudson: want to look at this trivial branch?
<thumper> cool
<thumper> mwhudson: should be your way soon
<thumper> mwhudson: ta
<mwhudson> thumper: np
* 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
<leonardr> intellectronica, can you please review https://code.edge.launchpad.net/~leonardr/lazr.restful/double-your-enjoyment-2/+merge/15045 ?
<intellectronica> leonardr: sure. do you need my credit card details?
<intellectronica> leonardr: r=me
<leonardr> intellectronica, tx
* sinzui changed the topic of #launchpad-reviews to: on call: intellectronica || reviewing: - || queue [sinzui] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<intellectronica> sinzui: branch?
<sinzui> intellectronica: https://code.launchpad.net/~sinzui/launchpad/vocab-storm-bug-413287/+merge/15125
* intellectronica changed the topic of #launchpad-reviews to: on call: intellectronica || reviewing: sinzui || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<intellectronica> sinzui: all good. r=me
<sinzui> thanks!
* abentley changed the topic of #launchpad-reviews to: on call: intellectronica,abentley || reviewing: sinzui,- || 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,abentley || reviewing: -,- || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<bac> intellectronica, abentley: i have a small branch for your reviewing pleasure
<abentley> bac: Lay it on me.
<bac> https://code.edge.launchpad.net/~bac/launchpad/bug-266890/+merge/15153
* abentley changed the topic of #launchpad-reviews to: on call: intellectronica,abentley || reviewing: -,bac || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<rockstar> intellectronica, could you take a second look at my branch you were reviewing last week?
<intellectronica> rockstar: sure thing. have you incorporated the comments from my initial review?
<abentley> bac: This looks like a good thing to do.  Do you think a mixin is the best way to express the commonality between ProductAdminView and ProductReviewLicenseView?
<bac> abentley: i did.  do you have another suggestion?
<bac> abentley: they really are two completely different beasts that share that one field
<abentley> bac: Okay, then.  I thought that ProductReviewLicenseView was something like a subset of ProductAdminView.
<abentley> bac: r=me.
<bac> abentley: thanks!
<rockstar> intellectronica, I have.
<intellectronica> rockstar: great. i've got no other comments. r=me
* abentley changed the topic of #launchpad-reviews to: on call: intellectronica,abentley || reviewing: -,- || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<rockstar> intellectronica, thanks!
<intellectronica> abentley: if it's ok with you, i'm going to go off duty here. call soon and after that i'd like to get some time for my own work.
<intellectronica> though as always, feel free to summon me if all of a sudden it gets very busy
* intellectronica changed the topic of #launchpad-reviews to: on call: abentley || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<abentley> intellectronica: That sounds fine.
* sinzui changed the topic of #launchpad-reviews to: on call: abentley || reviewing: - || queue [sinzui] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<sinzui> abentley: I have a branch that removes the old main-template from our tree: https://code.launchpad.net/~sinzui/launchpad/remove-main-template/+merge/15165
<abentley> sinzui: I'm going to get lunch and then review it.  Cool?
<sinzui> abentley: np, the branch is bonus work
<leonardr> abentley, can you add me to the queue? https://code.edge.launchpad.net/~leonardr/lazr.restful/version-specific-request-interface/+merge/15172
<leonardr> it's kind of a complicated branch, so feel free to ask lots of questions
<abentley> leonardr: okay.
* abentley changed the topic of #launchpad-reviews to: on call: abentley || reviewing: sinzui || queue [leonardr] || 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: abentley || reviewing: leonardr || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<abentley> leonardr: since you're not wanting to land this branch, why do you want it reviewed?
<leonardr> abentley: because i don't want to have someone review a 5k loc branch later on
<abentley> leonardr: We have other tools that help with that, like looms and pipelines.
<leonardr> all right, but i'd still rather have the code reviewed as it's finished, so that if there's a catastrophic error in this branch i find out today
<leonardr> i can always do it later, it's not a big deal
<abentley> leonardr: I think it's better to do it later.  The review process isn't really meant to find bugs, or even design issues.  You're much more likely to find them yourself, as you work on the next stage.
<leonardr> abentley, ok
<abentley> leonardr: I'd be happy to show you how to use pipelines to manage this set of work.
<leonardr> abentley, can i ping you tomorrow?
<abentley> leonardr: sure.
<leonardr> ok
* abentley 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 2009-11-24
<jtv> henninge, care to review my fix?  https://code.edge.launchpad.net/~jtv/launchpad/bug-487445/+merge/15185
<henninge> jtv: let's see what you are fixing ...
<henninge> jtv: wow, you are quick!
<jtv> henninge: I wrote the fix this morning
<jtv> when I saw that stub was ahead of me I marked my bug invalid, but kept the branch around
<henninge> jtv: still waiting for the diff to appear.
<jtv> http://paste.ubuntu.com/326709/
<henninge> jtv: Actually, I was not sure if all the dependcies are needed but lazr.choiceedit is needed for sure.
<jtv> henninge: it can sometimes be worth whittling that down... not a job for this branch though I think
<jtv> we want a quick fix here
<henninge> jtv: true, r=me
<jtv> thanks
<henninge> jtv: I was starting on a branch to fix that when I hit the APR_CONFIG problem.
<jtv> stuart hit that one too
<jtv> oops, I'm running out of time... have a bunch of things to do outside
<jtv> started work far too early; various little brush fires
<henninge> jtv: go ahead, see you later, then
<jtv> my branch is in pqm... please watch it for me!
<henninge> jtv: thanks, I will
* 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
* 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
<bac> good morning gmb
<gmb> Hi bac. 
<gmb> Another quiet day today, it seems.
<gmb> Well, so far anyway.
<bac> quiet is good
<gmb> Indeed.
<gmb> So good, I think I'll grab some lunch.,
<jtv> gmb: thanks for the review
<barry> gmb, bac if you're looking for something to review, would you like to take my lazr-js branch?  https://code.edge.launchpad.net/~barry/lazr-js/482298-picker-batching/+merge/15176
<bac> barry i'd love to!
<barry> bac: awesome, thanks
<bac> barry: is there an URL that demonstrates it?
<barry> bac: the best thing to do is load the examples/picker/index.html file
<bac> barry: ok
<barry> bac: by choosing "numbers" you get the simplified api; "letters" gets you the more complex api
<bac> rt
<bac> barry: i've loaded that page.  the 'show widget' button is disabled.  have i done something wrong?
<barry> bac: hmm...
<barry> bac: did you run 'make'?
<bac> nope
<bac> doing so now
<barry> bac: i just tried it.  it doesn't seem to help
<bac> oy
<barry> bac: something's changed/broken in trunk i think.  let me ping the rhinos
<bac> ok
<bac> barry: after the make the picker example now loads
<barry> good!  unfortunately it still doesn't for me :(
<bac> reloaded the page?
<barry> yep, js errors
<barry> i'm trying the trunk now
<barry> trunk works.  i'm chatting w/ noodles775 in rhinos to try to fix my branch
<bac> barry: what's the deal with the roman numerals in index.html?  is it used?
<barry> no, it's just sample code
<bac> ah, ok
<barry> bac: make clean & make fixed my local problem
<bac> ok
<bac> barry: it looks good to me.
<barry> bac: thanks
 * gmb EoDs
* gmb changed the topic of #launchpad-reviews to: on-call bac || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<abentley> bac: Could you please review https://code.launchpad.net/~abentley/launchpad/errorlog-context/+merge/15217 ?
<bac> abentley: certainly
<bac> abentley: r=bac
* bac 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
<abentley> bac: Thanks!
 * bac -> EOD
<bigjools> wgrant: after you finish checking the source format in your change, why is there an assertion error instead of an upload error if the format is not recognised?
<wgrant> bigjools: Let me check.
<wgrant> bigjools: We've already checked that the distroseries supports the format, so if the case isn't handled there it is a bug.
<wgrant> We will not get into that method unless Soyuz knows about the format.
<bigjools> okay, then it makes more sense to me to be checking enum values instead of strings then
<bigjools> I'll add that to the review
<wgrant> bigjools: True.
<wgrant> That code pre-dates the enum.
 * wgrant fixes.
<bigjools> right
<bigjools> wgrant: what are these new orig tarball "comp" files for?
<bigjools> component, I mean
<wgrant> bigjools: If I have foo_1.2-3.orig-bar.tar.gz, it will be extracted into bar/ in the new source tree.
<bigjools> ah interesting
<nhandler> Could someone help me with the test section of the merge proposal for https://code.launchpad.net/~nhandler/launchpad/bugfix151113 ? I'm not really sure what test would be appropriate for this (I think I've spent more time looking into what test to use than it took to patch the actual bug)
<bigjools> nhandler: If I have time I can take a look in a while when I've finished reviewing wgrant's branch
<wgrant> nhandler: lib/lp/blueprints/doc/specification-notifications.txt
<nhandler> Thanks wgrant 
<nhandler> I might be mis-interpreting the test result, but it looks like it is failing because I fixed the bug. It is expecting a notification with the incorrect white space. In this case, should I just patch the test?
<wgrant> nhandler: Exactly.
<nhandler> Alright, I just proposed the branch for merging
<bigjools> nhandler: I approved it but I can't land it right now, can you find someone else who will do it for you please
<nhandler> bigjools: Yeah, I'll try. Thanks for approving it
<bigjools> np - perhaps... /me looks around.... barry or bac can do it?
<wgrant> bigjools: I considered using lambdas, but I think I really need statements.
<bigjools> wgrant: the other option is to slap it all in a new class
<bigjools> but it's up to you
<bigjools> I just don't like a gazillion chained elifs :)
<wgrant> Right.
<bigjools> and on that note, I need to EOD
<wgrant> Thanks.
<bigjools> as the sun sets over Austin
<bigjools> no prob - I'll check it first thing tomorrow.  I'm not sure if we ought to land it just yet in case some 3.0 packages hit the builders before they're ready
<bigjools> although I guess the lack of a new dpkg on cocoplum would prevent that
<wgrant> bigjools: We just won't allow the new formats in any serie yet.
<bigjools> ah of course
<bigjools> ok, cool, then I'll get it landed in the morning if it looks good
<wgrant> Great.
<bigjools> thanks a bunch for making the change
<wgrant> np
<bac> bigjools: wha? you're in austin?
<bigjools> yes
<bigjools> bac: I went to the last UT home game on Saturday :)
<nhandler> bac: Would you be up for landing https://code.edge.launchpad.net/~nhandler/launchpad/bugfix151113/+merge/15225
#launchpad-reviews 2009-11-25
<bac> nhandler: perhaps.  let me look.
<bac> nhandler: i'll try, if i can come up with the proper way to invoke ec2
<mwhudson> bac: ec2 land should figure that out for you :-)
<bac> nhandler: looks like i've figured it out.  it's off to ec2 and will land if the tests pass
<bac> mwhudson: yep, i just discovered.  damn, it's nice.
<mwhudson> yes
<bac> mwhudson: we should keep jml.
<mwhudson> heh
<mwhudson> i think we did
<nhandler> Thanks a lot bac
<mwhudson> thumper: want to review a branch that upgrades us to Twisted 9.0.0 (from an egg, no less)
<mwhudson> ?
<thumper> aye
<mwhudson> (hardly any changes, in fact)
<mwhudson> thumper: should have mail and a diff now
<mwhudson> thumper: thanks
* allenap changed the topic of #launchpad-reviews to: on-call: allenap || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<jml> bac, keep me where?
<gmb> allenap: Morning. Could you review https://code.edge.launchpad.net/~gmb/launchpad/subscribers-timeout-bug-487015/+merge/15238 for me please? 
<allenap> gmb: Sure :)
<gmb> Ta
* allenap changed the topic of #launchpad-reviews to: on-call: allenap || reviewing: gmb || queue [abel] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<bac> jml: the context was a discussion of how cool 'ec2 land' was.  i meant "keep you around" doing cool stuff.  all good.
<jml> :D
* jtv changed the topic of #launchpad-reviews to: on-call: allenap, jtv || reviewing: gmb, - || queue [abel] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<allenap> gmb: needs-information on your branch. I have to go out now, but we can talk about it later.
* allenap changed the topic of #launchpad-reviews to: on-call: allenap, jtv || reviewing: lunch, - || queue [abel] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<gmb> allenap: Okay, thanks.
<gmb> allenap: Ah, hell, I've completely misunderstood the use of subscription_class.
<gmb> Cocking cock cockity cock.
 * gmb goes for lunch, will rethink this over a chicken salad.
 * jtv suddenly understands why gmb was going on about cocks
 * jtv chokes back a bad ripoff of the "poultry in motion" pun
<jtv> adeuring: the topic says you have a review in the queue, but I don't see it
<adeuring> jtv: that's a private branch, ca. 400 lines diff. Want to review it?
<jtv> adeuring: sure
<adeuring> jtv: thanks!
* jtv changed the topic of #launchpad-reviews to: on-call: allenap, jtv || reviewing: lunch, abel || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<gmb> allenap: Reply sent.
* jtv changed the topic of #launchpad-reviews to: on-call: allenap, jtv || reviewing: lunch, - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<jtv> gmb: how was your cock salad?
<gmb> jtv: Bit salty.
<gmb> Ah...
<gmb> We're getting into mneptok territory here.
<jtv> well, you started it.
<jtv> not that it's a bad thing...
<jtv> (19:10:12) gmb: Cocking cock cockity cock.
<jtv> (19:12:27) ***gmb goes for lunch, will rethink this over a chicken salad.
<gmb> Quite so.
* allenap changed the topic of #launchpad-reviews to: on-call: allenap, jtv || reviewing: -, - || 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: allenap || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<allenap> gmb: Can you push your subscribers-timeout branch?
<gmb> allenap: FAIL.
<allenap> :)
<gmb> allenap: Pushed.
<allenap> gmb: Fanks.
<abentley> gary_poster: are you free to look at https://code.edge.launchpad.net/~abentley/launchpad/errorlog-context/+merge/15217 today?
<gary_poster> abentley: yes
<gary_poster> It's on my list
<abentley> gary_poster: Cool, thanks.
<gary_poster> np
<gary_poster> will try to get it sooner rather than later
<gary_poster> already started, but other bits intervened
<gmb> allenap: Ta for the review.
* EdwinGrubbs changed the topic of #launchpad-reviews to: on-call: allenap, Edwin || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<allenap> gmb: You're welcome dude :)
<allenap> EdwinGrubbs: https://code.edge.launchpad.net/~edwin-grubbs/lazr-js/activator-ie-fixes/+merge/14969 has been approved by mars, but there's an outstanding review request for ~launchpad. Is that bogus?
<EdwinGrubbs> allenap: yes, that's bogus.
<allenap> EdwinGrubbs: I've marked the proposal as approved.
<abentley> bigjools: I'm getting test failures in buildd-slavescanner.txt, with a pristine stable and up-to-date sourcecode.  Do you know why? http://pastebin.ubuntu.com/327796/
<bigjools> abentley: it looks like it merged a conflict wrong
<bigjools> the test is right, the output is wrong
<bigjools> al-maisan: can you take a look at that please? --^
<abentley> bigjools, al-maisan: I don't understand how failing tests got into stable.  I thought maybe it was a difference between karmic and hardy.
<bigjools> yeah it's odd
 * al-maisan looks
<al-maisan> this is very odd indeed
<al-maisan> abentley: is this a devel or db-devel branch?
<al-maisan> I take it it's devel
<abentley> al-maisan: No, it's stable.
<al-maisan> ah, OK
<al-maisan> abentley: what's the best way to fix stable? Fix it in devel?
 * al-maisan checks the devel branch
<abentley> al-maisan: Yes.  Stable is just the last revision of devel that has passed the tests.
<al-maisan> aye
<al-maisan> abentley: so, devel does not have the issue you observed.
<al-maisan> abentley: I guess the sample data in stable got skewed somehow -- that's why the error occurs.
 * al-maisan looks at the stable branch now
<abentley> al-maisan: devel and stable seem to have the same revno right now, so if stable is affected, devel must be affected.
<al-maisan> abentley: but that test passes on my system
<abentley> al-maisan: I have now reproduced the issue on devel.
<al-maisan> abentley: aha, how did you go about it?
<al-maisan> abentley: did you do a "make schema" before running the test in question?
<abentley> al-maisan: No, I didn't do make schema.
<al-maisan> but that's important here
<al-maisan> because the flaw is mostly buried in the sample data
<al-maisan> s/mostly/most likely/
<abentley> al-maisan: I see, but I don't normally run make schema if I can avoid it.
<al-maisan> well, you can't avoid it now :P
<al-maisan> abentley: so, "make schema; bin/test -vv -t buildd-slavescanner.txt" should pass in devel, it does for me.
<abentley> al-maisan: Yes, I'm running make schema now.  I'll let you know how it turns out.
<al-maisan> abentley: "make schema; bin/test -vv -t buildd-slavescanner.txt" passes in stable as well!
<abentley> al-maisan: confirmed: running make schema fixed it.  Thanks for your help.
<al-maisan> abentley: you are welcome
<abentley> al-maisan: Would it have been possible to land the sampledata update in db-devel rather than devel?  That would have avoided the issue for me.
<al-maisan> abentley: iirc wgrant introduced the sample data "clean-up" in a devel branch and unrelated to db schema changes
<al-maisan> we mandate db-devel for schema changes
<al-maisan> but not for sample data fixes afaik
<al-maisan> anyway, I am out of here :)
<al-maisan> Good night!
<sinzui> EdwinGrubbs: ping
<gary_poster> abentley: hi.  I have several questions.  I should start by saying that I like the general functionality, your clean ups, and the introduction of "with" to Launchpad :-) .  I do have a few design questions that may simply indicate a request to add some docs/comments, or may suggest some changes.
<gary_poster> Before I get into that, let me check my understanding.  Am I right that this munges request variable information with information that we think might be helpful into the same OOPS bucket?
<abentley> gary_poster: Yes.
<gary_poster> abentley: ok, cool.  glad I understood.
<abentley> gary_poster: That was where we were already putting that kind of info, via the ScriptRequest.
<gary_poster> abentley: I'm not familiar with that, but saw that class in the tests.  So, you mean, we were already (argualy ab-)using the request variable data using some other mechanism?
<abentley> gary_poster: Yes.
* allenap 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
<gary_poster> abentley: OK, cool.  So the first thought (of three that I know of) is that, while I'm fine with this mechanism being continued as an expedient choice, I'd prefer an XXX (and bug) saying that we would prefer a branch of OOPS tools that supported this sort of behavior explicitly, in a separate bucket that we could then add to our oops generation machinery.
<gary_poster> I'd also like to make sure that our API to support this general kind of functionality (that is, essentially annotating an oops with arbitrary information) not be tied to the current mechanism of storing and sending them.  IOW, I'd like to think about what we want, and then make an API for it, and then use this underlying implementation as an expedient solution for now.
<gary_poster> I have some thoughts on what it is I think we want, but can you go along with me so far, or do you have some concerns or thoughts?
<gary_poster> That was not said very clearly, sorry.
<abentley> gary_poster: I'm fine with the XXX/bug.
<gary_poster> cool.  OK, moving on.
<abentley> I'm not sure I understand about tying the annotation functionality to the current mechanism.
<abentley> gary_poster: ^
<gary_poster> abentley: right.  what I mean is, let's think about what we want on the error tool, divorced from the fact that we are going to implement it by stuffing it in request variables for now.
<gary_poster> Specifically, is this functionality really only for "Error Variables"?  It seems like that's one possible usage of many.  You could also store arbitrary status messages.  
<gary_poster> I almost see this more as a logger behavior, where the API should be to pass a string, with optional kw values to be interpolated into the string if it is turned into an OOPS.  We're annotating the OOPS with some information.
<gary_poster> Then, within the OOPS request variables, the location of the invocation of the context (i.e., the frame, as with a traceback) could be the first value, and the message would be the second.  This would suggest a different name for the function, of course.  ``contextAnnotation``?  ``contextLog``?  ``contextMessage``?
<abentley> gary_poster: I could see that maybe we'd want to do that, but YAGNI.
<abentley> gary_poster: (I tried to avoid the word 'context', especially because of the association of context managers with "with" statements)
<bac> EdwinGrubbs: could you do a review for me?
<EdwinGrubbs> bac: sure
<bac> EdwinGrubbs: great.  i've sent it off but it hasn't appeared yet
<bac> EdwinGrubbs: https://code.edge.launchpad.net/~bac/launchpad/bug-487965/+merge/15251
<gary_poster> abentley: sorry for taking so long, but you reminded me that you are duplicating functionality that is already provided.  I was getting the details for you.  I'd be happy if you used this mechanism.  It is even less work, because it should work now.  If you are willing to use this, the branch should be reduced to simply adding the desired information to the job code, and moving on.
<gary_poster> Our tracebacks are actually generated a zope exceptions package (http://svn.zope.org/zope.exceptions/trunk/src/zope/exceptions/exceptionformatter.py?view=auto fwiw).  If you put a variable named "__traceback_info__" with a string (or a value that can be cast as a string, such as a dict of strings) then it will be included in the traceback generated by the error tool.  Alternatively or additionally, if you put a variable 
<gary_poster> "__traceback_supplement__" with a tuple/list containing a callable first, followed by any args to call the callable with, then the output will be included in the traceback.
<gary_poster> I think this would easily satisfy what you need, and would do so without polluting the request variables, and would do so clearly indicating the frame in which the information was defined, and would do so without new code.
<abentley> gary_poster: It seems like these variables need to be globals, correct?
<gary_poster> abentley: they can be locals
<abentley> gary_poster: So you define them in the caller, and if the callee oopses, it gets access to them?
<gary_poster> abentley: not sure what the caller and the callee are in your question.  you define the variables wherever you know about them.  If there is an exception, and the error reporting utility is asked to render it, it generates a traceback including these variables in the output.  The tests for this are all unit tests; a doc test would actually have been nice.  Let me pastebin an example modified from the unit tests.
<abentley> gary_poster: In this case, the caller would be JobRunner.runAll, and the callee would be Diff.fromFile.
<gary_poster> abentley: http://paste.ubuntu.com/327911/ the callee doesn't look at the values.  the error reporting utility does, if it is rendering the traceback.  in this example, the print statement is essentially what the error reporting utility is doing.  Notice that the traceback has the interesting information in the middle of it.  This is what you should see in the OOPS tool.
<EdwinGrubbs> bac: review sent
<gary_poster> I'd be fine with sugar to make this prettier, but the results are better, I think.  And it is already part of what we have.
<gary_poster> (sugar: e.g., a callable that dumps the magic name in the locals or somthing.  I don't like the frame tricks, but that would keep you from misspelling __traceback_info__ without a warning)
<bac> thanks EdwinGrubbs.  nice catches, especially the cmp logic error that has been in production for a very long time
<abentley> gary_poster: I'm not getting good results, but it looks like the test suite doing stupid stuff to the traceback.  I'll let you know.
<gary_poster> abentley: ok cool
<leonardr> random zope question for gary or anyone really
<leonardr> i don't understand why these two lines of code are not equivalent:
<abentley> gary_poster: any idea why the tracebacks I generate in the test suite are only a single line?
<leonardr> getMultiAdapter((context, request), ICollection)
<leonardr> ICollection((context, request))
<gary_poster> leonardr: because in the second case you are trying to adapt a tuple.  There's a discussion about that on the zope list today as a matter of fact
<gary_poster> abentley: I'm afraid not
<gary_poster> abentley: Happy to futz if you want to give me a branch or a patch
<leonardr> gary: i'll look on the zope list if that'll answer my question more easily, but why is getMultiAdapter(tuple, interface) not 'adapting a tuple'?
<gary_poster> leonardr: oh!  sorry
<gary_poster> leonardr: oh, no, I was right the first time
<abentley> gary_poster: I think it's because I'm raising and catching the exception in the same function.  The traceback doesn't include the callstack of the function that catches the exception.
<gary_poster> leonardr: because that's the api
<gary_poster> leonardr: these are equivalent:
<leonardr> i guess that's good enough for me, i was just curious
<gary_poster> getMultiAdapter((context,), IFoo)
<gary_poster> IFoo(context)
<gary_poster> (mostly equivalent; there is some stuff that I consider legacy that happens in the second case, but it shouldn't affect anything)
<leonardr> so if i did ICollection((context, request),) it might work? not that i'm going to try that, at that point it's ugly
<gary_poster> abentley: oh, so this is a case in which the zope tools are insufficient, and we should go back to your original idea?
<gary_poster> leonardr: no
<abentley> gary_poster: I think this system only works when the exception handler is lower in the callstack than the method embedding the info.
<leonardr> gary: i see why not. ok
<gary_poster> abentley: well, you can pass the exception info explicitly to the reporter.  is this the last bit of your original diff? looking.
<gary_poster> abentley: I assume what you are trying is in lib/lp/services/job/runner.py .  could I see a pastebin of what you are trying?
<abentley> gary_poster: http://pastebin.ubuntu.com/327933/
<gary_poster> looking thx
<abentley> gary_poster: The JobRunner bit: http://pastebin.ubuntu.com/327936/
<gary_poster> abentley: http://pastebin.ubuntu.com/327937/
<gary_poster> or use a repr
<abentley> gary_poster: But runAll won't appear in the traceback.
<gary_poster> I mean, what you originally had was ``__traceback_info__ = repr(dict(job.getOopsVars()))``.  Maybe that is necessary for your keys or values, don't know
<gary_poster> huh, ok... http://paste.ubuntu.com/327911/ this seemed to imply that it would be fine.  Trying equivalent in interpreter.
<abentley> gary_poster: That's because the traceback includes the stack frame where the exception is handled.  It doesn't include the stack frame of the caller of the exception handler, AFAICT.
<gary_poster> abentley: fwiw, http://paste.ubuntu.com/327940/
<gary_poster> oh wait, maybe I misunderstand
<gary_poster> reviewing your code again
<gary_poster> abentley: oh!  you are saying, because you catch in runJob and reraise, this is hosed, right?  Sorry for not following before
<abentley> gary_poster: No, I didn't mean that.
<gary_poster> ok, still not following then.
<abentley> gary_poster: You seem to have disproved my theory about why I'm only gettting a single line of traceback.
<gary_poster> ah, ok
<gary_poster> abentley: I think what I said might be the case though
<abentley> gary_poster: I'm not looking at the rereaise case.  I'm looking at the case where Diff.fromFile handles an exception and does not reraise it.
<gary_poster> abentley: ok, will try to find that.  Was about to suggest http://pastebin.ubuntu.com/327944/
<abentley> gary_poster: I was simultating that with the handleError function.
<gary_poster> abentley: ah-ha.  with handleError, yes, your frame annotation will never get a chance to be called.  Is that a real example?  That is, do you need to be able to handle a job that calls reporter.handling itself?  If so, you can either use the mechanism I described within the job itself, or we need to go back to your original approach, and my other comments for it.
<gary_poster> s/That is, do you need to be able to handle a job that calls reporter.handling itself?/That is, do you need a job to be able to call reporter.handling itself?/
<abentley> gary_poster: Yes, I need a job to be able to reporter.handling itself.  That was the motivating case.
<abentley> I'm not sure how we could propagate that info into Diff.fromFile without doing something similar to what I proposed.
<gary_poster> abentley, ok, chalk that one to the perils of communication then.  Sorry fr not understanding your use case well enough.  So, back to your original design, yes.  We ought to document why and when it is appropriate as opposed to annotating a frame.  And I'd prefer for this to be a "give me something I can str" API for now, rather than a key-value pair API.  So to cut to the chase, my requests on your original branch:
<abentley> gary_poster: The reason I'm not just passing the vars into raising via the ScriptRequest is that Diff.fromFile is high in the callstack (that is, has many indirect callers) and has no information about the job, or any reason to expect it's being called from a job.
<gary_poster> abentley: I like your API generally; I'm already sold on the basic idea
<gary_poster> - add a bug/XXX saying that we want to store the information from this API someplace else other than polluting the request info
<gary_poster> - change the API to accept something that is or can be cast into a string
<gary_poster> - stuff the value into the request variables with some nasty key so it is clear where it is coming from (i.e., not the request)
<gary_poster> - move on
<gary_poster> abentley: you ok with that?  happy to explain rationale if needed
<gary_poster> abentley: forgot one: change the name to something appropriate to this behavior of recording a string
<abentley> gary_poster: Where do you want the "give me something I can str?"  If you want that in contextErrorVariables, that doesn't lend itself to multiple uses.
<gary_poster> abentley: yes, that was my intent (though with a different name, as I said).  why not?  The end goal is to produce something that can be output to an error report--a string.
<abentley> gary_poster: In my design, you can do contextErrorVariables(a=b) in a caller, and contextErrorVariables(c=d) in a callee, and they're all nicely combined together.
* sinzui changed the topic of #launchpad-reviews to: on-call: Edwin || reviewing: - || queue [sinzui] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<gary_poster> abentley: not sure why that is an advantage over errorAnnotation(('foo', 'bar')) and errorAnnotation(('baz', 'bing')).  That can produce request vars of '__error_annotation__': "('foo', 'bar')" and '__error_annotation__': "('baz', 'bing')" 
<abentley> gary_poster: You end up with multiple strings that way, one for every invocation of contextErrorVariables.  Then you have to combine them somehow, and you lose alphabetical sorting, so you probably have to put in separators so people know that the sorting restarts.
<gary_poster> that is, the nesting and duplicate values is fine
<gary_poster> abentley: I don't understand why the sorting is valuable.
<sinzui> EdwinGrubbs: can you review my fix for the milestone table.
<EdwinGrubbs> sinzui: sure
<abentley> gary_poster: Sorting makes it easy to find something in a long list.
<gary_poster> abentley: Sure; I'm fine with sorting request variables.  Sorting these things makes the order unknown.  If you make the modification I'm suggesting, you can find these annotations easily one place in the request variables for now.  You could even make the key have a suffix based on the nesting depth of the annotation.
<gary_poster> I really don't like the fact that you can't tell what came from the request and what came from your addition.  I also don't think key-value pairs are something to cook into the API.
<abentley> gary_poster: I wasn't arguing for sticking them in with the request variables.
<gary_poster> abentley: oh...not cleat then, sorry
<gary_poster> clear
<abentley> gary_poster: I don't understand what's wrong with a key/value API.  I like to keep my variables machine readable until it's time to format them.
<abentley> I don't understand why sorting key/value pairs based on their key name would make their order unknown.
<gary_poster> abentley: I don't understand why you don't understand, which usually means I have a base misunderstanding.  I think we should try to bring this to a close.  Do you want to try Skype, to move more quickly?
<gary_poster> or SIP is fine
<abentley> gary_poster: let's try SIP.
<thumper> gary_poster: at least for our use case, the order doesn't matter
<gary_poster> ok
<thumper> gary_poster: in what situations are you thinking that the order might matter?
<abentley> gary_poster: I am sip:7369@canonical.com
<gary_poster> thumper: I believe Python's sort now maintains ordering within equivalent values, so if that's the case then it's a matter of readability, because you can't immediately tell what values come from the request, what come from one of these error log annotations, and what comes from a lower level of error log annotations.  When I look at the results, I want to be able to see what values come from where, without having to loo
<gary_poster> sourcecode.
<gary_poster> Maybe abentley will convince me of the error of my ways; about to call. :-)
<gary_poster> abentley: does this jibe with your understanding?  http://paste.ubuntu.com/328032/
<abentley> gary_poster: That's great.
<gary_poster> cool, thanks.  submitting
<EdwinGrubbs> sinzui: review sent
* 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
<sinzui> thanks
* 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
<thumper> https://code.edge.launchpad.net/~thumper/launchpad/allow-admin-to-set-branch-privacy/+merge/15259 pretty please
<thumper> this will make our losas happy
#launchpad-reviews 2009-11-26
<thumper> mwhudson: could you please look over the above review?
<thumper> mwhudson: it's short and will please the losas
<mwhudson> thumper: okay
<thumper> thanks
<mwhudson> thumper: the diff lacks enough context for me to tell this, but i presume there's a test that a random user cannot make a branch private?
<EdwinGrubbs> mwhudson: are you the OCR today?
<mwhudson> EdwinGrubbs: i guess i am
<mwhudson> thumper: also should special users be allowed to make any branch public?
<mwhudson> thumper: it doesn't look like they can here?
<EdwinGrubbs> mwhudson: can you review a 125 line diff for me? https://code.edge.launchpad.net/~edwin-grubbs/launchpad/bug-297833-invite-private-team/+merge/15258
<mwhudson> EdwinGrubbs: looking
<mwhudson> EdwinGrubbs: self.request.form.get('field.newmember') is pretty horrid
<mwhudson> EdwinGrubbs: is there no way of getting a widgets value if it's not considered valid by the Field?
<mwhudson> (i can believe there's not but it's a bit sad)
<mwhudson> thumper, EdwinGrubbs: these are not a very online reviews
<mwhudson> EdwinGrubbs: i don't see how to reproduce the "I also noticed that the $team/+add-my-teams would OOPS if you" part
<thumper> mwhudson: yes there is
<thumper> mwhudson: I did think about making any branch public and decided against it
<thumper> mwhudson: because the only time they can't is if the policy says private only
<mwhudson> thumper: oh right
<thumper> mwhudson: I wanted the UI to not allow them to shoot themselves in the foot
<thumper> mwhudson: they technically could through the api
<mwhudson> yeah, that's fair enough
<thumper> mwhudson: but the UI is a bit safer
<mwhudson> thumper: reviewed
<thumper> mwhudson: ta
 * mwhudson goes to buy some cake
<mwhudson> thumper: want to review a bzr-svn branch?
<EdwinGrubbs> mwhudson: sorry about disappearing after you started the review. I'll find someone else to finish it. Thanks for the feedback.
<thumper> mwhudson: sure
<mwhudson> thumper: https://code.edge.launchpad.net/~mwhudson/launchpad/bzr-svn-monitor/+merge/15264
<mwhudson> thumper: my plan is to land this, and enough to stop the ui blowing up when it sees a bzr-svn import
<mwhudson> then we can test on staging by getting the losas to make bzr-svn imports with sql
<mwhudson> and if they work and add the ui to create them on edge
* noodles775 changed the topic of #launchpad-reviews to: on-call: noodles775 || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<jtv> noodles775: got one for you... https://bugs.edge.launchpad.net/rosetta/+bug/488218
<noodles775> jtv: on it.
<jtv> mp at https://code.edge.launchpad.net/~jtv/launchpad/bug-488218/+merge/15247
<jtv> thanks!
* noodles775 changed the topic of #launchpad-reviews to: on-call: noodles775 || reviewing: jtv || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<bac> jml:  in light of the current problem with ec2 failing i've added an option to ec2 land to make it *not* go headless.  would you like to review it?
<jml> bac, sure, but I don't see how that helps.
<jml> bac, wouldn't it be better to address the root cause of the problem?
<bac> jml: i assume mwhudson is working on the root cause.  staying attached did help me get my branches landed last night but i thought it might be general enough to be useful in the future.
<jml> bac, oh right, an option. sorry I misread :)
<bac> jml:  ec2 land --attached
<bac> using land and going headless are somewhat orthogonal
<bac> though i agree headless should be the default
<jml> bac, agreed. 
<jml> bac, I didn't add the option because I couldn't think of a use-case for attached mode.
<jml> (fwiw, I wouldn't assume mwhudson is working on it myself)
<bac> jml: i'm not sure if mwhudson is or not.  this work was done last night before the cause of the failure was discovered.  i assume the Build Engineer (who's that now?) will have a look.
<bac> jml: i would but i'm on holiday today
<bac> and about to head out
<jml> bac, oh right, I forgot about the turkeys.
<bac> jml:  anyway, a MP was just submitted.  if you want to review it please do.  i'll check back later and respond/land.
<jml> bac, will do.
<bac> jml:  thanks!
<gmb> noodles775: Can I stick https://code.edge.launchpad.net/~gmb/launchpad/prince-of-ajax-dupefinding-bug-358510/+merge/15273 on your queue?
<noodles775> yup :)
<gmb> Kewl.
* gmb changed the topic of #launchpad-reviews to: on-call: noodles775 || reviewing: jtv || queue [gmb] || 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 || reviewing: gmb || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<noodles775> gmb: where is launchpad_form_id actually set? I assume a parent template?
<gmb> noodles775: Yes. It's not used in this branch, it will be used in subsequent branches. I can land it with one of those if you'd prefer.
<noodles775> gmb: well, there's a number of things in this branch that indicated that you wouldn't be landing this? (empty errors, 'FAIL' etc.). Is there any reason to land this? Why can't it be landed with the subsequent branches?
<noodles775> or is that what you meant?
<gmb> noodles775: Damn, I must've missed trimming that crap out. Sorry. I've had to merge this in from old branches bits and pieces at a time.
<gmb> noodles775: And I don't want to land it all in subsequent branches because it's 400+ lines already.
<gmb> noodles775: As a whole, this thing is well over 1200 lines.
<gmb> So I've got to land it in bits.
<noodles775> gmb: but that shouldn't matter (using the 'Prerequisite branch' option, and/or pipelines)?
<noodles775> I landed a 3.5k branch which was 7 separate reviews... it should work?
<noodles775> But that's up to you.
<noodles775> It's just quite difficult to review without being able to see it, or test it.
<gmb> noodles775: Okay. Leave the review for now; I'll have to get back to you.
<gmb> Thanks anyway.
<noodles775> gmb: I'm happy to r=me if it's not going to land until it's tested in subsequent branches etc.
<gmb> noodles775: In that case, please do. I'll have another branch ready by tomorrow anyway.
<gmb> noodles775: Sorry, just having to juggle things here at the moment.
<noodles775> OK.
<noodles775> Np.
<jtv1> gmb: almost done with that branch... just cleaning up the test
* noodles775 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
<BjornT_> noodles775: landing big branches is technically possible, but should still be avoided. big diffs makes it harder to back out revisions, and it's easier for mistakes to slip through. some of us actually look at the diff that lands :)
<noodles775> BjornT_: Yeah, I agree of course, but I still think the smaller branches need to be complete units of work (ie. that are tested)
<BjornT_> noodles775: yes, of course
<noodles775> For the large branch that I mentioned above, it was for the PPA index redesign... I landed all the backend branches in small parts where possible, but there remained a large number of dependent branches that couldn't be landed on their own.
<noodles775> Maybe I just didn't find the best sequence of small steps.
<jtv> gmb: requested review from you.
<gmb> jtv: Cool. I'll look at it shortly.
<jtv> gmb: I'll have to eod now and give my wrists a rest.
<jtv> The test is almost all the work.
<gmb> Righto.
<abentley> thumper: Any chance you could review https://code.edge.launchpad.net/~abentley/launchpad/fix-inline-reply/+merge/15295 ?
<thumper> abentley: ok
#launchpad-reviews 2009-11-27
<thumper> mwhudson: could I talk through https://code.edge.launchpad.net/~thumper/launchpad/edit-commit-msg-link/+merge/15260 with you?
<thumper> mwhudson: it is very unexciting
<thumper> mwhudson: the only major change is LPS vs YUI() in many places
<thumper> mwhudson: I talked this through with mars and he is v.happy that someone else is taking something off his plate
<mwhudson> thumper: okay
<thumper> mwhudson: thanks
<mwhudson> thumper: do you want to skype?
<mwhudson> i should read the diff first in any case
<thumper> sure
<thumper> ping me if you want to cover anything
<mwhudson> thumper: i'm trying to play with it but things keep getting in my way and then i get distracted
<thumper> :)
<thumper> I'll have another soon
<thumper> normalizes the review_type in the merge proposal model code
<thumper> so '  ' -> None
<mwhudson> ah cool
<thumper> waiting for the scanner on that one
<mwhudson> thumper: the commit editor doesn't work for me any more in launchpad.dev :(
<thumper> ??
<thumper> what do you mean?
<mwhudson> thumper: i click the pencil icon and it works like a link
<thumper> :)
<thumper> I know what that is
<thumper> make clean build
<thumper> yui 3.0
<mwhudson> ok
<mwhudson> another thing to delay this review :(
<mwhudson> thumper: hm, i was merging it into devel :(
<thumper> :(
<mwhudson> ah now it works at last
<mwhudson> complete with duff looking multi-line editor :-)
<thumper> https://code.launchpad.net/~thumper/launchpad/set-empty-review-type-as-none/+merge/15302
<thumper> mwhudson: yeah, I think deryck is looking at that one
<mwhudson> thumper: in test_pending_review_registrant it's not super obvious to me that the merge_proposal.registrant is who's passed into nominateReviewer
<thumper> hmm.. yeah
<mwhudson> thumper: could you give makeProposalWithReviewer an optional "requester" argument do you think?
<thumper> and default it?
<thumper> sure
<mwhudson> ywah
<mwhudson> yes even :)
<mwhudson> thanks
<mwhudson> thumper: reviewed
<thumper> ta
<jtv> stub: I thought I could get mwhudson to review my testfix branch, but haven't heard from him in a bit... want to help out?
<jtv> https://code.edge.launchpad.net/~jtv/launchpad/bug-489068/+merge/15309
<stub> jtv: That shouldn't be an XXX - it should just be a standard comment
<jtv> Fixing...
<stub> jtv: Wouldn't the real fix be to include boto in the launchpad-developer-dependencies?
<jtv> stub: well it's not used in the test, and probably shouldn't be
<jtv> I can just imagine the potential for recursion if this test really fired up an EC2 image.  :)
<stub> We try to keep the environments as similar as possible - this is just sweeping this drift between the environments under the carpet where it might bite someone later.
<stub> (Which is understandable, as it is busted *now* and updating ec2 images seems to take days or longer)
<jtv> in that case, I can file a bug for having a boto egg (or launchpad-dependency?)
<stub> Yes. I'm wondering if that comment should be an XXX referencing that bug then.
<jtv> Fair enough.
<stub> approved
<jtv> I'll make them separate comments for the two imports.
<jtv> thanks
* 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
<allenap> adeuring: Could you review my multithreaded checkwatches branch please? https://code.edge.launchpad.net/~allenap/launchpad/multithreaded-checkwatches/+merge/15283
<adeuring> allenap: sure
<allenap> adeuring: Thanks :)
* adeuring changed the topic of #launchpad-reviews to: on-call: adeuring || reviewing: allenap || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<adeuring> allenap: the pattern try: return func(...); finally: self._logout() in _interactionDecorator() looks a bit odd. It is not wrong, but jtv spotted a bug in a similar pattern in ec2test. Effectively, the "return" transforms the "Finally" into an "except", so I think it is better to use the latter
<allenap> adeuring: I don't understand. I thought the thing in ec2test was a try...else?
<adeuring> allenap: ouch, right. But let me test this "finally" nevertheless...
<adeuring> allenap: OK, the "finally" code is indeed called! my bad... sorry for the noise
<allenap> adeuring: Not at all, it's fine :)
<adeuring> allenap: perhaps I misunderstood something aabout threading, so i may be wrong. but: updateBugTracker() calls self.txn.begin() and later self.txn.commit(). As I understand it, all worker threads share one "self" instance, so don't they begin/commit the same transaction?
<allenap> adeuring: This was a fun thing for me to work through :) The transactions are stored as thread local vars. I think self.txn is just a reference to the transaction module.
<adeuring> allenap: that's really interesting. Could you add a comment explaining this so that other people don't ask the same question as I did ;)?
<allenap> adeuring: Sure, good idea.
<adeuring> allenap: last question (hopefully...): do you know if calls like self.log.inf() or self.warning() are thread safe?
 * adeuring is a bit paranoid regarding threading
<allenap> adeuring: I don't know actually. I'll check.
<allenap> adeuring: The logging module has been written to be thread-aware (though I've spotted a race-condition already). I think that's enough.
<allenap> adeuring: What do you think?
<adeuring> allenap: yeah, sure. Though it might be worth to file a bug about the race condition
<allenap> adeuring: Yeah, definitely :)
<adeuring> allenap: r=me
* 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
<jtv> adeuring: got a review for you!  https://code.edge.launchpad.net/~jtv/launchpad/bug-487447/+merge/15315
 * adeuring is looking...
* adeuring changed the topic of #launchpad-reviews to: on-call: adeuring || reviewing: jtv || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<adeuring> jtv: you're adding a permission for public.project. Do you really need that?
* 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
<jtv> adeuring: hi, sorry for the delay... yes I want to do that because a project can have a translation group.
<adeuring> jtv: ah, right, I didn't know that. Anyway, r=me
<jtv> adeuring: thanks!
<adeuring> welcome :)
<allenap> adeuring: Do you fancy another review today?
<adeuring> allenap: sure
<allenap> adeuring: It's a big one at ~1500 lines but there's a lot of removals and refactoring in there. Take a look and see if you feel like doing it. I won't mind at all if you say no :) https://code.edge.launchpad.net/~allenap/launchpad/update-project-group-page-bug-434764/+merge/15322
<allenap> adeuring: I just don't know how to break it up now :(
<adeuring> allenap: I'll give it a try ;)
<allenap> adeuring: Thanks :)
<noodles775> adeuring: time for a one-liner? https://code.edge.launchpad.net/~michael.nelson/launchpad/470181-ppa-inline-style/+merge/15325
<adeuring> noodles775: nice distraction from a novel I'm reviewing ;)
<noodles775> lol
<adeuring> noodles775: r=me
<noodles775> Thanks!
<adeuring> allenap: r=me; 1 nitpick
<allenap> adeuring: Woohoo! Thank you :)
* adeuring 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 2009-11-28
<allenap> gmb: Are you around? I need to ask for a review for a testfix.
<allenap> jml: You wouldn't happen to be around to review a testfix.
<allenap> ?
<allenap> mwhudson, thumper: Are either of you around to review a testfix?
<jml> allenap, what are you doing up so early on a saturday?
#launchpad-reviews 2010-11-29
<wallyworld_> StevenK: are you able to take another look at those recipe build listing branches? - i've made some changes as suggested by you and tim
<jtv> Hi gmb, got a very light review for you, with a bigger one to follow: https://code.launchpad.net/~jtv/launchpad/recife-pre-stats/+merge/42079
* 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
<jtv> No reviewers today?
* jtv changed the topic of #launchpad-reviews to: On call: - || Reviewing: - || queue: [jtv, jtv] ||  This channel is logged:  http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jtv> stub: are you as busy as henning is?  Need some reviews!
<henninge> jtv: what makes you think I am busy? ;-)
<henninge> No staging - no QA
<jtv> henninge: okay, then I need some reviews!
<henninge> let's do one at a time ... ;)
* henninge changed the topic of #launchpad-reviews to: On call: jtv || Reviewing: - || queue: [jtv, jtv] ||  This channel is logged:  http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jtv> Yes, let's.  :)
* henninge changed the topic of #launchpad-reviews to: On call: henninge || Reviewing: jtv || queue: [ jtv] ||  This channel is logged:  http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<henninge> wow, in-line diffs for updates.
<jtv> Yes, we have those now.
<henninge> jtv: very nice simplification of the test.
<henninge> jtv: I just don't see what you describe as "runs in both old-model and new-model statistics calculation."
<jtv> In all honesty, I did it to get the test to start up faster.  :-)
<jtv> I clear both flags as an alternative to deleting the message.
<henninge> ah! so lines 148-151 could use a comment I think ;)
<henninge> jtv: but apart from that, r=me ;)
 * jtv looks
<jtv> OK, I'll note that.
<henninge> thanks
* henninge changed the topic of #launchpad-reviews to: On call: henninge || Reviewing: jtv || queue: [ ] ||  This channel is logged:  http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<henninge> ok, the next branch is a bit larger
<jtv> Yup, though this pre-branch got it under the 800 line.
<jtv> henninge: actually, instead of a comment, how about I just fix getPOFilesWithTranslationCredits?
<henninge> in what way?
<jtv> By making it look at the translation side that's appropriate for the POFile, rather than always at is_current_ubuntu.
<jtv> (In other words, updating it to the Recife model).
<jtv> By the way, my router had come back to life but went back into a coma again, so I'm on a slow edge connection today.  Makes it hard to  get to the kanban board. :(
<henninge> jtv: so I guess voice chat is out of the question?
<jtv> henninge: pretty much :(
<henninge> :(
<henninge> jtv: if the test works as it is now you should finish the statistics stuff first - although I fear that is all inter-related.
<jtv> henninge: might as well finish this while you're reviewingâit's not a big change.
<jtv> Better this than forget to fix it!
<henninge> that's cool, too
<jelmer> g'morning!
<henninge> Hi jelmer ;)
<jelmer> henninge: Can I add another branch to the queue?
<henninge> jelmer: you always can ydd a branch to the queue ... :-P
<henninge> s/ydd/add/
* jelmer changed the topic of #launchpad-reviews to: On call: henninge || Reviewing: jtv || queue: [jelmer ] ||  This channel is logged:  http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jelmer> henninge: thanks (-:
<henninge> jtv: what does that change "Merging preparatory branch" mean? Where do all the property changes come from?
<jtv> henninge: I merged the branch you just reviewed, but since that one was branched off db-devel whereas this branch was branched off recife (which in turn was branched off an even older db-stable), it brought in a lot of other changes that won't show up in a diff against db-devel.
<henninge> I see
<jtv> In other words: irrelevant updates.  :-)
* jtv changed the topic of #launchpad-reviews to: On call: henninge || Reviewing: jtv || queue: [jelmer, jtv] ||  This channel is logged:  http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<henninge> jtv: what's the trick in this?
<henninge> 137	+        for count, in IStore(self).execute(query):
<henninge> 138	+            return count
<henninge> Would [0] not do the same?
<henninge> or I think there is even a "first" method.
<jtv> Only if I listify first.  I wish I had a really concise way of stating it, but I'm willing to go with any suggestion you may have.  :)
<jtv> Oh, first, that might work.
<henninge> jtv: and what about "one"?
<henninge> I am still trying to figure out why you'd only want the first, or why the query returns more than one.
<jtv> There is only one.
<jtv> henninge: first() does the trick.  Also magically turns the tuple-shaped row into a single int, which is what I really want.
<henninge> jtv: one() does not do that?
<jtv> The whole trick to that code was that I had a result set that looked like an iterator over [(n)], and I just wanted the n.
<jtv> Oh, hang on, first() did _not_ work.  What I saw was a screenful of errored tests.
<jtv> There is no "one" (or first) on this type of result set.
<jtv> Duck-typing to the descue.
<jtv> Or whatever the opposite of rescue is.
<henninge> jtv: what is "this type of result set" ?
<jtv> PostgresResultSet.
<jtv> Store.execute returns something different than Store.find.
<henninge> jtv: it has get_one, though
<henninge> http://people.canonical.com/~jkakar/storm/storm.databases.postgres.PostgresResult.html
<jtv> ah!
<jtv> Now trying return IStore(self).execute(query).get_one()[0]
<jtv> That works.
<jtv> Thanks.  I always look through the documentation in the source code, but didn't find anything helpful.
<jtv> Change pushed.
<henninge> cool
<jtv> Meanwhile, could anyone else review this other very small branch?  https://code.launchpad.net/~jtv/launchpad/recife-getpofileswithtranslationcredits/+merge/42083
<jtv> jelmer, is that something you could do?
<jelmer> jtv: Sure, should we trade branches? Mine is very small as well.
<jtv> jelmer: sure!
<jtv> Got a direct link?  My connection's so slow right now it'd save me a lot of time.
<jelmer> https://code.launchpad.net/~jelmer/launchpad/681974-building-commit/+merge/42081
 * jtv goes into a frenzy of browsing attempts
<jtv> jelmer: good cover letter.  I'm surprised it's the buildd manager confusing the upload processor by moving the file before committing, and not another way around.
<jelmer> jtv: This is happening with binary builds that the buildd manager has fetched from the builders.
<jelmer> jtv: There is another archive uploader instance that processes the source uploads from users, which the buildd manager then sends off to the builders.
<jtv> jelmer: ah so the data flows in the opposite direction from what I thought.
<jtv> Depending on what the upload processor's transactions look like and even which isolation level it uses, there may _conceivably_ still be race conditions but even then this should narrow it own.  And I'm not assuming that you got it wrong anyway.  :)
<jtv> I'm also assuming that you're not committing in some untenable state, and therefore r=jtv.
<jelmer> jtv: Thanks :-)
<jelmer> jtv: I don't understand why line 27 of your change is now necessary, how is it related to your other changes?
<jelmer> + clauses.append(POTemplate.id == POFile.potemplateID)
<jtv> jelmer: yes it is a bit obscure.  It pulls POTemplate into the join.
<jtv> I did reference it, but in a nested query.  And so that join doesn't get pulled into the outer query.
<jelmer> ahh, I see
<jtv> The ORM probably couldn't do that if it wanted, because it'd be ambiguous whether I wanted it in the outer query or just the inner one.
<jtv> (In this case it doesn't matter much, but I'm assuming in a hand-wavy way that it might in other cases)
<jelmer> jtv: yeah, I can see how that could make a difference.
<jelmer> jtv: That was all I had. r=me
<jtv> dankuwel
<jtv> phew, my router is working again
* jtv changed the topic of #launchpad-reviews to: n call: henninge || Reviewing: jtv || queue: [] ||  This channel is logged:  http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* jtv changed the topic of #launchpad-reviews to: On call: henninge || Reviewing: jtv || queue: [] ||  This channel is logged:  http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jtv> jelmer: if you use "ec2 land," there's no need to include the [r=â¦] tag etc. in your commit message.  That gets added automatically.
<jelmer> jtv: I didn't add anything, ec2 land did!
<jtv> oh!
<jelmer> I guess it's now modifying the commit message on lp before send it off to ec2.
<jtv> there's no stopping progress, I suppose
<jelmer> :)
<jtv> That could actually be helpful when landing fails because of testfix.
<jtv> No endless re-composing your commit message for every landing attempt.
* henninge changed the topic of #launchpad-reviews to: On call: henninge || Reviewing: - || queue: [] ||  This channel is logged:  http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<henninge> jelmer, jtv: thanks guys for taking each other's reviews.
<henninge> jtv: r=me with a few comments to consider. ;-)
 * jtv jelmer & henninge all bow at each other
<jtv> henninge: great, thanksâsorry for the heavy review loa
<jtv> d
<henninge> :-D
<henninge> jtv: np. I will see that I get the migration script run now that staging is alive again. Or did you already take action?
<jtv> henninge: no, I didn't know it was up!
<jtv> henninge: you're right that there's no point to renaming that method I'm replacingâI just did it to keep your diff to a reasonable size, and left cleaning up the now-unused method as one of the TODO items.
<jtv> I can remove it now if you're willing to rubber-stamp itâthere are no calls to it.
<henninge> jtv: that's what I meant - no calls. ;-) rs=me
<jtv> Thanks.
<abentley> henninge: Could you please review https://code.launchpad.net/~abentley/launchpad/bzr-2.2.2/+merge/41981?
<henninge> abentley: r=me
<jam1> henninge: can I get a quick review of: https://code.edge.launchpad.net/~jameinel/launchpad/lp-service/+merge/42129
<jam1> That was already merged and approved into db-devel, but I had meant to target devel
<jam> hmm... the diff appears to be empty, maybe db-devel has already become devel?
<henninge> jam: the branch's status is "Merged"
<jam> anyone know when that happened? l-osas said the functionality wasn't present last Wed
<jam> henninge: right, it *was* merged into db-devel, but I meant to target 'devel'
<henninge> jam: when did that happen? Merging into db-devel, I mean?
<jam> I thought I saw that db-devel was going to be merged/pushed over devel, but I don't know for sure that happened
<jam> henninge: *My* branch was merged into db-devel probably 2 weeks ago, or so
<henninge> jam: it happened on the 16th
<jam> I didn't realize I had the wrong target unti the 24th
<jam> (well, I didn't realize until today :)
<henninge> jam: devel and db-devel are merged before each roll-out.
<jam> I was trying to get someone to use the new code, and they said it didn't work on the 24th.
<jam> but right now, it appears to be properly merged into devel
<jam> which is confusing me
<henninge> jam: it was merged on the 9th as r9951, r9973 was rolled out (if I remember correctly)
<henninge> jam: so, yes, your code is deployed.
<jam> henninge: thanks. Now I have to figure out why it wasn't available on qastaging...
<jam> or if something else weird is going on
<jam> anyway, thanks for your help
<henninge> jam: it is there for sure (unless it was reverted by a leter revision)
<henninge> s/leter/later/
<jam> henninge: I just cleared out the mp, sorry about the noise
<salgado> hi henninge.  can you review a trivial one for me?
* henninge 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
<henninge> salgado: sure
<henninge> last onen for today ;)
<salgado> henninge, https://code.launchpad.net/~salgado/launchpad/refactor-blueprints-tests/+merge/42139
<salgado> thanks!
<bac> hi jcsackett, sorry for taking so long but i think your branch looks great
<jcsackett> bac: cool! glad to hear it.
<henninge> salgado: r=me ;)
<salgado> thanks henninge
* abentley changed the topic of #launchpad-reviews to: On call: abentley || Reviewing: - || queue: [] ||  This channel is logged:  http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<salgado> abentley, how about a trivial one to start the day? ;)
<salgado> https://code.launchpad.net/~salgado/launchpad/expose-blueprint-dependencies/+merge/42145
<abentley> salgado: sorry, was lunching.
* abentley changed the topic of #launchpad-reviews to: On call: abentley || Reviewing: salgado || queue: [] ||  This channel is logged:  http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<salgado> abentley, np.  I've just pushed another tiny change there, though, so you might want to wait for the diff to update
<abentley> salgado: Is this test refactoring something I should review?
<salgado> abentley, the ones to use assertContentEqual?  Yes, although they're not related to the changes on the branch I thought you wouldn't mind reviewing them together?
<abentley> salgado: Sure.  I just noted they were merged from another branch, so i wasn't sure.
<salgado> but the "merge test refactoring from other branch" has landed on devel already
<salgado> abentley, I just had to merge that directly from the branch because at that time it wasn't on devel
<abentley> salgado: So you could have set that other branch as a prerequisite branch.
<salgado> abentley, yeah, although it wasn't really.  I just merged to make sure there'd be no conflicts
<abentley> salgado: But now that you've merged it, you can't land this branch until that one's approved.
<salgado> abentley, but the other one has landed already
<abentley> salgado: now, it has, and I see that in the updated diff.  I was looking at an older one a minute ago.
<salgado> it landed 15 mins ago or so.   but 1h ago it hadn't landed and I merged to make sure there'd be no conflicts
<salgado> abentley, sorry for the confusion
<abentley> salgado: do you know about lp.testing.ws_object?  It's a more general version of getSpecOnWebservice.
<salgado> abentley, nope, I didn't know about that
 * salgado checks
<abentley> salgado: The functionality looks fine.  At 23, you should use a multi-line import.  r=me.
<salgado> abentley, I could use that in getSpecOnWebservice() but I don't see much benefit in doing so
<salgado> abentley, oh, so multi-line imports are to be used even when they're not necessary?
<abentley> salgado: yes.
<abentley> salgado: I don't think you need getSpecOnWebservice at all.
<salgado> you mean to inline its functionality everywhere?
<abentley> salgado: I mean use ws_object instead.
<salgado> that's basically the same as I still need to get the Launchpad instance.  although I could store that as an instance variable on the testcase class
<salgado> that doesn't sound like a good idea
<salgado> abentley, or do you mean something else?
<abentley> salgado: It just seems like wasted effort to have different functions for getting objects via the web service, one for each type  of object, when you could just have one.
<abentley> salgado: If you'd like to enhance ws_object so that the launchpad parameter is optional, that would be a nice improvement for everyone.
<salgado> abentley, I'm not sure that'd help in this case as these tests need a 'devel' Launchpadlib and I don't think that should be the default
<lifeless> you could curry something
<salgado> curry?
<abentley> salgado: so we could have a parameter on ws_object to specify the launchpadlib version.  That would also be helpful for everyone.
<abentley> salgado: I don't think wanting to vary the launchpadlib version is a good reason to have different implementations of ws_object for different objects.
<abentley> salgado: At most, it's a reason to have different versions of ws_object for different launchpadlib versions.
<salgado> I don't think so either; I'm just saying that the current one is not a good fit
<salgado> but I can change it as you suggest
<abentley> salgado: I've already approved this, so it's only a suggestion to consider.  I just think it's better to spend effort in ways that help the whole team, where we can.
<salgado> agreed, and that's why I'm happy to do the change you suggested. :)
<abentley> salgado: cool.
* abentley changed the topic of #launchpad-reviews to: On call: abentley || Reviewing: - || queue: [] ||  This channel is logged:  http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<salgado> abentley, the annoying thing is that the launchpad object is the first argument to ws_object() and it doesn't make sense to make both arguments optional so I'd have to change the method's signature and update all the callsites. :(
<abentley> salgado: It's true.
<salgado> abentley, I think I'll save this work for later, then
<abentley> salgado: sure.
<jcsackett> abentley: have one to throw on your queue. https://code.edge.launchpad.net/~jcsackett/launchpad/distribution-enum-selection-677536/+merge/42161
* jcsackett changed the topic of #launchpad-reviews to: On call: abentley || Reviewing: - || queue: [jcsackett] ||  This channel is logged:  http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<abentley> jcsackett: ack
<abentley> jcsackett: r=me
<jcsackett> abentley: thanks!
* abentley changed the topic of #launchpad-reviews to: On call: abentley || Reviewing: - || queue: [] ||  This channel is logged:  http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<gary_poster> abentley: 4 lines when you get a chance: https://code.launchpad.net/~gary/launchpad/ec2lucid/+merge/42179
<abentley> gary_poster: r=me
<gary_poster> thanks abentley
* 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
#launchpad-reviews 2010-11-30
* gmb changed the topic of #launchpad-reviews to: On call: gmb || Reviewing: - || queue: [] ||  This channel is logged:  http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* gmb changed the topic of #launchpad-reviews to: On call: gmb || Reviewing: lunch || queue: [] ||  This channel is logged:  http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* gmb changed the topic of #launchpad-reviews to: On call: gmb || Reviewing: wallyworld || queue: [] ||  This channel is logged:  http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<gmb> wallyworld_: Who was your pre-implementation contact for your branch? I ask because whilst your solution seems sane I don't have enough domain knowledge to know whether or not there's a better way to do it.
<wallyworld_> gmb: which one? the codehosting error leakage?
<gmb> wallyworld_: Yes, sorry.
<wallyworld_> gmb: i talked with tim, aaron, and michael hudson. michael has an issue about raising a second oops but i couldn't see another way to do it. but i'm really new to this section of code. i'm sure tim wouldn't mind if you wanted to run any concerns by him
<gmb> wallyworld_: I just wanted to check who you'd talked with about it; I'm not familiar with the code either. If Tim et. al. have given you their blessing that's good enough for me.
<wallyworld_> gmb: ok. i'll double check with tim to ensure he is 100% happy because i really don't want to screw anything up
<wallyworld_> thanks for looking at the code
<gmb> wallyworld_: Cool, works for me. Best to request a review from him on the merge proposal so that we can track it.
<gmb> wallyworld_: No problem.
<gmb> wallyworld_: r=me. I've added thumper as a reviewer, too.
* gmb changed the topic of #launchpad-reviews to: On call: gmb || Reviewing: jcsackett || queue: [] ||  This channel is logged:  http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* gmb changed the topic of #launchpad-reviews to: On call: gmb || Reviewing: henninge || queue: [] ||  This channel is logged:  http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<henninge> gmb: thanks for picking that up ;)
<gmb> :)
<gmb> henninge: r=me with a couple of nitpicks.
* gmb changed the topic of #launchpad-reviews to: On call: gmb || Reviewing: - || queue: [] ||  This channel is logged:  http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<henninge> gmb: thanks
<gmb> np
<henninge> gmb: oops, I left the changes in migrate_current_flag.py in there by accident. That was just meant for debugging something else.
* leonardr changed the topic of #launchpad-reviews to: On call: gmb, leonardr || Reviewing: - || queue: [] ||  This channel is logged:  http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<gmb> henninge: Heh, okay.
<bac> hi gmb can i have a review of a small branch?
<gmb> bac: Sure
<bac> gmb: https://code.edge.launchpad.net/~bac/launchpad/bug-674897/+merge/42238
<bac> thanks
<gmb> bac: r=me
<bac> thanks graham
<jcsackett> anyone available to do a quick ui review on https://code.edge.launchpad.net/~jcsackett/launchpad/launchpad-ids-270310/+merge/42190?
<salgado> jcsackett, can it be in about 1h, after my lunch?
<jcsackett> salgado: sure, that sounds great.
<jcsackett> salgado: when you get to it, screenshot is in Demo in the proposal write-up.
<salgado> cool, will remember that
<adeuring> gmb, leonardr: could you please review this MP: https://code.edge.launchpad.net/~adeuring/launchpad/bug-596944-model/+merge/42253 ?
<leonardr> adeuring, sure
<adeuring> thanks!
<leonardr> adeuring: out of curiosity, do projects want to turn this off because our detection algorithm doesn't work for them, or because 'duplicate' bugs are helpful to them?
<adeuring> leonardr: many kernel or X related bugs are harware related. And  you have one symptom caused by drivers for different hardware
<leonardr> ok
<adeuring> leonardr: this lets people say "yes, that's my bug", when it fact isn't
<leonardr> adeuring, r=me
<adeuring> leonardr: thanks!
<jcsackett> salgado: sinzui grabbed the ui review i needed, so you're off the hook.
<sinzui> oh, sorry
<sinzui> I am still getting my head together after a week off
<salgado> jcsackett, yeah, I noticed that when I got back from lunch
<jcsackett> salgado: cool.
* henninge_ changed the topic of #launchpad-reviews to: On call: gmb, leonardr || Reviewing: - || queue: [henninge] ||  This channel is logged:  http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* henninge_ changed the topic of #launchpad-reviews to: On call: leonardr || Reviewing: - || queue: [henninge] ||  This channel is logged:  http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<henninge> Hi leonardr!
<henninge> Would be great if you could review my branch, please. ;)
<henninge> https://code.launchpad.net/~henninge/launchpad/db-devel-bug-611668-filtermethods-2/+merge/42293
<leonardr> henninge: sure, if i can do it in the next 20 minutes...
<henninge> have a look.
<henninge> leonardr: it's oversized but has a lot of repetetive changes in it.
<leonardr> henninge: describe to me the new translation model you mention in the mp?
<henninge> translations used to be flagged as "current" and/or "imported" and we had different sets for a project and its linked source package.
<henninge> the basis for the new model is that translations are shared between a project and its linked Ubuntu package.
<henninge> so the meaning for the two flags changed and they were renamed.
<henninge> They are now "is_current_ubuntu" and "is_current_upstream"
<henninge> which indicate if a particular translation is used by both the project and the Ubuntu packaged or if the two translations differ.
<henninge> scratch that second half
<henninge> ... or if it is just used on one side.
<henninge> so a lot of the transition is to make code "side aware", that is they have a concept on "this side" and "the other side".
<henninge> meaning "upstream" and "Ubuntu" or vice versa, depending on wether you look at the translations for a project or for an Ubuntu package.
<henninge> leonardr: can you follow ... ? ;-)
<leonardr> yeah, i think so
<henninge> The concept of "imported" is gone.
<henninge> "imported translation", I should say
<henninge> and has been replaced by "translation on the other side"
<leonardr> henninge: it looks like in pofile.py you have a database field name obtained from a utility. is that ok? is that field name hard-coded in different utilities?
<leonardr> or, rather, not from a utility but from some other data model object
<henninge> the SideTraits
<leonardr> is it hard-coded in different subclasses of SideTraits?
<henninge> exactly
<henninge> there are only two subclasses
<henninge> "upstream" and "ubuntu"
<leonardr> ok
<henninge> and the flag names "is_current_ubuntu" and "is_current_upstream" are hardcoded in those classes.
<henninge> (hence no quoting, if that is what you were wondering ;)
<leonardr> henninge: why did makeTranslationMessage become makeCurrentTranslationMessage? just to avoid ambiguity?
<leonardr> i saw that happened in the earlier branch too
<leonardr> when do you pass in diverged=True and when not? what's going on in the different methods starting at line 212?
<leonardr> (sorry for not digging in myself, but it'll go faster if you just answer my stupid questions)
<henninge> that's ok
<henninge> part of the change in this feature is that we got rid of an old method called "updateTranslation" which had grown out of proportion and is deeply rooted in the old model.
<henninge> it has been replaced by a couple of light-weight methods.
<henninge> makeCurrentTranslationMessage uses those new method, whereas makeTranslationMessage uses updateTranslation to create translation messages.
<leonardr> henninge: what's the difference between a diverged translation and a changed translation?
<henninge> the difference is between "diverged" and "shared"
<henninge> "shared" translations are used across productseries or distroseries respectively.
<henninge> "diverged" translations are pinned to a specific series and take precedence over shared ones.
 * henninge looks at line 212
<leonardr> henninge: i still don't understand what a 'changed' translation is
<henninge> ah!
<henninge> Changed is really an old term
<leonardr> how come you usually change makeTranslationMessage to makeCurrentTranslationMessage, but on line 758 you change it to makeSuggestion? Is a suggestion a type of translation message, like a user-contributed one?
<henninge> leonardr: exactly. It has no "is_current" flag set.
<henninge> unprivileged uses can enter suggestions which can then be accepted by translation reviewers.
<leonardr> henninge: StatistcsFiltersTestScenario should be Statistics...
<henninge> which results in setting the is_current* flag
<henninge> oh, right
<leonardr> is this a class you intend to fill out? it doesn't do anything right now
<henninge>  StatistcsFiltersTestScenario?
<henninge> it does.
<henninge> It is used by TestUpstreamFilters and TestUbuntuFilters to make a test case.
<henninge> They are all based on StatisticsTestScenario which has the actual tests.
<henninge> the tests are all run from both sides "Upstream" and "Ubuntu"
<henninge> leonardr: is that what you meant?
<henninge> I still meant to explain about "changed"
<leonardr> ok, i see what you mean. yes, explain about changed
<henninge> It used to be that a translation was "imported" from a file, usually what the project had published upstream.
<henninge> so it was imported with both "is_current" and "is_imported" set.
<henninge> Now, if somebody went and changed that translation, effectively adding a new translation, the flags would be split between the two.
<henninge> One is actually used, so "is_current", the other still shows how it was imported, so "is_imported"
<henninge> The "ChangedInUbuntu" filter showed all strings for which a translation different from the imported one exists.
<henninge> Since the "imported" concept has been replaced by the "upstream/ubuntu" concept, the Filter now shows where translations differ between the project and its linked Ubuntu package.
<henninge> that's why I changed the name to "DifferentTranslations"
<henninge> leonardr: is that clearer now?
<leonardr> henninge: i believe i was confused by the code starting on line 624, where you use 'changed' in the sample data
<henninge> I did?
<leonardr> yeah, line 627, "changed translation"
<leonardr> i don't really understand why it was "changed" and not "diverged", except that you didn't specify diverged=true
<henninge> leonardr: look at the previous all to makeCurrentTranslation
<henninge> line 615
<henninge> it specifies "current_other=True"
<henninge> which means the translations is created with both flags set.
<leonardr> is_current and is_imported
<leonardr> ok
<leonardr> if you set diverged=True then is_current is false and is_imported is true?
<henninge> well, is_current_ubuntu and is_current_upstream nowaday
<leonardr> and if you set nothing, what happens?
<henninge> no flags == suggestion
<henninge> "diverged" is not indicated by the flags but by another propety of the translation that links this translation to a specific translation template which in turn is linked to a specific series
<henninge> if that property ("potemplate" btw) is None, the message is "shared".
<leonardr> henninge: i absolutely must go, sorry. i can take another look when i come back (probably in about 2 hours), or you can find someone else, or we can take it up tomorrow
<leonardr> i think i'm pretty close to approving this
<henninge> I may or may not be around in two hours.
<leonardr> ok, i'll ping you
<henninge> thanks for taking your time to think into this
<henninge> yes, please try.
* leonardr changed the topic of #launchpad-reviews to: On call: - || Reviewing: - || queue: [henninge] ||  This channel is logged:  http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
#launchpad-reviews 2010-12-01
* jtv changed the topic of #launchpad-reviews to: On call: - || Reviewing: - || queue: [henninge, jtv] ||  This channel is logged:  http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<StevenK> wallyworld_: Do you need me to look at those MPs for you?
<wallyworld_> StevenK: yes please
<StevenK> wallyworld_: In _test_recipebuild_listing, why are you removing the security proxy from naked_recipebuild.distribution ?
<StevenK> And if it's called _test_... doesn't that mean it isn't called at all?
<wallyworld_> StevenK: from memory, if i didn't do that i got some sort of access exception
<wallyworld_> StevenK: let me switch to that branch to have a look
* jtv changed the topic of #launchpad-reviews to: On call: jtv || Reviewing: henninge || queue: [jtv] ||  This channel is logged:  http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<wallyworld_> StevenK: it's called _test because that method is internal and called by other real test methods which first do some setup and then invoke the _test method
<wallyworld_> StevenK: the real test methods are test_recipebuild_listing_anonymous and test_recipebuild_listing_with_user
<StevenK> Ah, that's fine then
<wallyworld_> StevenK: cool. took me a second to remember what i had done :-)
<StevenK> The only other thing I can think of is different indentation for function definitions, but I can't remember rightly
<StevenK> wallyworld_: You have one Approve and one Needs Fixing, enjoy
<wallyworld_> StevenK: thanks!
<wallyworld_> StevenK: with the imports, i had then all split, one per line, but i'm almost certain i was told by someone else that if it fits on one line, leave it like that. but i prefer the multiline approach myself and hence agree with you
<StevenK> Odd
<wallyworld_> StevenK: i'll follow up and see if i misunderstood what i was being told :-)
* jtv changed the topic of #launchpad-reviews to: On call: jtv || Reviewing: abentley || queue: [jtv] ||  This channel is logged:  http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jtv> StevenK: want another review to practice on?  We're both on call I suppose but I can't very well review my own.  :)
<jtv> Also, if StevenK reviews my branch, I'll need a mentor!
<jtv> Anyone else reviewing?
<StevenK> jtv: I can have a look
<jtv> StevenK: thanksâthere should be some things you can shoot at.
<StevenK> jtv: Done
<jtv> thanks!
* jtv changed the topic of #launchpad-reviews to: On call: jtv || Reviewing: - || queue: [jtv] ||  This channel is logged:  http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jtv> StevenK: can you live with the compromise I propose in my MP?
<jtv> (Just submitted)
<StevenK> jtv: Sounds good, Make It So
<jtv> Thanks.  And I thought "make lint" used to warn about dead variablesâ¦
<jtv> StevenK: the diff has updated.  Enough to change your vote?
<StevenK> jtv: Done
<jtv> thanks again
<StevenK> jtv: Care to mentor my review of https://code.launchpad.net/~gary/launchpad/bug683115/+merge/42322 ?
<jtv> heh, I was just looking at that one
* jtv changed the topic of #launchpad-reviews to: On call: jtv || Reviewing: - || queue: [jtv*] ||  This channel is logged:  http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jtv> StevenK: (I wonder if I could mentor your review of my branch :-)
<StevenK> jtv: Care to review https://code.edge.launchpad.net/~stevenk/launchpad/db-spph-ancestry/+merge/41943 ?
<jtv> StevenK: of course.  I may be a bit slow though, as today is do-or-die day for a year-long refactoring project.
<jtv> StevenK: you have my code vote
* jelmer changed the topic of #launchpad-reviews to: On call: jtv, jelmer  || Reviewing: - || queue: [jtv*] ||  This channel is logged:  http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jelmer> jtv: are those branches still pending?
<jtv> jelmer: there's only one that I can see, but yes it is!
* jelmer changed the topic of #launchpad-reviews to: On call: jtv, jelmer || Reviewing: - || queue: []  ||  This channel is logged:  http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jtv> We emptied out the queue.  Might as well get out while we're on top.
* jtv 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
<salgado> jelmer, can you review a trivial one for me? (https://code.launchpad.net/~salgado/launchpad/bug-683106/+merge/42351)
<jelmer> salgado: There's a (trivial) conflict in that branch, can you fix that?
<salgado> jelmer, doing so now. :)
<jelmer> salgado: Are you sure there's no way to use launchpadlib anonymous access at the moment?
<salgado> jelmer, there is
<salgado> it works out-of-the-box for everything but collections
<salgado> in the case of collections, lazr.restful does a hard-coded check for launchpad.View, so for collections of objects that you want to expose to anonymous users, one must define launchpad.View adapters for those objects
<jelmer> salgado: Sorry, I was a bit vague. I mean the launchpadlib_for_anonymous convenience function
<jelmer> I thought we already had one, and was wondering if you'd checked for that.
<salgado> oh, there's one helper which uses WebserviceHTTPCaller
<salgado> but that's only available for pagetests, it doesn't allow you to specify the version of the webservice you want to use and doesn't provide real launchpadlib-like objects (you have to deal with json directly)
<jelmer> what about Launchpad.login_anonymously() ?
<jelmer> hmm, that probably uses the local users standard directory for launchpad credentials
<salgado> yes, it would add the new credential to the cache, I think
<jelmer> It's a pity that code has to be duplicated. It'd be great if we could override the default directory launchpadlib uses more easily.
<jelmer> salgado: Anyway, that's beyond the scope of this branch. r=me
<leonardr> salgado: i can rationalize this if you file a bug for it, but i probably can't get to it anytime soon
<salgado> jelmer, I kind of agree with you there, but it's also arguable that having this extra helper together with the others will make this functionality more discoverable, and it's not that much code we're duplicating anyway
<jelmer> salgado: It seems reasonable to have specific helpers in the Launchpad tree (e.g. with a service_root that defaults to "http://api.launchpad.dev/") but atm launchpadlib_for() and launchpadlib_for_anonymous() duplicate code that's in launchpadlib. It'd be nice if they could just be trivial wrappers around the launchpadlib code.
<salgado> indeed
<henninge> jtv: I saved recife the roll-back diff (just recife) and the inverse diff 10008..10007 from db-stable to files and compared the diffs.
<henninge> I get this: http://paste.ubuntu.com/538642/
<henninge> Any idea why those lines differ?
<henninge> ah, I just realized that they are context
<henninge> jtv: approved
<bac> #startmeeting
<bac> doh
<salgado> leonardr, I didn't understand your comment earlier and you left right after you made it so I didn't have a chance to say so.  care to clarify?
<leonardr> salgado: sorry, i had to restart
<leonardr> as your reviewer said, the idea of a special anonymous version of launchpad_for is a little hacky. i was offering to clean that up for you later if oyu filed a bug
<salgado> leonardr, oh, to refactor Launchpad.login_anonymously() so that we can use that and avoid the extra helper?
* EdwinGrubbs changed the topic of #launchpad-reviews to: On call: jelmer, Edwin || Reviewing: - || queue: []  ||  This channel is logged:  http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<leonardr> salgado: yeah, or let you pass in the NOBODY user to launchpad_for or something
* benji changed the topic of #launchpad-reviews to: On call: jelmer, Edwin, benji || Reviewing: - || queue: []  ||  This channel is logged:  http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jelmer> hi Benji, Edwin
<EdwinGrubbs> jelmer: hi
<benji> EdwinGrubbs: I'd like to do some more mentored reviews today if we have any takers.
<EdwinGrubbs> benji: surprisingly, I don't see any merge proposals that we can take from https://code.launchpad.net/launchpad/+activereviews
<benji> yep, it seems unusually quiet
<salgado> leonardr, bug 683748
<_mup_> Bug #683748: Get rid of launchpadlib_for_anonymous <Launchpad Foundations:New> <https://launchpad.net/bugs/683748>
<jcsackett> benji: i can solve your quiet issue. here's an MP i could use some eyes on: https://code.edge.launchpad.net/~jcsackett/launchpad/anonymous-api-access-emails-681815/+merge/42309
* jcsackett changed the topic of #launchpad-reviews to: On call: jelmer, Edwin, benji || Reviewing: - || queue: [jcsackett]  ||  This channel is logged:  http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jcsackett> naturally i do this at lunch time for maximum inconvenience. :-P
<leonardr> salgado: thanks
<benji> jcsackett: I should be able to do a (mentored) review for you
<benji> jcsackett: I assume this is what you want reviewed? https://code.launchpad.net/~jcsackett/launchpad/anonymous-api-access-emails-681815/+merge/42309
<jcsackett> benji: yup, posted the link earlier.
<jcsackett> mentored is fine. i'm in the same boat, so i'm happy to give you mentoring fodder. :-)
<benji> :)
* benji changed the topic of #launchpad-reviews to: On call: jelmer, Edwin, benji || Reviewing: jcsackett || queue: []  ||  This channel is logged:  http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<benji> jcsackett: ok, you have my approval; hopefully EdwinGrubbs will be able to give his seal of approval too
<jcsackett> benji: cool, thanks!
<benji> (I only had a small and non-binding comment about the long "want" lines)
 * EdwinGrubbs looks at it
<EdwinGrubbs> jcsackett: when I ran your test without the fix to checkUnauthenticated(), it still passed. Does that happen for you?
<jcsackett> EdwinGrubbs: yeah, i just noticed that in running some tests while preparing for land.
<jcsackett> EdwinGrubbs: it looks like the webservice caller isn't getting emails regardless of how you go about it.
<jcsackett> EdwinGrubbs: i'm going to put the MP to WIP and will ping you when i have it sorted.
 * jcsackett sighs.
<jcsackett> EdwinGrubbs: i've pushed up changes, and added a comment explaining something i've discovered.
<jcsackett> it seems the security hole this patches doesn't exist in testruns--an interaction against qastaging that shows the hole doesn't show the hole against dev.
<jcsackett> at least, that's how it appears. in any event, i still was able to improve what the test was meant to show, and set it up so that it would catch any 404 errors we were worried about this change introducing.
<EdwinGrubbs> jcsackett: have you asked leonardr?
<leonardr> jcsackett, what's the problem?
<jcsackett> leonardr: i was working on a patch to deal with this bug: https://bugs.launchpad.net/launchpad-registry/+bug/681815
<jcsackett> i can't create circumstances to make this bug happen against dev in unittests, though the scripts attached to the bug do demonstrate it to exist on qastaging.
<jcsackett> EdwinGrubbs: you can take me out of the reviewing thing. after speaking with leonardr and creating a unit test that uses launchpadlib, i still can't create a test case that demonstrates this problem as expected. i'm going to take another look at it tomorrow.
* EdwinGrubbs changed the topic of #launchpad-reviews to: On call: jelmer, Edwin, benji || Reviewing: - || queue: []  ||  This channel is logged:  http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<EdwinGrubbs>  jcsackett: ok, good luck
<jcsackett> EdwinGrubbs: i'm hoping fresh eyes tomorrow will be sufficient luck. but thanks. :-)
<james_w> hi, could someone take a look at https://code.edge.launchpad.net/~james-w/launchpad/export-specification-bug-links/+merge/42426 ?
#launchpad-reviews 2010-12-02
* 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
<stub> StevenK: How does the new ancestor column interact with supersededby ?
<stub> We will need an index if we want to know what our child records are. Or if we ever delete records - I guess we should add it now or we might be sorry.
<StevenK> stub: ancestor and supersededby are seperate -- we use ancestor to find out the prior version
<StevenK> stub: Thanks for the review, I'll switch it to 34 and add the index
<stub> Sure. I'm fine with parent too if jml insists. I think it is a better name, as it is the direct ancestor, but with bow to consistency with the rest of soyuz.
<stub> c/with/will/
<StevenK> stub: Yeah, I'm more partial to ancestor, but I was going to mention it to Julian to get his thoughts too.
* 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
<jtv> Nobody on call today?  allenap, rockstar?
<allenap> jtv: Oh yes. I thought it was Wednesday today for no good reason.
* allenap changed the topic of #launchpad-reviews to: On call: allenap || Reviewing: - || queue: [jtv] ||  This channel is logged:  http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jtv> I find that days rarely change for no good reason.
<jtv> Sometimes I hear people of a certain religion claim that certain days are unexpectedly Sunday, but that sounds to me like a bit of a kludge to cover up some design problem or other.
<jtv> A bit like saying "it's monotheistic since the three main deities are really the same one in different dimensions"
<allenap> jtv: Now that we're on the same team I'm really going to have to make an effort to understand you :)
<jtv> That's like the old battle between armour and armament.  Sorry, the armament always wins out.
<jtv> As soon as you think you understand what I'm on about, you'll find I'll have moved on to new spheres of madness.
<jtv> It's sort of a mission, like
<jtv> But this once I'll explain.  I exploited a scoping ambiguity in your statement that you "thought it was Wednesday today for no good reason."
<jtv> You probably thought it for no good reason, but I went off on an inane tangent about today suddenly deciding for no good reason to be Wednesday.
<jtv> There is some system to this.  Sometimes.
<jtv> Dear internet: I know, I know, but that was no reason to disconnect me.  Thank you.
<gmb> allenap: Is it wrong that I understand everything jtv just said?
<jtv> Very.
<jtv> I'd be worried.
<jtv> Also, it means I'm not working hard enough at this.
<allenap> What?
<allenap> I'm looking forward to longer discussions like this at MegaThunderLightingFrogsToadsDome in January.
* jcsackett changed the topic of #launchpad-reviews to: On call: allenap, jcsackett || Reviewing: -, - || queue: [jtv] ||  This channel is logged:  http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jtv> allenap: gmb seems to follow what I said.  Unless he's bluffing, that means I'm not all that mad after all.  Which means I need to work harder at it.
<allenap> jtv: Okay :) I'll just go and review your branch, shall I?
<jtv> Yes, that may be the easiest way to get rid of me thanks.  :-)
<jcsackett> ...that was the oddest thread to try and follow in irc in my adult career.
* allenap changed the topic of #launchpad-reviews to: On call: allenap, jcsackett || Reviewing: jtv, - || queue: [] ||  This channel is logged:  http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<allenap> jcsackett: In your career as an adult, or ... no, no, that's jtv territory ;)
<jcsackett> allenap: :heheh. please, please don't embark on that. :-P
<jtv> allenap: I'm not _that_ much older.
<jtv> Anyway, as future teammates it's important that we all understand each other, is it not?  :-)
<jtv> Ohhhh, "adult."  I get it.
<jcsackett> there was something to get?
<jtv> I think.  I'm trying to follow allenap's sometimes naughty lines of reasoning here.
<jtv> Tsk, tsk.
<allenap> jtv: I wasn't being naughty!
 * jtv gives allenap a look of doubt and suspicion
<jcsackett> oh, we're going to have so much fun at standups.
<jtv> Well they call it standup for a reason.
 * jtv got himself some DVDs of The Best of Jasper Carrot and Rowan Atkinson's standup shows last weekend.
<jcsackett> oh dar.
<jtv> Not to mention the complete Blackadder and Fawlty Towers.
<jcsackett> s/dar/dear/
<jtv> If you need me to get serious, just say the word.  I also got the director's cut of Blade Runner.
<jtv> It was a charity fair so probably not polite to haggle, but the prices were too good to resist anyway.
<jcsackett> so the word is "replicant"?
<jtv> Not on Earth it isn't.  Illegal.
 * jcsackett laughs.
 * jtv trundles off for supplies.
<benji> jtv: I have a small bug-fix branch for you when you get a second (https://code.launchpad.net/~benji/launchpadlib/fix-nested-p-tags/+merge/42489)
<benji> jtv: oh, well, you're not reviewing, so you can ignore that and continue watching That Mitchell And Webb Look
* benji changed the topic of #launchpad-reviews to: On call: allenap, jcsackett || Reviewing: jtv, - || queue: [benji] ||  This channel is logged:  http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<benji> allenap and jcsackett: I have a small bug-fix branch for you when you get a second (https://code.launchpad.net/~benji/launchpadlib/fix-nested-p-tags/+merge/42489)
<jcsackett> benji: i can take a look at it. it'll require follow up though, since i'm being mentored.
<benji> I'm down with that.
<jcsackett> cool.
* jcsackett changed the topic of #launchpad-reviews to: On call: allenap, jcsackett || Reviewing: jtv, benji || queue: [] ||  This channel is logged:  http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jcsackett> benji: is there anything in launchpadlib to test generation?
<benji> jcsackett: nope; the generation itself is done in the launchpad Makefile; in fact, we intend on moving the XSLT to LP proper at some point
<jcsackett> benji: dig.
<jcsackett> benji: this looks super simple, since there's no way to test it. r=me.
<jtv> benji, jcsackett: I'm back, so could mentor
<jcsackett> jtv: i think sinzui has word on when i've "graduated", so i should probably have him do it. :-P
<benji> jcsackett: yep, it's quite simple (the kind of simple that results from 6 hours of banking one's head against XSL)
<jtv> ah ok
<jcsackett> well yes. xsl is always simple when done, giant pain when figuring out. :-P
<jcsackett> (i back that statement up with my own suffering. :-P)
<jtv> jcsackett: to quote James Clark in a recent presentation: "I certainly hope I won't be using XML 10 years from now.  That would be kind of depressing."  :-)
<jcsackett> jtv: yes. xml has its uses, but i hope we find something better for those uses.
<jcsackett> benji: i've gone ahead and requested another review from sinzui. and i've said his name here enough i'm sure he knows something is up. :-)
<sinzui> I do
<jtv> jcsackett: I heard ASN.1 is pretty good.
<jcsackett> see? irc magic.
<jcsackett> jtv: i'll have to take a look at it.
 * jelmer wonders if jtv is joking or being serious
<jtv> jelmer: if you figure it out, tell me.  I don't know the first thing about ASN.1.
<jelmer> jtv: I would recommend you keep it it that way.
<jcsackett> jelmer: bad, huh?
<jtv> *chuckle*
<jelmer> I'm not sure if it's bad, it's just not my cup of tea.
<benji> jcsackett: just don't say his name three times or you will summon him to your relm.
<jtv> Yet, with strange Ã¦ons, even COBOL may dieâ¦
* allenap changed the topic of #launchpad-reviews to: On call: allenap, jcsackett || Reviewing: -, benji || queue: [] ||  This channel is logged:  http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* jcsackett changed the topic of #launchpad-reviews to: On call: allenap, jcsackett || Reviewing: -, - || queue: [] ||  This channel is logged:  http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jtv> thanks allenap!
<allenap> jtv: Welcome :)
 * jtv suddenly remembers that South Park quoted that bit of Lovecraft very differently than he remembered from the Iron Maiden album cover.  Have to look that up.
* sinzui changed the topic of #launchpad-reviews to: On call: allenap, jcsackett || Reviewing: -, - || queue: [sinzui] ||  This channel is logged:  http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<sinzui> allenap, can you review https://code.launchpad.net/~sinzui/launchpad/closed-teams-0/+merge/42500
<allenap> sinzui: Sure.
* allenap changed the topic of #launchpad-reviews to: On call: allenap, jcsackett || Reviewing: sinzui, - || queue: [] ||  This channel is logged:  http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* allenap changed the topic of #launchpad-reviews to: On call: allenap, jcsackett || Reviewing: sinzui, - || queue: [abel] ||  This channel is logged:  http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jcsackett> adeuring: i assume the abel in the queue is you and the MP is https://code.launchpad.net/~adeuring/launchpad/bug-596944-browser/+merge/42505?
<adeuring> jcsackett: yes
<jcsackett> adeuring: okay, i can grab that.
<adeuring> jcsackett: cool, thanks!
* jcsackett changed the topic of #launchpad-reviews to: On call: allenap, jcsackett || Reviewing: sinzui, adeuring || queue: [] ||  This channel is logged:  http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jcsackett> adeuring: high level this is about adding the ability on dsp and products to set it so new bugs can skip the "is this a dupe" step?
<adeuring> jcsackett: yes
<jcsackett> adeuring: nice.
<adeuring> jcsackett: see the linked bug report: dupe searches caused some headaches for the kernel team and the Xorg team
<adeuring> because people thought wrongly that they were affected by exiting bugs which were filed for different hardware
<lifeless> its a bit of a kludge really
<lifeless> if we called out to a web service
<lifeless> it would be more powerful
<lifeless> and let them dup things where the hardware matches even if the description does, and exclude on hardware too.
<adeuring> lifeless: right, but that would require some work on better intergration of the HWDB with malone.
<adeuring> and there may be other scenarios where the dupe search is not useful
<lifeless> adeuring: not really
<adeuring> how so?
<lifeless> adeuring: lp makes a web service call
<adeuring> ?
<lifeless> the thing we call can use their *existing* mechanism for checking hardware (e.g. tags) on that bug and ther bugs.
<lifeless> no changes to hwdb stuff in lp needed
<lifeless> adeuring: I'm saying if we implemented a callback for this
<lifeless> as for other projects, exactly my point - they can trivially disable dup lookups with a callback that returns [] as the dup list.
<adeuring> lifeless: ah, right, that's an intersting approach
<lifeless> I think what you're doing is fine fpr now - its what they asked for.
<lifeless> I'm simply noting that its a special case of a more general thing of 'let the user determine some policy'
<lifeless> and perhaps the most useful and most general thing we can do here is to let them define it using their own code.
<adeuring> lifeless: so... something like PythonScripts in Zope2?
<jcsackett> adeuring: you've landed multiple branches for this, right? that's why i'm seeing places where the enable_... field you're adding already exists?
<adeuring> jcsackett: yes, there are two other branches (already merged) to the DB patch and for the model code.
<jcsackett> adeuring: okay, thanks.
<lifeless> adeuring: I was thinking an API http[s] endpoint.
<adeuring> lifeless: could you explain a bit?
<adeuring> I mean: how could an API call defined by a project maintainer help when an ordinary LP user files a bug?
<lifeless> user hits the page
* allenap changed the topic of #launchpad-reviews to: On call: allenap, jcsackett || Reviewing: -, adeuring || queue: [] ||  This channel is logged:  http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<lifeless> the page makes an API call
<lifeless> lp server -> project maintainer server
<adeuring> ah, interesting idea!
<lifeless> (or perhaps web client -> project maintainer server)
<lifeless> api result comes back with
<lifeless> some blurb
<lifeless> and candidate dups
<lifeless> blurb might be "We do not permit duplicate bugs because our logs and crashes have misleading similarities for unrelated causes."
<lifeless> oh also
<lifeless> it could come back and say
<lifeless> "This crash was caused by running out of disk space. It is probably not a bug as documented <link>, are you sure you want to file a bug."
<jcsackett> adeuring: r=me. i have requested another review from sinzui as he is mentoring my reviews.
<adeuring> jcsackett: thanks!
<jcsackett> if history is any indication, that won't take long.
<jcsackett> you're welcome. :-)
* jcsackett changed the topic of #launchpad-reviews to: On call: allenap, jcsackett || Reviewing: -, - || queue: [] ||  This channel is logged:  http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* allenap changed the topic of #launchpad-reviews to: On call: jcsackett || Reviewing: - || queue: [] ||  This channel is logged:  http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<allenap> Cheerio.
<jcsackett> bye, allenap.
<jcsackett> EdwinGrubbs: could you take another look at https://code.launchpad.net/~jcsackett/launchpad/anonymous-api-access-emails-681815/+merge/42309 ? i have dealt with the test issue. there's a diff showing the new test stuff in the last comment.
<EdwinGrubbs> jcsackett: sure
<jcsackett> thanks.
<jcsackett> going on a quick lunchbreak, all, just leave links to MPs and your name in the queue if you need a review.
<EdwinGrubbs> jcsackett-lunch: r=me
<adeuring> sinzui: can you have another look at my MP?
<sinzui> yes
<sinzui> adeuring, Your test additions are lovely. Remove the previous additions to lib/lp/bugs/stories/guided-filebug/xx-product-guided-filebug.txt. We want to test something once and test it very well
 * jcsackett makes note that things he thinks are polite difference may in fact be objections to the MP...
<jcsackett> sinzui: is it generally safe to object to stuff that uses stories for testing?
<sinzui> yes
<jcsackett> cool.
<sinzui> A story is a customer acceptance test that explains the role and the goal, and verifies it is accomplished
<sinzui> jcsackett,  1/3 of test time is spent in stories that are not telling us why a feature exists and how the user knows he did it
<jcsackett> sinzui: that makes sense (the goal of the story). and i think you've mentioned the test-length we have being an issue of bad stuff in stories.
<sinzui> stories/doctests often grow too long. There are non-obvious conditions at play in long doctest. Stories use sample data which is often wrong when telling a story
<adeuring> sinzui: I already reverted the story changes
<adeuring> ...and thanks for the review!
<adeuring> gah, just noticed that reverted only part of it...
* jcsackett 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-12-03
<wallyworld> StevenK: want another code review to do?
<StevenK> wallyworld: No? :-)
<wallyworld> StevenK: that's ok. i thought i'd offer it to you first
<StevenK> Actually, let me ask a different question. "Do I have a choice?" :-)
<StevenK> wallyworld: But yes, I'll take a look
<wallyworld> StevenK: of course there's a choice :-) . want wasn't sure if you were still needed to do them as part of the metoring
<wallyworld> StevenK: if you want :-) https://code.edge.launchpad.net/~wallyworld/launchpad/recipe-find-related-branches/+merge/42097
<wallyworld> StevenK: feel free to pass if you want. i won't be offended :-)
<StevenK> wallyworld: You mention a bug in the summary, but you don't link it to the branch? :-)
<StevenK> wallyworld: I'm only surprised you have to kick the security proxy out of the way so much
<wallyworld> StevenK: linked now
<wallyworld> StevenK: what i found was that without removing the proxy, stuff would break when running the tests. can't recall the exact error now
<StevenK> wallyworld: Perhaps this means the zcml should be tweaked instead of telling zope to get out of your way?
<wallyworld> StevenK: yes, perhaps. i can take another look
<StevenK> wallyworld: Anyway, +1'd, but that would be cool
<wallyworld> StevenK: i guess, given in some places we need to removeSecurityProxy, i'm unsure what the guiding principals are as to when to go naked vs tweak zcml
<wallyworld> thanks for the review btw
* 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
<wgrant> adeuring: Can I throw a review your way?
<adeuring> wgrant: sure; I'm currently reviewing an MP from EdwinGrubbs, but i'm nearly finished
* wgrant changed the topic of #launchpad-reviews to: On call: adeuring || Reviewing: - || queue: [wgrant] ||  This channel is logged:  http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<wgrant> https://code.launchpad.net/~wgrant/launchpad/bug-675621-packages-binary-scaling/+merge/42607 is the MP.
* 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
* adeuring changed the topic of #launchpad-reviews to: On call: adeuring || Reviewing: wgrant || queue: [] ||  This channel is logged:  http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
 * adeuring needs to remember that C-Y does not paste stuff from the clipboard in most programs
<adeuring> wgrant: r=me
* 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
<wgrant> adeuring: Thanks!
<wgrant> adeuring: Could you please throw it at EC2?
<adeuring> wgrant: sure
<gmb> adeuring: Howdy. Can I have a review for a fairly trivial JS branch? https://code.edge.launchpad.net/~gmb/launchpad/lazr-wizard-linkage/+merge/42616
<adeuring> gmb: sure
<gmb> Thanks
<adeuring> gmb: r=me
<gmb> adeuring: Thanks.
* 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
<jtv> I was trying to get Stuart to review this, since he told me where to stick my new code, but it seems he's already gone: https://code.launchpad.net/~jtv/launchpad/bug-684669/+merge/42621
<bac> hi adeuring.  much happening today?
<adeuring> bac: well, three reviews so far
<bac> adeuring: hopping!
<bac> jtv: i'll look at it
<jtv> thanks bac
* bac changed the topic of #launchpad-reviews to:  On call: adeuring,bac || Reviewing:  -,jtv || queue: [] ||  This channel is logged:  http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<bac> jtv: are there any tests where you could demonstrate this loggin?
<jtv> bac: I could probably arrange for one.
<jtv> Wasn't sure it was needed, since this will be exercised all the time anyway.
<bac> have you shown it being exercised?
<jtv> bac: hmmâ¦ writing a test for this is harder than I thought.  I demonstrated the debug output manually by running a script with -vvv but that'd be a bit of a splurge for testing something so basic.
<jtv> I don't see any way to inject a fake database connection.  There may be a way to inject a fake logger, but I don't know how.
<jtv> (But FWIW yes I did see it work and it was just the thing I wished I'd had so many times before)
<bac> jtv: ok.  i just wanted to ensure it had been exercised at least a little, to ensure the formatting string gets populated correctly, no syntax errors, etc
* 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
<jtv> bac: a very valid concernâI've had breakage in the past because of silly mistakes in composing exception messages.  Thanks.
<EdwinGrubbs> adeuring: I added another test to the branch you reviewed, and I refactored the tests that you mentioned.
<EdwinGrubbs> https://code.edge.launchpad.net/~edwin-grubbs/launchpad/bug-638924-milestone-page-eager-loading/+merge/42586
<adeuring> EdwinGrubbs: cool -- though I think branch was fine before :) I'll have a look
<adeuring> EdwinGrubbs: still r=me, just one minor nitpick
<EdwinGrubbs> thanks
* adeuring changed the topic of #launchpad-reviews to: On call: bac || 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: - || Reviewing:  - || queue: [] ||  This channel is logged:  http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
