#launchpad-reviews 2009-10-19
<adeuring> henninge: could you please review this branch: https://code.edge.launchpad.net/~adeuring/launchpad/hwdb-class-udev-device-8/+merge/13554 ?
<adeuring> intellectronica: ^^^?
* intellectronica changed the topic of #launchpad-reviews to: on call: intellectronica || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com
<intellectronica> adeuring: yes, looking at it now
<adeuring> intellectronica: thanks!
* henninge changed the topic of #launchpad-reviews to: on call: intellectronica, henninge || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com
<henninge> I am here now ...
<henninge> HI intellectronica! ;)
<intellectronica> hi henninge, happy new week!
* intellectronica changed the topic of #launchpad-reviews to: on call: intellectronica, henninge || reviewing: adeuring, - || queue: [] || This channel is logged: http://irclogs.ubuntu.com
<intellectronica> adeuring: wouldn't it be better to use path expressions to find the nodes you want? less likely to make this sort of mistake
<adeuring> intellectronica: I'm doing this now, I believe. Or do I miss something?
<intellectronica> adeuring: i'm looking at parent.parent.parent.parent and thinking out loud how we can avoid it
<adeuring> intellectronica: that's hard... We simply need the grandparent of a device, in UdevDevice.scsi_controller
<intellectronica> adeuring: the code in general looks fine, but i find it hard to follow, so i'm wondering if there's anything that can be done to improve it
<intellectronica> yes
<adeuring> intellectronica: yes, I know that it is not that simple. That's the reason for the sometimes long-winded comments ;)
<intellectronica> adeuring: r=me. i find the code quite hard to wrap my head around, but i guess that's mostly just because it's a complex document that's being parsed. i couldn't think of any way to improve on that with small changes. and yes, the commentary does help a lot
* intellectronica changed the topic of #launchpad-reviews to: on call: intellectronica, henninge || reviewing: -, - || queue: [] || This channel is logged: http://irclogs.ubuntu.com
<intellectronica> henninge: would you like me to review your branch?
<adeuring> intellectronica: thanks!
<jtv> henninge: your fix looks good.  The comment for the test is lacking the word "entry" after "an," and the test method is confusingly named; I'd suggest test_approve_only_if_needs_review.  Ceterum censeo r=me.
<henninge> intellectronica: thanks but I was impatient and asked jtv. He is familiar with the problem, anyway.
<jtv> henninge: also, w.r.t. your test plan, not that it matters much but I don't think the date shown in the UI will be updated.  AFAIK we have a bug open for that.
<intellectronica> cool bananas
<henninge> jtv: thank you
<jml> al-maisan, I'll look at that branch now.
<al-maisan> jml: thanks
<henninge> jtv: http://paste.ubuntu.com/296752/
<jtv> henninge: fine
* intellectronica changed the topic of #launchpad-reviews to: on call: intellectronica, henninge || reviewing: edwin, - || queue: [] || This channel is logged: http://irclogs.ubuntu.com
<adeuring> intellectronica, henninge: fancy to reveiw a sequel of my previous branch: https://code.edge.launchpad.net/~adeuring/launchpad/hwdb-class-udev-device-9/+merge/13562 (~650 lines, but ca 40%...50% is the definition of test data)
<intellectronica> adeuring: i'm reviewing a branch of edwin's i picked off the reviews queue, but can pick your branch when i finish, it won't be long
<adeuring> intellectronica: thanks!
* intellectronica changed the topic of #launchpad-reviews to: on call: intellectronica, henninge || reviewing: edwin, - || queue: [abel] || This channel is logged: http://irclogs.ubuntu.com
* intellectronica changed the topic of #launchpad-reviews to: on call: intellectronica, henninge || reviewing: abel, - || queue: [] || This channel is logged: http://irclogs.ubuntu.com
<intellectronica> maaan, those hwdb branches are starting to feel like soyuz to me. i have to work so much out just in order to be able to understand what they do
<bigjools> lies
<intellectronica> adeuring: r=me
* intellectronica changed the topic of #launchpad-reviews to: on call: intellectronica, henninge || reviewing: -, - || queue: [] || This channel is logged: http://irclogs.ubuntu.com
<adeuring> intellectronica: thanks!
* abentley changed the topic of #launchpad-reviews to: on call: intellectronica, henninge,abentley || reviewing: -, - || queue: [] || This channel is logged: http://irclogs.ubuntu.com
<adeuring> intellectronica: fancy another sequel of this HWDB submissions stuff (a bit shorter this time: 177 lines diff)? 
<intellectronica> adeuring: sure
<adeuring> intellectronica: thanks!
<adeuring> https://code.edge.launchpad.net/~adeuring/launchpad/hwdb-class-udev-device-10/+merge/13567
<allenap> abentley: Can you review a short lpbuildbot branch for me please? There is some bzrlib stuff in there so I'd like you to check my assumptions. https://code.edge.launchpad.net/~allenap/lpbuildbot/explicitly-disconnect-transports-bug-419421/+merge/13575
<abentley> allenap: sure.
<allenap> Thanks.
* abentley changed the topic of #launchpad-reviews to: on call: intellectronica, henninge,abentley || reviewing: -, -, allenap || queue: [] || This channel is logged: http://irclogs.ubuntu.com
<abentley> allenap: The transports variable isn't doing a lot for you.  I'm not sure what disconnecting two transports with the same underlying connection does.
<allenap> abentley: I don't follow.
<abentley> allenap: transports is an optional parameter to Branch.open_containing.
<abentley> allenap: It's only valuable if you pass it into another call later.
<allenap> abentley: I was relying on the fact that reusable transports get appended to it, so I could disconnect them.
<abentley> allenap: I don't think we provide a strong guarantee that passing it into open_containing will append all transports that were opened in the process of opening the branch.
<allenap> abentley: Ah, okay.
<abentley> allenap: Are you sure you need to disconnect all the transports?
<allenap> abentley: Well, no, but from the bug report it seems that ssh processes are not getting reaped all the time. I'm assuming that they are related to bzr operations. If that's a correct assumption, then I want to try explicitly reaping them.
<allenap> abentley: It may be that the assumption is wrong, but I want to try this because I don't have any better ideas :)
<abentley> allenap: Right, but are you sure you need to disconnection all the *transports*?  Because I would have thought you only needed to disconnect all the *connections*.
<allenap> abentley: I didn't understand the distinction before. All the connections I would say. Calling transport.disconnect() closes the connection, so I'm relying on that.
<abentley> allenap: Right, so I think you can just do branch.bzrdir.root_transport.disconnect()
<allenap> abentley: Okay. I assume that will basically disconnect everything that bzrlib has done from everything. I think that'll be fine for our buildbot configuration as it stands, but it's bad behaviour if we ever add more pollers to the configuration.
<abentley> allenap: No, that will just disconnect the ssh connection related to the branch you just opened.
<allenap> abentley: Ah, I read that wrong! I read bzrlib.bzrdir.root_transport.disconnect, and I assumed root_transport was a module global.
<allenap> abentley: Okay, thanks for that, I'll use that approach.
<abentley> allenap: Cool.
<abentley> allenap: Also, you should pull the "branch =" statement out of the try block.  If that were to fail, you wouldn't want to go to the finally block.
<allenap> abentley: Yeah, agreed.
<abentley> I can't say whether this approach will actually solve the zombie problem, but it looks fine otherwise.
<jml> al-maisan, hello
<al-maisan> hello jml , just read your email
<al-maisan> thanks very much for clarifying things
<jml> al-maisan, my pleasure.
<al-maisan> so, the branch-sourcepackage relation is 1-on-1 .. but
<jml> al-maisan, there are two kinds of relationships between a branch and a sourcepackage
<al-maisan> aha
<sinzui> intellectronica: henninge, abentley: Can one of you take https://code.edge.launchpad.net/~sinzui/launchpad/package-link-validation-1/+merge/13578 ?
<jml> al-maisan, a branch can *belong* to a source package
* sinzui changed the topic of #launchpad-reviews to: on call: intellectronica, henninge,abentley || reviewing: -, -, allenap || queue: [sinzui] || This channel is logged: http://irclogs.ubuntu.com
<jml> al-maisan, (or a product, or be a junk branch)
<al-maisan> yes...
<jml> al-maisan, alternatively, a branch can be the *official branch* for a (sourcepackage, pocket)
<intellectronica> sinzui: sure, i'll review it
<jml> the two concepts are independent.
* intellectronica changed the topic of #launchpad-reviews to: on call: intellectronica, henninge,abentley || reviewing: sinzui, -, allenap || queue: [] || This channel is logged: http://irclogs.ubuntu.com
<jml> al-maisan, furthermore, a branch can be the official branch for more than one (sourcepackage, pocket) ISTR
<al-maisan> jml: can be the can be the *official branch* for a number of (sourcepackage, pocket) items?
<al-maisan> ah
<abentley> allenap: AIUI, this method is called in a thread from a long-running process.  Correct?
<allenap> abentley: Yes.
<abentley> allenap: Have you considered passing a transports parameter into it from the long-running process?
<jml> al-maisan, this is deliberate. I'm beginning to think it's also misguided.
<al-maisan> jml: is it correct to assume that in such a (sourcepackage, pocket) collection a sourcepackage.distroseries only appears once?
<intellectronica> sinzui: i don't understand the way you structured that conditional around line 32 of the diff
<abentley> allenap: That would presumably limit you to one SSH connection per long-running process.
<jml> al-maisan, I'm afraid not.
<intellectronica> sinzui: why do you need an extra if?
 * sinzui should have annotated the cover letter
<al-maisan> jml: how's that beneficial?
<allenap> abentley: That's certainly a possibility, quite an easy one to do. The transports will go for no more than 10 minutes without being used. Is that likely to cause problems? What happens if bazaar.launchpad.net goes down? Will bzrlib cope?
<jml> al-maisan, that's all you have to work with in terms of assumptions: http://pastebin.ubuntu.com/296938/
<al-maisan> ah
<jml> al-maisan, I'm not sure it _is_ beneficial :)
<al-maisan> aha :)
<jml> al-maisan, but the code you write here has to work with our current model
<al-maisan> jml: that's understod.
<abentley> allenap: I'm not sure how bzrlib reacts if the host goes away while a connection is open.
<sinzui> intellectronica: There are two kinds of errors the user can get when setting a sourcepackage for a series: the first rule that exists is that you cannot reuse the the exact combination of distroseries, sourcepackage, and productseries.
<al-maisan> jml: hmm .. this looks a bit funny:  UNIQUE, btree (distroseries, sourcepackagename, pocket)
<abentley> allenap: I'm more worried about the fact that transports weren't designed to be thread-safe, so you'd have to take care of that yourself.
<sinzui> intellectronica: The error gets its own message because the problem is locale to the series being edits
<jml> al-maisan, that means...
<intellectronica> sinzui: but why `message="..."\n if message:...`? you don't need that if
<jml> al-maisan, I'm on a call I want to keep short. you'll have my full attention in a minute
<al-maisan> jml: sure
<sinzui> intellectronica:sorry. I have the plague and I and thinking is a challenge
<allenap> abentley: I think gary_poster must have had that problem because the custom BzrPoller we use overrides buildbot's to avoid concurrency already.
<al-maisan> jml: well, if I am to check whether we can upload to a pocket and a number of (distroseries, pocket) combination can appear in that storm result set, the question is which one should I check..
<sinzui> intellectronica: That is crack I should have removed when I refacteded the branch between commits
<allenap> abentley: Meaning: gary_poster wrote the custom poller afaict.
<gary_poster> riht
<gary_poster> right
<al-maisan> maybe all of them and be satisfied when I hit the first one that "passes"
<intellectronica> sinzui: also, when you have a multiline conditional (like around line 38), it's nice if you can add a comment immediately before the then clause, so that it's clearer where the condition stops and the statements to execute begin
<intellectronica> sinzui: ditto near line 46
<al-maisan> jml: I guess I am a bit confused since there can be one "current" source package release in a distro series and I assumed that an official branch would be linked to that ..
<sinzui> intellectronica: okay
<sinzui> I can add comments
<intellectronica> cool
<abentley> allenap: I don't think Launchpad PQM Bot should be subscribed to code review email.
<allenap> abentley: No, that seems odd.
<intellectronica> sinzui: it's a bit strange when you set next_url to the value of cancel_url, becuase even though it's the correct url, it implies that you're canceling
<sinzui> indeed
<intellectronica> sinzui: i would add another property, even though it's inefficient, just for readability
<allenap> abentley: I don't have the power to fix that. You?
<sinzui> I can define next_url and and set cancel_url from it.
<abentley> allenap: Yes.  I've left it subscribed, so it can see the branch, but with no email options set.
<intellectronica> sinzui: yeah, i think that's the best solution
<allenap> abentley: The code that calls getRawChanges catches all exceptions, so wouldn't crash if bzrlib went awry due to a failed transport being passed in. But I wonder if (a) keeping those transports around would cause problems on all subsequent runs (so requiring a restart of buildbot), or (b) if we emptied the list of transports after an exception would we leak processes again?
<intellectronica> sinzui: i'm not sure about the improved description for the packaging field. it's a lot nicer, but i wonder if users will find the concrete examples helpful. i, for example, have no idea what cadaver and libneon do, so mentioning them only serves as a distraction
<intellectronica> i know what apache is, though sometimes i wish i didn't
<sinzui> intellectronica: yes. I struggled with that example
<sinzui> intellectronica: INCLUDE is a rare condition. upstreams rarely need to know about that
<abentley> I don't think a is true.  Transports are meant to be reused.  b might be true.
<intellectronica> sinzui: i would cut right after the question. i think it's formed clearly enough that users will be able to rely on it alone to understand
<jml> al-maisan, back. really sorry about that. team standup.
<sinzui> oh, good. I was tempted to do that, but thought saying more would help
<jml> al-maisan,  UNIQUE, btree (distroseries, sourcepackagename, pocket) just means that each SuiteSourcePackage has exactly one official branch.
<jml> al-maisan, the goal is for there to be official branches for every SuiteSourcePackage
* abentley changed the topic of #launchpad-reviews to: on call: intellectronica, henninge,abentley || reviewing: sinzui, -, -|| queue: [] || This channel is logged: http://irclogs.ubuntu.com
<intellectronica> sinzui: i find the way you format the call to dict on line 222 a bit strange. i think that if you use the function call, you should avoid the trailing coma
<jml> al-maisan, as for which one to check
<jml> al-maisan, I think you should check all of them and the real question becomes whether the results are ANDed or ORed.
<sinzui> intellectronica: I agree with you. I should not have use a trailing comma.
<jml> al-maisan, Really, you're right and the model is wonky here. I'd recommend ANDing them (since it's more restrictive) and we can chase up a more restrictive model w/ james_w and cjwatson.
<al-maisan> jml: I am trying to think whether OR'ing them would be OK as well
<intellectronica> sinzui: the rest looks great, so r=me with these changes
<sinzui> intellectronica: Here is the incremental: http://pastebin.ubuntu.com/296953/
<jml> al-maisan, if a distroseries is closed for a pocket then it should be impossible to upload to the associated branch, I think.
<al-maisan> jml: I agree, however, there's that one-branch-to-many (sourcepackage, pocket) relationship
<intellectronica> sinzui: perfect
* intellectronica changed the topic of #launchpad-reviews to: on call: intellectronica, henninge,abentley || reviewing: -, -, -|| queue: [] || This channel is logged: http://irclogs.ubuntu.com
<jml> al-maisan, fwiw, with the real data, the relationship is in fact 1:1
<al-maisan> so, if any of the (sourcepackage, pocket) combinations become non-valid i.e. karmic/release after Nov. 1st
<al-maisan> jml: that's good news
<al-maisan> in which case we should reconsider the following constraint:
<al-maisan>     "seriessourcepackagebranch__ds__spn__pocket__key" UNIQUE, btree (distroseries, sourcepackagename, pocket)
<jml> umm, don't you mean, "add a uniqueness constraint to branch"?
<al-maisan> I thought a seriessourcepackagebranch contraint like (distroseries, sourcepackagename) probably makes more sense 
<jml> no, because that would make it impossible to have an lp:ubuntu/karmic-security/openssh branch and an lp:ubuntu/karmic/openssh branch
<al-maisan> but then it depends how seriessourcepackagebranch will be used
<al-maisan> jml: maybe we could have brief voice call
<jml> al-maisan, sure.
 * al-maisan fires up skype
<jml> al-maisan, ?
<al-maisan> jml: just a second
<intellectronica> bac: need a reivew?
<bac> intellectronica: yes, please.  i was waiting for the MP to show up before asking but got distracted.
<bac> intellectronica: it'll be your easiest of the day
<intellectronica> bac: so that's just an old test reinstated, right?
<bac> indeedy
<bac> intellectronica: and a smidgen of drive-by
<intellectronica> bac: looks fine, r=me. thanks for remembering to reactivate that test
<bac> intellectronica: thanks
<leonardr> intellectronica, henninge, or abentley: a very simple branch for one of you
<leonardr> https://code.edge.launchpad.net/~leonardr/lazr.authentication/correct-scheme/+merge/13586
<henninge> leonardr: I am on it
* intellectronica changed the topic of #launchpad-reviews to: on call: henninge,abentley || reviewing: -, -|| queue: [] || This channel is logged: http://irclogs.ubuntu.com
* henninge changed the topic of #launchpad-reviews to: on call: henninge,abentley || reviewing: leonadr, -|| queue: [] || This channel is logged: http://irclogs.ubuntu.com
* henninge changed the topic of #launchpad-reviews to: on call: henninge,abentley || reviewing: leonardr, -|| queue: [] || This channel is logged: http://irclogs.ubuntu.com
<henninge> leonardr: looks nice!
<henninge> leonardr: shouldn't the base class do some check if self.AUITH_SCHEME actually exists or provide a default value?
<henninge> Or am I being over-paranoid?
<leonardr> hmm
<leonardr> there's no sensible default value
<leonardr> if you forget it with a new authentication class, the crash will happen at the appropriate time (when you try to use the authentication for the first time, before you commit anything)
<leonardr> so i think it's ok
<henninge> leonardr: ok, but please add a comment to the base class saying that derived classes need to implement this.
<henninge> to the doc string
<henninge> leonardr: r=me
<rockstar> henninge, can I hop into your queue?
<henninge> rockstar: no queue, just forgot to update the topic
<henninge> ;)
* henninge changed the topic of #launchpad-reviews to: on call: henninge,abentley || reviewing: rockstar, -|| queue: [] || This channel is logged: http://irclogs.ubuntu.com
<rockstar> henninge, okay, I'll send it directly to you then.
<henninge> rockstar: show me the mp
<rockstar> henninge, sending now.
<leonardr> henninge, abentley: even more trivial than my last branch: https://code.edge.launchpad.net/~leonardr/lazr.restful/0.9.13/+merge/13588
<henninge> leonardr: r=me
<rockstar> henninge, https://code.edge.launchpad.net/~rockstar/launchpad/code-import-cvs-oops/+merge/13589
<rockstar> henninge, how's it going?
<henninge> rockstar: done, r=me
<rockstar> henninge, thanks!
* henninge changed the topic of #launchpad-reviews to: on call: henninge,abentley || reviewing: -, -|| queue: [] || This channel is logged: http://irclogs.ubuntu.com
<barry> henninge, abentley-lunch ping.  i have a very simple branch
<henninge> barry: bring it on
<barry> henninge: thanks! mp'ing it now
* henninge changed the topic of #launchpad-reviews to: on call: henninge,abentley || reviewing: barry, -|| queue: [] || This channel is logged: http://irclogs.ubuntu.com
<barry> henninge: it's almost ridiculously trivial :)  https://code.launchpad.net/~barry/launchpad/mm2112/+merge/13591
<henninge> barry: the trivality is soothing at this hour ... ;-)
<henninge> barry: r=me
<barry> henninge: thanks! :)
* henninge changed the topic of #launchpad-reviews to: on call: henninge,abentley || reviewing: -, -|| queue: [] || This channel is logged: http://irclogs.ubuntu.com
* henninge changed the topic of #launchpad-reviews to: on call: abentley || reviewing: -|| queue: [] || This channel is logged: http://irclogs.ubuntu.com
<EdwinGrubbs> sinzui: can you do a UI review of this MP (screenshot links in the last comment). https://code.edge.launchpad.net/~edwin-grubbs/launchpad/bug-426899-ppa-links/+merge/13505 
<sinzui> EdwinGrubbs: I really think that the link to "PPA packages related to Curtis Hovey" should be (i) Related PPA packages
<EdwinGrubbs> sinzui: sounds good
<sinzui> hmm. My page is wrong too
<sinzui> Has my hate for these pages evolved to apathy. I am not even surprised that they are wrong
<sinzui> Why would motus trust these page?
<EdwinGrubbs> sinzui: which piece of information is wrong on your page?
<sinzui> EdwinGrubbs: That package in my ppa is superseded. I expected to see the one that users can download.
<sinzui> EdwinGrubbs: have you looked at my related projects. I weep every time I see that page
 * sinzui thinks team participation has no place in related projects or packages
<EdwinGrubbs> sinzui: I don't think that is as big of a problem for people that are not in the Registry Administrators team.
<EdwinGrubbs> rockstar: can you do a secondary ui review of https://code.edge.launchpad.net/~edwin-grubbs/launchpad/bug-426899-ppa-links/+merge/13505
<sinzui> I found a bug relating to it. jml had agreed with the issue. And scottk was concerned too. All 3 of us are concerned. rarely can one issue unite three disparate personalities like this one
<rockstar> EdwinGrubbs, sure.
<rockstar> EdwinGrubbs, looks good.
<jml> sinzui, huh what?
#launchpad-reviews 2009-10-20
<wgrant> sinzui: +uploaded-packages did not show superseded packages for a time. It *needs* to continue to show superseded packages.
<wgrant> +ppa-packages should probably do the same (for consistency), but it is not as important.
<wgrant> But your argument that you would expect to see downloadable packages isn't really valid; if I wanted to see downloadable packages I would go to the PPA's page.
<thumper> https://code.edge.launchpad.net/~thumper/launchpad/branch-no-active-reviews/+merge/13610 anyone?
<mwhudson> thumper: can you review https://code.edge.launchpad.net/~mwhudson/launchpad/in-memory-launchpad-server/+merge/13613 ?
 * thumper looks
<thumper> mwhudson: https://code.edge.launchpad.net/~thumper/launchpad/branch-no-active-reviews/+merge/13610 ?
<thumper> mwhudson: there were two other BMPs created in the two hours between these
<mwhudson> thumper: my question about that was "why do branches implement icanhaslinkedbranch then?"
<mwhudson> but i guess i probably don't want to know
<mwhudson> thumper: yeah, that's lame
<thumper> mwhudson: branches implement IHasMergeProposals
<thumper> because you can get the merge proposals for a branch
<mwhudson> oh right
<mwhudson> maybe you should be able to adapt ibranch to ibranchcollection in some sense?
<mwhudson> never mind
 * mwhudson approves
<mwhudson> although i think i broke some js somehow
<thumper> mwhudson: maybe
<thumper> perhaps that is the better option
<thumper> although it seems a bit artificial to create a branch collection for exactly one branch
<mwhudson> well, i guess a question is, is $branch_url/+activereviews going to be a useful thing?
<mwhudson> (and how would it be different from +merges?)
<thumper> mwhudson: I've started looking through your change, but I need to go make dinner
<thumper> it is quite long :)
<mwhudson> thumper: yeah, it's fairly hairy
<mwhudson> thumper: if you don't get to it can you send jml a mail about it?
<thumper> ok
<thumper> hmm.. ec2 land can't find the public branch location
<thumper> bzr info can
<thumper> pebkac
* gmb changed the topic of #launchpad-reviews to: on call: gmb || reviewing: -|| queue: [] || This channel is logged: http://irclogs.ubuntu.com
 * gmb -> afk for a bit
* bac changed the topic of #launchpad-reviews to: on call: gmb,bac || reviewing: -|| queue: [] || This channel is logged: http://irclogs.ubuntu.com
<bac> good morning
* bac changed the topic of #launchpad-reviews to: on call: gmb,bac || reviewing: -,jtv || queue: [] || This channel is logged: http://irclogs.ubuntu.com
<abentley> gmb, bac: Could I have a review of https://code.edge.launchpad.net/~abentley/launchpad/parse-binary/+merge/13600 please?
<intellectronica> gmb: can i interest you in a very short patch>
<bac> abentley: sure
<gmb> intellectronica: Certainly.
* bac changed the topic of #launchpad-reviews to: on call: gmb,bac || reviewing: -,jtv || queue: [abentley] || This channel is logged: http://irclogs.ubuntu.com
<abentley> bac: tx
* gmb changed the topic of #launchpad-reviews to: on call: gmb, bac || reviewing: abentley, intellectronica || queue: [] || This channel is logged: http://irclogs.ubuntu.com
<intellectronica> gmb: http://pastebin.ubuntu.com/297491/ disables the filebug redirection for ubuntu packages
<gmb> intellectronica: r=me.
<intellectronica> gmb: thanks
<gmb> bac: I'm going to go off-call to deal with a pressing OOPS problem with +filebug. If it gets busy, please ping me and I'll come back.
* gmb changed the topic of #launchpad-reviews to: on call: gmb || reviewing: abentley || queue: [] || This channel is logged: http://irclogs.ubuntu.com
<bac> gmb: righto
<intellectronica> with the current version of utilities/ec2 i can't submit a branch, because it complains about bzr's pqm plugin missing. any idea what's going on?
<intellectronica> i can run without a problem with older versions
<intellectronica> bac: maybe you have a clue? ^^^^^
<bac> intellectronica: haven't seen it.  have you asked allenap, our build engineer?
<intellectronica> no, allenap would be a good person to ask indeed
 * allenap looks
<bac> intellectronica: curious to hear what you find out.  was there a new version from yesterday?  it worked for me yesterday morning.
<allenap> intellectronica: Can you paste the failure?
<intellectronica> bac: the latest revisions are from yesterday and the day before. i don't know when it happens because i was using old versions on those days
<intellectronica> allenap: http://pastebin.ubuntu.com/297511/
<allenap> intellectronica: I have an idea.
<intellectronica> allenap: b.t.w don't tell anyone, but the machine i'm running this on is running jaunty. could it be that the update to make it work with karmic broke working with a previous version?
<allenap> intellectronica: Only yesterday a change was made to utilities/ec2 to make it use python2.4.
<allenap> intellectronica: Mmm, I don't know if that will matter...
<allenap> Okay, so maybe /usr/bin/python could see the pqm plugin okay, but /usr/bin/python2.4 can't.
<bac> thanks abentley.  r=bac
* bac changed the topic of #launchpad-reviews to: on call: bac || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com
<allenap> intellectronica: Try /usr/bin/python2.4 -m bzrlib.plugins.pqm
<allenap> intellectronica: And the same with just /usr/bin/python.
<abentley> bac: Thanks!
<allenap> gary_poster: I think you changed the interpreter in utilities/ec2 to be python2.4. I thought that boto didn't work with python2.4?
<gary_poster> allenap: uh, dunno, working for me. you encountering a problem?
<gary_poster> allenap: btw thank your for buildbot config fix.  will review asap
<allenap> gary_poster: I haven't looked at it on karmic yet (only upgraded yesterday), but I seem to remember that boto used some python2.5+ features. Of course, I can't remember what now. If it works then I shouldn't complain :)
<allenap> intellectronica: Anyway, try doing /usr/bin/python utilities/ec2
<gary_poster> allenap: :-) ok.  I'm on karmic and it is working for me.  We have a LP Python 2.5 branch with passing tests also, btw, but can't land it till LOSAs have bandwidth.  We're trying to make it to 2.6.
<gary_poster> allenap: I changed the ec2 script because it wasn't working for me without that change...I'll dig into it if you need me to.
<allenap> gary_poster: If you can remember, I'm interested to know why, but don't spend time digging.
<allenap> gary_poster: Though I suspect that, like for intellectronica, it won't work for me now :-/
<gary_poster> allenap: I don't remember and have four other conversations pending. ;-) If you take out the change and it works for you, revert that bit with my blessing.  We may need to look at it more closely later, but not blocking is important, and I'd prefer not to consume the bandwidth on it ATM
<allenap> gary_poster: That works for me.
<gary_poster> allenap: thank you
<intellectronica> allenap: with /usr/bin/python i get a different error
<allenap> intellectronica: Progress :)
<intellectronica> yeah, that's what theoretical scientists call "problem shift" :)
<allenap> I need to remember that :)
<intellectronica> allenap: http://pastebin.ubuntu.com/297523/ is what i get
<allenap> intellectronica: Oh my.
<adeuring> bac: could you please review this MP: https://code.edge.launchpad.net/~adeuring/launchpad/hwdb-build-udev-device-list/+merge/13644 ?
<bac> adeuring: i'm on it!
* bac changed the topic of #launchpad-reviews to: on call: bac || reviewing: abel || queue: [] || This channel is logged: http://irclogs.ubuntu.com
<adeuring> bac: thanks!
<allenap> intellectronica: Okay, I'm clutching at straws now, but could you try: rm _pythonpath.py && bin/buildout && utilities/ec2
<intellectronica> allenap: i find it hard to believe that this would matter in a new branch, but let me try
<intellectronica> allenap: yeah, that didn't help
<allenap> intellectronica: Oh, a new branch.
<gary_poster> intellectronica, allenap: I think I know what the problem is, in vague terms, though I don't know why the current version works for me and breaks for intellectronica.  I can at least do a quick call with allenap to share the details and maybe also come up with a solution.  Again, I'm also fine with a revert (of just this file, he said nervously!) if that helps
<intellectronica> gary_poster: it's not urgent to fix this, since i can continue using the older revision locally. i just wanted to alert you guys to the problem
<gary_poster> intellectronica: got it, thank you
<allenap> gary_poster: If intellectronica is okay for now then it sounds like you've got more pressing things to do. I'll see if I can figure something out anyway; I have other suspicions.
<achuni> bac: if you really have the time, thanks:
<achuni> https://code.launchpad.net/~canonical-isd-hackers/canonical-identity-provider/wip1/+merge/13351
<achuni> https://code.launchpad.net/~canonical-isd-hackers/canonical-identity-provider/wip2/+merge/13362
<achuni> https://code.launchpad.net/~canonical-isd-hackers/canonical-identity-provider/wip/+merge/13363
<achuni> https://code.launchpad.net/~canonical-isd-hackers/shipit/sso-test-fixes/+merge/13643
<achuni> (in that order)
<gary_poster> allenap: ok cool, thank you.  Is it failing for you?
<allenap> gary_poster: No, actually, it seems fine.
<gary_poster> allenap: ah. :-/  yeah, diagnosing is hard when it works locally.  The error looked like bzr was not finding its site-packages plugins, which makes sense, because launchpad's bzr version would mask the site-packages version.  
<gary_poster> I'm not exactly sure why it *is* working for me (and others) now.  It would be interesting to try adding the pqm-submit plugin to launchpad's bazaar-plugins directory and then have ec2 set the bzr environ variable to look there for plugins.  That's a hack though.  ...again, being able to dupe would be nice.
<intellectronica> oh, are we now using a bzr version other than the site-installed one?
<allenap> gary_poster: I've just realised that I have the pqm plugin installed in ~/.bazaar/plugins, which might account for why it's finding it for me.
<allenap> intellectronica: The change to utilities/ec2 made it use the bzrlib in the lp tree, via the egg.
<gary_poster> allenap, intellectronica: yes
<gary_poster> (a nice advantage is that ec2 can now be put somewhere in your path (I have a symlink in ~/bin) and it works)
<allenap> intellectronica, gary_poster: Perhaps we should add bzr-pqm to setup.py and let it take care of it.
<allenap> s/it/buildout
<gary_poster> allenap: maybe.  I don't think bzr-pqm is available as a python package; Martin Pool and I were going to talk about that at some point
<gary_poster> allenap: if we were going to do that, I'd be tempted to do that in a slightly different way so that lp itself doesn't depend on the egg, just the script
<bac> achuni: can you subscribe me to those branches?  i don't have access.
<sidnei> heya, any candidate for a lazr-js review? 
<sidnei> https://code.edge.launchpad.net/~sidnei/lazr-js/jstestdriver-support/+merge/13645
<achuni> bac: sure, one sec
* bac changed the topic of #launchpad-reviews to: on call: bac || reviewing: achuni || queue: [] || This channel is logged: http://irclogs.ubuntu.com
<achuni> bac: subscribed
<allenap> intellectronica: As a work-around, it'll probably be enough to bzr branch lp:bzr-pqm ~/.bazaar/plugins/pqm
<bac> achuni: thanks.  i can see it now.
<achuni> great
<intellectronica> allenap: aha! problem shift, once again :) http://pastebin.ubuntu.com/297560/
<allenap> intellectronica: Ah! This version of boto won't work with python2.4.
<allenap> intellectronica: Can you do python -c 'import boto; print boto.Version'
<allenap> intellectronica: I have 1.8d
<allenap> I'm on karmic.
<intellectronica> allenap: 1.5b
<intellectronica> what a strange versioning scheme
<allenap> intellectronica: Yeah! I wonder if 1.8d is python2.4 compatible, and 1.5b not.
<intellectronica> it sounds unlikely that an older version will be less compatible with an older version of python, but who knows
<allenap> intellectronica: The README says "Efforts are made to keep boto compatible with Python 2.4.x but no guarantees are made."
<allenap> intellectronica: So it could go back and forth.
<allenap> intellectronica: Let's bypass utilities/ec2 altogether! PYTHONPATH=lib python -c 'from devscripts.ec2test.entrypoint import main; main()'
<allenap> intellectronica: In the next week or so LP will be python2.5 at least so this problem will go away... I hope.
<intellectronica> allenap: yeah, that works
<allenap> intellectronica: Woohoo! I'll send an email to the list for anyone else who is suffering.
<intellectronica> allenap: thanks
<sidnei> i wonder if i should append myself to the queue ;)
<bac> sidnei: you have a branch?  append away
<bac> achuni: i've done wip1 with some comments.
* sidnei changed the topic of #launchpad-reviews to: on call: bac || reviewing: achuni || queue: [sidnei] || This channel is logged: http://irclogs.ubuntu.com
<bac> sidnei: do you have an URL for your branch?  i don't see it on +activereviews for lp
<sidnei> bac: it's a lazr-js branch https://code.edge.launchpad.net/~sidnei/lazr-js/jstestdriver-support/+merge/13645
<bac> sidnei: thanks.  i'll get right on it but will break for lunch in a bit
<sidnei> bac: no hurry
<achuni> bac: thanks
<bac> sidnei: if i'm uncomfortable with the javascript i may defer.  just letting you know.
<sidnei> bac: that's fine, yeah.
<bac> sidnei: er, the diff is 3500 lines.  is that a mistake?
<sidnei> bac: it's based on https://code.edge.launchpad.net/~bjornt/lazr-js/buildoutification, which might have not been merged yet
<bac> sidnei: can i get you to make an accurate diff and paste it?
<sidnei> bac: supposedly yeah :)
* bac changed the topic of #launchpad-reviews to: on call: bac-lunch || reviewing: achuni || queue: [sidnei] || This channel is logged: http://irclogs.ubuntu.com
<sidnei> bac: here https://pastebin.canonical.com/23563/
<leonardr> bac-lunch, please add https://code.edge.launchpad.net/~leonardr/lazr.restful/2.6/+merge/13653 to your queue
* bac changed the topic of #launchpad-reviews to: on call: bac || reviewing: achuni || queue: [sidnei] || This channel is logged: http://irclogs.ubuntu.com
* bac changed the topic of #launchpad-reviews to: on call: bac || reviewing: achuni || queue: [sidnei, leonard] || This channel is logged: http://irclogs.ubuntu.com
* bac changed the topic of #launchpad-reviews to: on call: bac || reviewing: achuni || queue: [sidnei, leonardr] || This channel is logged: http://irclogs.ubuntu.com
<gary_poster> bac, leonardr I already reviewed that one.  changing topic
* gary_poster changed the topic of #launchpad-reviews to: on call: bac || reviewing: achuni || queue: [sidnei] || This channel is logged: http://irclogs.ubuntu.com/
<bac> thank gary_poster
<gary_poster> :-) welcome
<bac> sidnei: why did you change the DOCTYPE spec to have 'http:///' with three slashes?
<sidnei> bac: might have been a bad search & replace.
<bac> sidnei: ok.  yeah, it looks pretty funny
<sidnei> ok, fixed those
<bac> sidnei: where did yui.patch.js come from?  is it an official YUI patch?  should you include some attribution or reference?
<sidnei> bac: it was from jstestdriver itself. maybe there should be some attribution yeah.
<bac> sidnei: done
<sidnei> bac: you rock. thanks!
<EdwinGrubbs> bac: Can I put this in your queue? https://code.edge.launchpad.net/~edwin-grubbs/launchpad/bug-435260-duplicate-links/+merge/13657
<bac> EdwinGrubbs: soitanly
<EdwinGrubbs> bac: BTW, I have to go to lunch now.
* Edwin-lunch changed the topic of #launchpad-reviews to: on call: bac || reviewing: achuni || queue: [sidnei, Edwin] || This channel is logged: http://irclogs.ubuntu.com/
<bac> Edwin-lunch: too bad.  i was going to jump you to the front since no one else who wants a review is around atm
<bac> Edwin-lunch: done.  nice work.  that's some fancy CSS you got there.
* bac changed the topic of #launchpad-reviews to: on call: bac || reviewing: - || queue: [-] || This channel is logged: http://irclogs.ubuntu.com/
<EdwinGrubbs> bac: thanks
<bac> np
<henninge> bac: I have a review for you, if you don't mind.
<henninge> https://code.edge.launchpad.net/~henninge/launchpad/bug-456498/+merge/13668
* henninge changed the topic of #launchpad-reviews to: on call: bac || reviewing: - || queue: [henninge] || This channel is logged: http://irclogs.ubuntu.com/
<bac> hi henninge
<bac> henninge: i'll look at it now
<henninge> bac: thank you
<bac> henninge: can you reword the last sentence at lines 11&12?  i think you have an extra word or are missing some.
<henninge> bac: yes, the "detect" should go
<bac> henninge: why didn't you make trans_credit_type an enum?
<henninge> bac: to keep it simple, actually
<bac> henninge: to make this patch simple, maybe, but i don't think the code is going to be in the long run.  i just hate seeing all of those magic strings.
<bac> henninge: and to a translations outsider, a value of 'Your emails' seems odd
<henninge> bac: true
<bac> henninge: otherwise this is a lovely patch
<bac> henninge: could i get you to take a stab and making those an enum and seeing if it really is that much more complicated?
<henninge> bac: yes you can ;)
<bac> henninge: thanks!  i've got a few other niggly things i'll put in the MP.
<henninge> bac: thank you, too
* bac changed the topic of #launchpad-reviews to: on call: bac || reviewing: - || queue: [-] || This channel is logged: http://irclogs.ubuntu.com/
* bac changed the topic of #launchpad-reviews to: on call: - || reviewing: - || queue: [-] || This channel is logged: http://irclogs.ubuntu.com/
<EdwinGrubbs> beuno: Can you do a UI review of the branch you already pre-reviewed? New screenshots are in the last comment. https://code.edge.launchpad.net/~edwin-grubbs/launchpad/bug-435260-duplicate-links/+merge/13657
<beuno> EdwinGrubbs, on it
<beuno> EdwinGrubbs, done
<intellectronica> who wants to review a little branch?
<intellectronica> a windmill fix
<intellectronica> it's quite minimal
<intellectronica> mwhudson: how about you?
<intellectronica> or maybe rockstar?
<mwhudson> intellectronica: sure
<mwhudson> i'm probably the most ignorant team member about js though
<rockstar> mwhudson, I'd be happy the grab it if you're uncomfortable taking it.
<intellectronica> mwhudson: you rock. http://pastebin.ubuntu.com/297838/
<intellectronica> or rockstar, who's got rock in his own very name
<intellectronica> there's not much js there, though. it's written in python
<intellectronica> it's a fix for https://bugs.edge.launchpad.net/malone/+bug/456315
<mup> Bug #456315: Windmill test test_bug_tags_entry errors <js> <Launchpad Bugs:Triaged by intellectronica> <https://launchpad.net/bugs/456315>
<rockstar> intellectronica, this is just moving it the BugsWindmillLayer?
<intellectronica> rockstar: pretty much. i also converted some setupd code that used the ui to use the object factory, it makes it much shorter and nicer
<intellectronica> rockstar: see lines 33 - 45
<rockstar> intellectronica, lines 41 and 42 - abentley also had this problem.  Can you file a bug about canonical_url not being good for windmill tests?
<rockstar> intellectronica, I think the solution is ugly, but there's currently no way around it.
<intellectronica> rockstar: sure, doing that now. the solution is ugly because you must specify the port, but i'd like to extend canonical_url to take the scheme and port as parameters
<rockstar> intellectronica, essactly.
<intellectronica> rockstar: https://bugs.edge.launchpad.net/launchpad-foundations/+bug/456773
<mup> Bug #456773: Make canonical_url work nicely in Windmill tests <Launchpad Foundations:New> <https://launchpad.net/bugs/456773>
<rockstar> intellectronica, I see your comment about the click event not firing the js event but actually moving.  Have you found anything that can be done about this?
<rockstar> intellectronica, I ask because I now suspect that must be happening the branch subscriber tests -- They fail randomly at different places when the whole CodeWindmillLayer tests are run, but not when run by themselves.
<intellectronica> rockstar: i must admit i haven't looked at it lately. it's possible that with the new test infrastructure things have improved, but i'm not too hopeful
<rockstar> intellectronica, is there an mp somewhere?
<intellectronica> rockstar: no, let me create one
<rockstar> intellectronica, okay, when you create it, link me and I'll r=me
<intellectronica> rockstar: cheers. https://code.edge.launchpad.net/~intellectronica/launchpad/fix-test-bug-tags-entry/+merge/13678
<rockstar> intellectronica, done.
<intellectronica> lovely. thanks a bunch, rockstar
<rockstar> intellectronica, no problem.
#launchpad-reviews 2009-10-21
<abentley> intellectronica: I think it would be nice if windmill tests just pushed a config value that made canonical_url return the correct port and scheme.
<intellectronica> abentley: yeah, or they could even just decorate canonical_url to do the transformation
<abentley> That could work too.
<intellectronica> abentley: actually, i think the best solution is to simply have another canonical_url in the testing package, and just import it from there. that's a bit more explicit than modifying the behaviour behind the user's back
<abentley> intellectronica: The behaviour of canonical_url is already customized in a testing context-- I think it's not surprising.
<intellectronica> you're right, i didn't consider that it's already modified
<intellectronica> anyway, add that to the bug. it's too late for me to fix it now but i prolly will tomorrow morning
<thumper> mwhudson: feel like reviewing my branch?
<mwhudson> thumper: sure
 * thumper sends it in
<intellectronica> who wants to review a windmill fix? it's a very easy review
<intellectronica> gmb: awake?
<intellectronica> mwhudson: review? it's pretty much just moving code around without change
<mwhudson> intellectronica: ok
<intellectronica> mwhudson: https://code.edge.launchpad.net/~intellectronica/launchpad/fix-test-mark-duplicate/+merge/13682
<intellectronica> mwhudson: http://pastebin.ubuntu.com/297931/ if you don't want to wait for the diff to generate
<mwhudson> looking
<intellectronica> mwhudson: all the patch does is take the test, move it to the directory up one level, and make the test function a method of a new proper test suite
<mwhudson> intellectronica: done
<intellectronica> mwhudson: danke
<mwhudson> thumper: what's the difference between test_linked_to_development_package and test_linked_to_dev_package ?
<thumper> ?
<thumper> where?
<mwhudson> thumper: in your branch, lib/lp/code/model/tests/test_branch.py
<thumper> line number?
<mwhudson> 489 & 506
<mwhudson> they look like different ways (yay, sigh) of saying the same thing
<thumper> not quite
<thumper> one uses the RELEASE pocket
<thumper> so it is the dev focus
<thumper> the other uses the backports pocket
<thumper> so it definitely isn't
<mwhudson> thumper: not that test
<thumper> arse
<thumper> it is
<thumper> I thought I looked for a test that said that
<thumper> grr
<thumper> I can remove the duplicate
<mwhudson> soyuz's confusatron field creeps into code :(
<mwhudson> thumper: thanks
<mwhudson> thumper: branch reviewed
<thumper> thansk
<thumper> aaaaaaaaaaaaaaaaaaaarrrrrrrrrrrrrrrrgggggggggggggggghhhhhhhhhhhhhhhhh
 * thumper was reading wrong class
<mwhudson> thumper: ?
<thumper> lazr-js TextAreaEditorWidget
<thumper> mwhudson: I've fixed the accept_empty bit
<thumper> mwhudson: and I can live with the style id as we don't yet have descriptions for merge proposals
<thumper> ohh
<thumper> bugger
<mwhudson> thumper: it sounds like you're js hacking!
<thumper> mwhudson: look at the priv msg
<thumper> mwhudson: that is public
<mwhudson> hm?
<mwhudson> thumper: oh
<mwhudson> bugger
<thumper> yeah
 * thumper is fixing
<thumper> mwhudson: there is a branch that simply moves the bmp registant ready :)
<mwhudson> will loading the page do nasty things to my browser?
<thumper> no
<mwhudson> thumper: inline reviewing seems to have gone away
<thumper> sometimes, and I don't know why
<mwhudson> "yay"
<thumper> mwhudson: any idea why this test doesn't fail? https://pastebin.canonical.com/23598/
<mwhudson> thumper: wouldn't that only fail after you've fixed the bug?
<thumper> mwhudson: which I have
<mwhudson> thumper: oh
<mwhudson> then i don't know
<thumper> mwhudson: I was changing the test to show that it is fixed
<thumper> bum
<mwhudson> maybe extract_text is full of crack?
<thumper> mwhudson: the web ui shows the right thing
<thumper> it shouldn't be
<thumper> I wrote it :)
<mwhudson> heh heh
<thumper> mwhudson: I found it
<thumper> mwhudson: I'm editing the wrong buffer
<thumper> right file
<thumper> wrong branch
<thumper> D'oh
<mwhudson> lolz
<thumper> just did it again for the fix :(
<thumper> mwhudson: want to review the v.small fix?
<mwhudson> thumper: sure
<thumper> mwhudson: just waiting on the diff
<thumper> mwhudson: you should have it now
<mwhudson> thumper: please get spm to cowboy the template fix?
* jtv changed the topic of #launchpad-reviews to: on call: jtv || reviewing: â || queue: [] || This channel is logged: http://irclogs.ubuntu.com/
* jtv changed the topic of #launchpad-reviews to: on call: jtv || reviewing: danilo || queue: [] || This channel is logged: http://irclogs.ubuntu.com/
* allenap changed the topic of #launchpad-reviews to: on call: jtv, allenap || reviewing: danilo, - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/
<allenap> Morning jtv :)
<jtv> allenap: morning!
<jtv> allenap: oh, I already did danilo's.  All the other ones on the queue were for me.  :-)
* jtv changed the topic of #launchpad-reviews to: on call: jtv, allenap || reviewing: â, â || queue: [] || This channel is logged: http://irclogs.ubuntu.com/
<allenap> jtv: Okay, I'll do yours then. There's one from jelmer too, for launchpad-cscvs, but I think I might leave that for someone who's more familiar with vcs stuff than me.
<jtv> allenap: I can try that one then.  Hadn't seen it appear.
* allenap changed the topic of #launchpad-reviews to: on call: jtv, allenap || reviewing: â, jtv || queue: [] || This channel is logged: http://irclogs.ubuntu.com/
<adeuring> jtv: may I bother you with an MP having an overly long diff? 1000 lines, but mostly mechanical changes. Only ~130 lines are "real" changes.
<jtv> adeuring: ok
<adeuring> jtv: thanks!
* jtv changed the topic of #launchpad-reviews to: on call: jtv, allenap || reviewing: adeuring, jtv || queue: [] || This channel is logged: http://irclogs.ubuntu.com/
<jtv> adeuring: it's the new-attempt one?
<adeuring> jtv: no, this branch: lp:~adeuring/launchpad/hwdb-build-udev-device-list-2 . Need to write the mp
<adeuring> jtv: https://code.edge.launchpad.net/~adeuring/launchpad/hwdb-build-udev-device-list-2/+merge/13695/comments/35227/+reply
<adeuring> erm, scrap that "+reply"; want to add the test command and the "make lint" output
<jtv> adeuring: ok
<adeuring> jtv: https://code.edge.launchpad.net/~adeuring/launchpad/hwdb-build-udev-device-list-2/+merge/13695 , that's what i menat.
<jtv> adeuring: I had arrived there yes, thank you :)
<jtv> adeuring: I do think it's a bit ugly to do self.devices = devices = {}
<adeuring> jt: why?
<jtv> The "shorthand" of saying devices instead of self.devices doesn't even save you much here.
<adeuring> well it saves one word -- "self" ;) But I'll remove the local variable
<jtv> When I read that line, I have to worry: do you really mean two references to the same dict, or is it a mistake?
<jtv> Thanks.
<jtv> adeuring: in the hwdb_submission_processing test, SubmissionParserFakedBuildDeviceList is a bit long as a name.  How about MockSubmissionParser?
<adeuring> jtv: Sure
<jtv> Good testing technique, btw.  I sometimes use the real object and just inject a callable mock object in place of a method, but if you can get away with something short and sweet like this, great.
<adeuring> jtv: the problem with mock ojbects here is that it such data would probably look quite ugly and convoluted. The test file has many examples of this ;)
<jtv> adeuring: in test_processSubmission_buildDeviceList_failing it might be useful though.
<adeuring> jtwhy?
<jtv> def no(*args, **kwargs):
<jtv>     return False
<jtv> #...
<jtv> parser.buildDeviceList = no
<jtv> adeuring: that saves you a lot of long identifiers (though "no" may be a bit too short :-)
<adeuring> jtv:  question is what sort of test styles we prefer. I another review I got the recommendation to avoid monkey-patching ;)
<adeuring> jtv: personally, I don't mind much, btu we should be consistent ;)
<jtv> adeuring: IMHO, only when there's a clear winner among the options.  :-)
<adeuring> jtv: well, as I said, I really don't mind. So, you think I should use a method like "nyet(...)"?
<adeuring> ...oder "nein()"
<jtv> adeuring: in this case I think it's justified.
<adeuring> jtv: OK, changed.....
<jtv> adeuring: approved
<adeuring> jtv: thanks!
<jtv> macht nix
* jtv changed the topic of #launchpad-reviews to: on call: jtv, allenap || reviewing: â, jtv || queue: [] || This channel is logged: http://irclogs.ubuntu.com/
<leonardr> jtv, allenap: https://code.edge.launchpad.net/~leonardr/lazr.restfulclient/264098-eq-ne/+merge/13697
<allenap> leonardr, jtv: I can take that.
* allenap changed the topic of #launchpad-reviews to: on call: jtv, allenap || reviewing: â, leonardr || queue: [] || This channel is logged: http://irclogs.ubuntu.com/
<jtv> allenap: thanks, some very nice suggestions there.  And the one hard-to-document parameter turned out to be unused.  :)
<allenap> jtv: Woohoo :)
<allenap> leonardr: You have tests for recipe.__hash__(), but there is nothing in the diff that defines the __hash__ method.
<leonardr> allenap: i'm using the default python object implementation
<leonardr> it was the only way i could think of to show that these two python objects were different
<leonardr> but i'm probably missing something obvious
<allenap> leonardr: Okay. You could say recipe is recipe2 and expect False.
<leonardr> ok
<leonardr> that's nice and obvious
<danilos> jtv: got another one for you ;) https://code.edge.launchpad.net/~danilo/launchpad/bug-455771/+merge/13699 (diff should be regenerated any minute now)
<jtv> give them a finger and they want the whole hand...
<danilos> jtv: that's how it works :)
<danilos> jtv: anyway, the diff is there now
<jtv> sigh.  Coming...
<jtv> (for the public record: not serious :)
* jtv changed the topic of #launchpad-reviews to: on call: jtv, allenap || reviewing: danilo, leonardr || queue: [] || This channel is logged: http://irclogs.ubuntu.com/
<jtv> danilos: I'm too lazy to look it up, but you have a conditional top-portlet now.  Won't that upset your layout when the div is not shown?
* allenap changed the topic of #launchpad-reviews to: on call: jtv, allenap || reviewing: danilo, - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/
<danilos> jtv: nope
<jtv> danilos: too late; it's already in the mp.  :-P
<leonardr> jtv, allenap: and now, https://code.edge.launchpad.net/~leonardr/lazr.restfulclient/verbose-http-error/+merge/13702
<jtv> leonardr: sorry, I'm just eod'ing (and in standup)
<leonardr> np
* jtv changed the topic of #launchpad-reviews to: on call: allenap || reviewing: â || queue: [] || This channel is logged: http://irclogs.ubuntu.com/
<jtv> allenap: bowing out.  Have a good one!
<leonardr> allenap, can you look at the branch i linked above?
<allenap> jtv: Cheerio :) I'll review your bug #417968 branch now.
<mup> Bug #417968: Auto-approver: pathless template with unique name <import-queue> <Launchpad Translations:In Progress by jtv> <https://launchpad.net/bugs/417968>
* allenap changed the topic of #launchpad-reviews to: on call: allenap || reviewing: jtv || queue: [] || This channel is logged: http://irclogs.ubuntu.com/
* EdwinGrubbs changed the topic of #launchpad-reviews to: on call: allenap, Edwin || reviewing: jtv || queue: [] || This channel is logged: http://irclogs.ubuntu.com/
<leonardr> allenap, edwin: please review https://code.edge.launchpad.net/~leonardr/lazr.restful/fix-media-type-misspelling/+merge/13711
<allenap> leonardr: I'm on it.
* allenap changed the topic of #launchpad-reviews to: on call: allenap, Edwin || reviewing: jtv&leonardr, - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/
<allenap> leonardr: r=me
* allenap changed the topic of #launchpad-reviews to: on call: allenap, Edwin || reviewing: jtv, - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/
<leonardr> allenap: and the follow-up: https://code.edge.launchpad.net/~leonardr/lazr.restfulclient/fix-media-type-misspelling/+merge/13715
<allenap> leonardr: Okay.
* allenap changed the topic of #launchpad-reviews to: on call: allenap, Edwin || reviewing: leonardr, - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/
<allenap> EdwinGrubbs: Do you have much VCS-fu? There's a proposal from Jelmer for launchpad-cscvs, https://code.edge.launchpad.net/~jelmer/launchpad-cscvs/converted-from/+merge/13542
<jml> allenap, Were I you I'd politely ask abentley to review that one.
<allenap> jml: Okay.
<allenap> abentley: Please can you have a look at Jelmer's launchpad-cscvs proposal, https://code.edge.launchpad.net/~jelmer/launchpad-cscvs/converted-from/+merge/13542 :)
<abentley> jml, allenap: rockstar volunteers.
<rockstar> allenap, I can take it.  I'm one of the few who have actually braved cscvs.
<rockstar> And I like abentley too much to expose him to that.
<allenap> rockstar: Awesome :)
<jml> rockstar, yay
<allenap> leonardr: r=me, with a question/suggestion.
* allenap changed the topic of #launchpad-reviews to: on call: Edwin || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/
<leonardr> that's not a bad suggestion
<EdwinGrubbs> allenap: I don't think I'm qualified to review that.
<allenap> EdwinGrubbs: Sorry, rockstar took it already, shortly after I asked you.
<allenap> EdwinGrubbs: I should have said something :(
<EdwinGrubbs> allenap: well, I was having networking issues anyway.
<leonardr> jml, are you around?
<jml> leonardr, yes.
<jml> leonardr, how can I help?
<leonardr> i'd like your opinion on a branch i've written to address bug 319432
<mup> Bug #319432: RelativeURIError when trying to access an redacted object <api> <tech-debt> <launchpadlib :Triaged by leonardr> <https://launchpad.net/bugs/319432>
<leonardr> i'm writing an mp now
<jml> leonardr, my opinion is always available :)
<leonardr> jml: https://code.edge.launchpad.net/~leonardr/lazr.restfulclient/319432-redacted/+merge/13716
<leonardr> edwin, would you look at https://code.edge.launchpad.net/~leonardr/lazr.restfulclient/319432-redacted/+merge/13716 ?
<bac`> beuno: you around for some ui questions?
<beuno> bac`, sure
<bac`> beuno:  great.  i'm working on bug 419742.  (hey i like my jaunty little reverse grave.)
<mup> Bug #419742: downloads page should include some data about the releases <post-3-ui-cleanup> <Launchpad Registry:In Progress by bac> <https://launchpad.net/bugs/419742>
<bac`> salgado had made some changes coming up with http://launchpadlibrarian.net/30906325/download.png
<bac`> i've been trying to work in poolie's other comments and have gotten this:
<bac`> http://people.canonical.com/~bac/download.png
<bac`> beuno: i was hoping you could give me some ideas if i'm on the right track
 * beuno looks
<bac`> beuno: i've made the 'release notes' optional, added 'changelog' and made it optional and collapsible,  and moved the date out there to the right.
<bac`> oh, added more vertical separation between the sections
<beuno> vertical separation == awesome
<bac`> unbolded "Release notes:"
<beuno> the label/value there is unclear
<bac`> for RN?
<beuno> yes
<bac`> you mean you want the bold back?
<bac`> what do you think about the twisty hanging out to the side of changlog?
<beuno> well, it seems to be the standard everywhere else, so yes for the bold
<bac`> ok, <b>RN and CL
<beuno> so, I like the idea
<bac`> it used to say "There are no release notes" but i just got rid of the section if they aren't there
<beuno> that's great
<bac`> cool
<beuno> the changelog, we don't show any of it, right?
<bac`> not unless they twist
<bac`> then it slithers in
<bac`> and you're ok with the release date hanging out to the side of the h2?
<beuno> ok, so it's not really "the full changelog" as much as the "the changelog"
<beuno> I like what you did there in the h2, yes. I'd like to see it in staging with real data, but it's nice
<bac`> beuno: oh, right.  i c-n-p that part.  didn't notice the text
<bac`> beuno: great.  i'll ask poolie later if this meets his concerns
<beuno> bac`, I'd just say "> Changelog"
<bac> ok
<beuno> I don't know about the date on the right though
<beuno> seems too disconnected
<beuno> and
<beuno> infally
<beuno> the "add download for release" is odd
<beuno> why not just below each release?
<bac> ok a '(+) Add download file' to the right below the table
<beuno> yes, right-aligned
<beuno> and you win
<bac> great!  thanks for the suggestions
<bac> beuno: what would you think about putting each release into a portlet to give them more separation from one another?
<beuno> bac, it sounds good. Can you mock it up cheaply so we can look at it?
<bac> done.  let me snap it and scp
<bac> http://people.canonical.com/~bac/download2.png
 * beuno waits
<bac> this does not have the first one marked as 'top-portlet' as that will require some reorganization of the template
* sinzui changed the topic of #launchpad-reviews to: on call: Edwin || reviewing: - || queue: [sinzui] || This channel is logged: http://irclogs.ubuntu.com/
<sinzui> EdwinGrubbs: do you have time to review https://code.launchpad.net/~sinzui/launchpad/package-link-validation-2/+merge/13727
<EdwinGrubbs> sinzui: sure
<EdwinGrubbs> sinzui: I don't understand what it means that "the distroseries should also be linked". Is that a link on the page, or should the distroseries be linked with the productseries in the db?
<EdwinGrubbs> sinzui: I get an error when I try to link "mozilla-firefox" on this page https://launchpad.dev/firefox/trunk/+ubuntupkg
<EdwinGrubbs> sinzui: Even if the error is correct, the message should be on the field instead of having two errors above the form.
<sinzui> ha
<sinzui> EdwinGrubbs: I bet the example is bad sample data
<sinzui> EdwinGrubbs: on edge +ubuntupkg has a table listing the distroseries and sourcepackage, but they are not linked. You need to hack the URL to see the object that the series is linked too
<sinzui> EdwinGrubbs: https://launchpad.dev/firefox/+packages
<sinzui> ^ See, that is why this view is broken, you just tried to create a duplicate link. Another series (0.9)  is already the source pacakge
<sinzui> EdwinGrubbs: did you notice that the error message had a link to the package: https://launchpad.dev/ubuntu/hoary/+source/mozilla-firefox
<sinzui> ^ That shows that 1.0 is in fact the upstream for mozilla-firefox
<sinzui> EdwinGrubbs: So for trunk cannot be the upstream for warty or hoary because those SPs are already linked. You can make grumpy Frozen there by making it the currentseries, then you can try to link trunk to mozilla-firefox
<EdwinGrubbs> sinzui: It still seems like PackagingAddView.validate() should use setFieldError() instead addError(), or the separate "There is 1 error" message shouldn't appear.
<sinzui> EdwinGrubbs: I think you are right
<sinzui> EdwinGrubbs: my last addition to the base class does use setFieldError. The original code did not. I can quickly fix the whole class
<sinzui> EdwinGrubbs: oh I see
<sinzui> EdwinGrubbs: This is a bit tricky
<sinzui> EdwinGrubbs: In the base class we do not know if there error is the series or the sourcepackage. In the new class, we know for certain that the error is sourcepackage.
<sinzui> EdwinGrubbs: I could verify if the default_distroseries is not None and choose setFieldError()
<EdwinGrubbs> ok
<EdwinGrubbs> sinzui: It also seems strange to me to call packaging_util.packagingEntryExists(), when a single call to Packaging.selectOneBy() would allow you to make both checks, and it would allow you to say which productseries is already linked to the source package.
<sinzui> It is not 
<sinzui> There are two issues and one is easier to fix than the oser
<sinzui> other
<EdwinGrubbs> sinzui: can there be multiple packaging entries for the same sourcepackagename and distroseries?
<sinzui> The specific match probably means the *you* made a mistakes and you can fix it. The other error is someone else and you may want to investigate before making the chage
<sinzui> No, but there are 81 in production
<sinzui> Users trying to fix the data from the dsp, sp, p, or ps views are getting oopes as they struggle to fix the data
<sinzui> EdwinGrubbs: a hack was put in place two years ago where we recognised this happens, but we only show the last one. this compounded the error because you cannot see the shadowed link to know it exists. the user is trying to add the link when he should be deleting the one he is seeing
<sinzui> Except we never wrote code to delete a link to an SP
<sinzui> There are a lot of oops that revolve around the the root cause that we let multiple links be created
<sinzui> EdwinGrubbs: when this branch lands, it will not be possible to create a duplicate link, We can then fix the schema to not allow it to happen. My next branch is to allow the deletion on links to SPs
<EdwinGrubbs> sinzui: is there a bug open to add a unique constraint on (distroseries, sourcepackagename) on the packaging table?
<sinzui> Yes, it is on our list of bugs in progress that we review every morning
<sinzui> https://bugs.edge.launchpad.net/bugs/196774
<mup> Bug #196774: It shouldn't be possible to link multiple productseries to a sourcepackage in a given distroseries <package-link> <Launchpad Registry:In Progress by sinzui> <https://launchpad.net/bugs/196774>
<EdwinGrubbs> doh
<EdwinGrubbs> sinzui: ok, that answers my questions. I'll send off the review in a sec. There are just minor style issues.
<sinzui> okay.
<sidnei> alright!
<sidnei> Edwin-food: add https://code.edge.launchpad.net/~sidnei/lazr-js/yui-3.0.0/+merge/13737 to your queue when you get back
<sidnei> uhm. i don't know how to get a proper diff i fear.
<sidnei> EdwinGrubbs: ok, ignore that mp, take this one: https://code.edge.launchpad.net/~sidnei/lazr-js/yui-3.0.0-redux/+merge/13743
<EdwinGrubbs> sidnei: what happened?
<sidnei> EdwinGrubbs: the order i merged the two base branches made it hard to generate a meaningful diff
<sidnei> EdwinGrubbs: this one has a link to a paste with the right diff.
* EdwinGrubbs changed the topic of #launchpad-reviews to: on call: Edwin || reviewing: sidnei || queue: [] || This channel is logged: http://irclogs.ubuntu.com/
<EdwinGrubbs> sidnei: how did you create the diff? I would like one that ignores whitespace changes, so I can focus on what really changed.
<sidnei> EdwinGrubbs: bzr diff -rancestor:../jstestdriver-support 
<sidnei> EdwinGrubbs: and then prune the 10k lines of the new YUI :)
<sidnei> and the yui-patch.js file, which is copied verbatim from upstream jstestdriver
<sidnei> i guess i need --diff-options
<EdwinGrubbs> ugh
<sidnei> yeah, ugh :(
<EdwinGrubbs> sidnei: don't worry about getting a new diff, I see why diff was showing me more lines than I thought it should. You moved some javascript from the top of the file to the bottom. I'll handle that change.
<sidnei> ok, thanks!
<sidnei> EdwinGrubbs: i need to go to a funeral, but will be back in a bit. have fun!
<EdwinGrubbs> sidnei: I'm sorry to hear that.
<sidnei> EdwinGrubbs: yeah. not a close person, but it sucks anyway. im happy as long as isn't mine *wink*
#launchpad-reviews 2009-10-22
<al-maisan> Good morning 
* al-maisan changed the topic of #launchpad-reviews to: on call: al-maisan || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/
<adeuring> al-maisan: could you please review this MP: https://code.edge.launchpad.net/~adeuring/launchpad/bug-457475-udev-device-id/+merge/13771 ?
<al-maisan> adeuring: yep .. will get to it in a few minutes.
<adeuring> al-maisan: thanks!
* al-maisan changed the topic of #launchpad-reviews to: on call: al-maisan || reviewing: - || queue: [abel] || This channel is logged: http://irclogs.ubuntu.com/
<adeuring> al-maisan: just noticed that I should add one more tests....
<al-maisan> adeuring: that's fine .. please let me know when the branch is ready.
* al-maisan changed the topic of #launchpad-reviews to: on call: al-maisan || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/
<adeuring> al-maisan: sure. sorry for the mess....
<al-maisan> no problem :)
<adeuring> al-maisan: done. See the comment in the MP
<al-maisan> adeuring: OK .. thanks.
* al-maisan changed the topic of #launchpad-reviews to: on call: al-maisan || reviewing: adeuring || queue: [] || This channel is logged: http://irclogs.ubuntu.com/
<al-maisan> adeuring: your branch is targeting db-devel .. is that right?
<adeuring> al-maisan: I'm an idiot... the target should be devel...
<al-maisan> adeuring: no worries .. just checking.
<al-maisan> adeuring: so, udev devices do not get IDs reported for them and you are making up an ID by counting the udev devices in the report?
<al-maisan> adeuring: how is that device_id for udev device used later on?
<al-maisan> *devices
<adeuring> al-maisan: yes. as written in the the MP, they are needed for a SQL table column, that they main purpose of that column is to help debugging the processing script. It makes it easy to figure out which device related to which record in the table
<al-maisan> adeuring: is the scope of the device_id limited to the respective HWDB report?
<adeuring> al-maisan: yes, the ID should be unique only within one submission
<al-maisan> adeuring: I see, thanks
<adeuring> al-maisan: I know that there should be a test how UdevDevice is used to populate the SQL tables, but there are qt least two more bugs that need fixing before I can do that.
<adeuring> s/qt/at/
<al-maisan> adeuring: hmm..
<al-maisan> adeuring: stupid question re.
<al-maisan>         self.assertEqual(1, device.id)
<adeuring> al-maisan: yes?
<al-maisan> how can you know that the id value is 1
<al-maisan> because it is the root/first device?
<adeuring> ah, the method setUp() defines self.root_device, and there is the dict item 'id': 1
<adeuring> al-maisan: the change of lines 635ff in test_hwdb_submission_parser.py
<adeuring> al-maisan: I meant the change in 3036ff in test_hwdb_submission_processing.py
<adeuring> (not my best day....)
<al-maisan> adeuring: I see .. thanks
<al-maisan> adeuring: r=me
<adeuring> al-maisan: thanks!
* al-maisan changed the topic of #launchpad-reviews to: on call: al-maisan || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/
<adeuring> al-maisan: fancy another review? https://code.edge.launchpad.net/~adeuring/launchpad/bug-458029-kernel-package-name-for-udev-submissions/+merge/13781
<al-maisan> adeuring: yes, just give me a few minutes.
<adeuring> al-maisan: thanks!
* al-maisan changed the topic of #launchpad-reviews to: on call: al-maisan || reviewing: - || queue: [adeuring] || This channel is logged: http://irclogs.ubuntu.com/
* al-maisan changed the topic of #launchpad-reviews to: on call: al-maisan || reviewing: adeuring || queue: [] || This channel is logged: http://irclogs.ubuntu.com/
<al-maisan> adeuring: I need to take a short break and will look at your branch immediately after I return.
<adeuring> al-maisan: sure, np
<al-maisan> adeuring: looking at your branch now
<adeuring> al-maisan: great!
<abentley> al-maisan: Could you please review https://code.launchpad.net/~abentley/launchpad/reply/+merge/13739 ?
<al-maisan> abentley: as soon as I finish with adeuring's branch
* al-maisan changed the topic of #launchpad-reviews to: on call: al-maisan || reviewing: adeuring || queue: [abentley] || This channel is logged: http://irclogs.ubuntu.com/
<abentley> al-maisan: Thanks!
<al-maisan> you are welcome
* danilos changed the topic of #launchpad-reviews to: on call: al-maisan || reviewing: adeuring || queue: [abentley, danilo] || This channel is logged: http://irclogs.ubuntu.com/
<danilos> al-maisan: hi, I hope you'll have enough time for another review after you are done with adeuring and abentley :)
<al-maisan> hello danilos, it may be a bit tight but I should be able to squeeze it in hopefully ;)
<danilos> al-maisan: cool, thanks :)
<al-maisan> adeuring: r=me
* al-maisan changed the topic of #launchpad-reviews to: on call: al-maisan || reviewing: - || queue: [abentley, danilo] || This channel is logged: http://irclogs.ubuntu.com/
<adeuring> al-maisan: thanks!
* al-maisan changed the topic of #launchpad-reviews to: on call: al-maisan || reviewing: abentley || queue: [danilo] || This channel is logged: http://irclogs.ubuntu.com/
<al-maisan> abentley: is this the MP in question: https://code.launchpad.net/~abentley/lp-production-configs/trunk/+merge/13783 ?
<al-maisan> abentley: sorry
<al-maisan> just read your qustion above
<abentley> al-maisan: no, it's https://code.launchpad.net/~abentley/launchpad/reply/+merge/13739
<al-maisan> yup
<al-maisan> abentley: r=me
<abentley> al-maisan: Thanks!
<abentley> barry: Could you do a ui review for me?  https://code.launchpad.net/~abentley/launchpad/reply/+merge/13739
* al-maisan changed the topic of #launchpad-reviews to: on call: al-maisan || reviewing: - || queue: [danilo] || This channel is logged: http://irclogs.ubuntu.com/
<al-maisan> danilos: is this the MP in question: https://code.edge.launchpad.net/~danilo/launchpad/bug-455771/+merge/13699 ?
<barry> abentley: sure if you can give me screen shots.  i have no way to run devel branches right now
<abentley> al-maisan: Actually, this needs UI review still, so the status should still be work-in-progress.
<danilos> al-maisan: no
<al-maisan> abentley: OK .. sorry .. will revise it.
<abentley> al-maisan: I've already done.
<al-maisan> abentley: thanks
<danilos> al-maisan: it seems my emails are still not getting through to LP: https://code.edge.launchpad.net/~danilo/launchpad/bug-458036/+merge/13787
<al-maisan> danilos: OK
* al-maisan changed the topic of #launchpad-reviews to: on call: al-maisan || reviewing: danilo || queue: [] || This channel is logged: http://irclogs.ubuntu.com/
<abentley> barry: Updated with screenshots.
<al-maisan> danilos: looks good, r=me
* al-maisan changed the topic of #launchpad-reviews to: on call: al-maisan || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/
* al-maisan changed the topic of #launchpad-reviews to: on call: - || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/
<danilos> al-maisan: thanks!
<al-maisan> danilos: does your branch need an extra ui review as well?
<danilos> al-maisan: I don't think so, I am just changing privileges on the page
<danilos> al-maisan: it has been UI reviewed before by beuno
<al-maisan> danilos: great .. I'll set it to approved then.
<danilos> al-maisan: ta!
<al-maisan> you are welcome
<barry> abentley: looking...
<barry> abentley: is there any way you can redo reply-2 and reply-4 to include the stuff above the comment box?  i'd like to see how that sits with the rest of the page.
<abentley> barry: The point is that it jumps to the reply box, and that's all you see on the page.
<abentley> barry: The reply box is the regular comment box.
<abentley> barry: We just populate it with the in-reply-to comment's text as a quote.
<barry> abentley: hmm.  in a tall browser window though there'll still be stuff above that, right?  i guess what i can't tell from this is the different between inline and separate page.  (one) point of inline is that you see the context with your reply.  i very much want this because i very much want to see the whole thing when i'm typing my reply
<abentley> barry: There's very little difference between inline and separate page, and that's why I've suggested that we should make our existing forms stuff easier to leverage asynchronously.
<barry> abentley: can you do just one full browser page shot?
<abentley> barry: http://people.canonical.com/~abentley/reply-5.png
<barry> abentley: beautiful!  ui*=me
<bac> beuno, mrevell:  why is the button on pop-up help 'Continue' instead of 'Close'?  was that a design decision or did it just happen?
<al-maisan> hello intellectronica, could you please do me a favour and review a branch of mine? I am off tomorrow and would like to wrap it up before I finish the day.
<beuno> bac, it's a good question. Sounds wrong, but I can't remember any specifics
<bac> it looks wrong to me
<al-maisan> intellectronica: just to whet your appetite, it added a method to IBugSet :)
<al-maisan> *adds
<intellectronica> al-maisan: oright, then :)
<mrevell> bac: I didn't choose the label so I'm not sure.
<al-maisan> intellectronica: https://code.edge.launchpad.net/~al-maisan/launchpad/clog-oops-452070/+merge/13789
<mrevell> bac: I can change it.
<al-maisan> intellectronica: thanks a million!
<bac> mrevell: that'd be great.
<bac> beuno: http://people.canonical.com/~bac/download5.png
<bac> mrevell: i'm just happy the button shows up now!
<mrevell> :)
<beuno> bac, looks nice. What if we collapsed them together?  "Release notes and changelog"
<beuno> maybwe probably don't need to much granularity
<bac> beuno: hmmm
<bac> beuno: i like having them separate.
<beuno> bac, why?
<bac> beuno: look what i found yesterday on meetup to introduce new features.  mrevell you might like it to.  http://people.canonical.com/~bac/meetup.png
<bac> beuno: so one twisty marked 'Release information' that expands to show release notes and changelog?
<mrevell> bac: I love it. We discussed something similar at the TL sprint
<beuno> bac, I love that way of showing new features and changes. I may even have an ajax movie showing that on LP  :)
<bac> notice that big red "Close" button!
<intellectronica> al-maisan: wouldn't it be easier to default preloaded_person_data to {} ? then you won't have to check that it's not None before trying to get the value
<bac> with an 'X' too
<beuno> bac, yes. Less clutter on the UI, and may make you make a decision you can't really make
<bac> beuno: ok, let me fix that up
<beuno> bac, it's a bit of an open question. I'm interested in why you want them seperate.
<al-maisan> intellectronica: yeah .. I am always a bit wary of default values like {} or [] because I keep forgetting how the scope rules work.
<al-maisan> intellectronica: None is clearer and immediately obvious..
<bac> beuno: i didn't find it cluttery.  but it's an easy experiment and you make a compelling argument.  let me try it and see
<intellectronica> al-maisan: fair enough. the difference is not big anyway
<al-maisan> intellectronica: yup .. using None saves me a few brain cells a day .. that's all :)
<intellectronica> al-maisan: is it really the case that you can only pass strings as the bug numbers to getByBugNubers?
<intellectronica> or is it just that storm/postgres coerce strings correctly?
<al-maisan> intellectronica: I did not test for integers .. let me try that.
<intellectronica> well, Bug.id is an integer, not a string
<intellectronica> if the comparison succeeds it's probably because postgres is not fussy
<al-maisan> intellectronica: yeah, you are probably right. I'll extend the tests in bug.txt to use normal numbers. That should work as well.
<intellectronica> al-maisan: i think it would be enough to change the tests to only use numbers, and remove (strings) from the documentation
<al-maisan> intellectronica: OK .. fine.
<al-maisan> by me
<intellectronica> al-maisan: is IStore(obj) equivalent to Store.of(obj) ?
<al-maisan> intellectronica: I understand IStore operates on the class
<al-maisan> whereas Store.of() uses objects
<intellectronica> oh right
<bac> beuno: http://people.canonical.com/~bac/download6.png -- i like
<intellectronica> but then, how does it know which store to adapt to?
<al-maisan> intellectronica: good question .. a (content) class maps to table I guess 
<al-maisan> intellectronica: http://pastebin.ubuntu.com/299118/
<intellectronica> right, but it could, in theory, be the same table in one of several stores
<al-maisan> intellectronica: master versus on of the slaves? Is that what you are aiming at?
<leonardr> allenap, since no one is on call maybe you'd be interested in reviewing my branch that fixes bug 451158?
<mup> Bug #451158: AttributeError: 'Credentials' object has no attribute 'authorizeSession' <launchpadlib :New> <https://launchpad.net/bugs/451158>
<leonardr> https://code.edge.launchpad.net/~leonardr/launchpadlib/oauth-in-restfulclient/+merge/13790
<al-maisan> intellectronica: *one of the slaves
 * al-maisan looks at the IStore implementation
<intellectronica> al-maisan: yeah. just my curiosity, b.t.w, i am not doubting that your code works, i just never saw this form
<allenap> leonardr: Yeah, okay. I'm in a meeting right now, but after, sure.
<al-maisan> intellectronica: I used it on a few occasions
<beuno> bac, sold
<leonardr> allenap, great
<intellectronica> al-maisan: r=me. hats off for the impressive improvement!
<al-maisan> intellectronica: thank you very much!
<beuno> bac, ui=me, and I'm off to lunch
<abentley> rockstar: Do you mind doing a UI review of https://code.edge.launchpad.net/~abentley/launchpad/reply/+merge/13739 ?
<allenap> leonardr: approved :)
<rockstar> abentley, sure.
* rockstar changed the topic of #launchpad-reviews to: on call: rockstar || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/
<rockstar> abentley, r=me
<rockstar> Er, ui=me
<rockstar> That's probably better shorthand.
<rockstar> abentley, do you know the bug that when you vote with the inline comment, your vote doesn't get inserted into the vote table?
<abentley> rockstar: Yes, I just can't bring myself to care.
<rockstar> abentley, I'll fix it.  I know exactly how to do it.
<rockstar> abentley, if there's currently a bug open, can you assign it to me?
<abentley> rockstar: I don't know if there's a bug open.
<rockstar> abentley, okay, I'll file one then.
 * rockstar lunches
<sidnei> rockstar: what's the procedure when you get a review with 'needs-fixing'? do you resubmit it with the incremental diff?
<sidnei> EdwinGrubbs: see my question above?
<sidnei> ^^
<EdwinGrubbs> sidnei: You would add a comment to the MP with your replies and also the incremental diff. "Resubmit" normally means creating an entirely new MP, and I assume that's not what you meant.
<sidnei> EdwinGrubbs: awesome, thanks!
<sidnei> EdwinGrubbs: i was wondering how the reviewers figure out that its out of 'needs-fixing' and should be reviewed again though.
<EdwinGrubbs> sidnei: I will receive an email with your new comment. You can also notify me directly, if it's urgent.
<sidnei> EdwinGrubbs: oh. so you're responsible for following up, since you did the first review. makes sense.
<jtv> rockstar: got two branches in the queue if you're interested in some light reviews
 * rockstar returns from lunch
 * jtv looks pretty pleased with his timing
<rockstar> jtv, links to mp?
<jtv> rockstar: oops, dinner just arrived.  Links follow:
<jtv> https://code.edge.launchpad.net/~jtv/launchpad/bug-457987/+merge/13774
<jtv> https://code.edge.launchpad.net/~jtv/launchpad/bug-458049/+merge/13779
<bac> hi rockstar -- can you do a review for me?
<rockstar> bac, absolutely.  Spit it on over.
<bac> https://code.edge.launchpad.net/~bac/launchpad/bug-419742/+merge/13807
<jtv> rockstar: thanks for the review
<rockstar> jtv, you are welcome.
<rockstar> bac, have you talked over with beuno why you can't do what he wants?
<bac> rockstar: not yet.  i've requested a UI review from him even though he approved the previous version
<rockstar> bac, yea, I was going to suggest that.  I think I'll just perform a code review for now then.
<bac> rockstar: i didn't realize the additional complication until some of the tests failed and i understood why we can't just whack it
<bac> rockstar: yes, that would be great
<rockstar> bac, yeah, that's pretty common.  beuno often finds solutions to satisfy the tests and himself.  He's clever like that.
<beuno> bac, you will want to kill me
<beuno> but
<beuno> that delete button
<beuno> makes me crazy
<bac> ?
<bac> beuno: oh, that
<beuno> "delete files
<beuno> floating in cyberspace
<bac> beuno: yeah, where would you like it?
<beuno> bac, how about we don't show it at all until we click on the checkbox?
<beuno> and
<bac> beuno: hmmm
<beuno> we show the button underneath the table
<beuno> or
<beuno> delete individual files and don't tell sinzui
<bac> beuno: those sound like good ideas.  but can i convince you to defer them to another branch?  i would like to get this one landed so i can move on to an OOPS that is affecting karmic
<beuno> bac, yes you can, it's not worst than it is now
<bac> beuno: no, that button is the same as the bad way i designed it a few years ago
<beuno> :)
<jtv> rockstar: still got that other branch on the queue if you feel like doing one more: https://code.edge.launchpad.net/~jtv/launchpad/bug-458049/+merge/13779
<rockstar> jtv, huh, thought I did that one already
<jtv> rockstar: you did the other one, for which thanks
<rockstar> Also, why the fuck are you awake?
<jtv> rockstar: look who's talking :-)
<rockstar> Hey man, if you're awake because you're also polyphasic, then I'm okay with that.  If you're awake just because, then man you crazy.
<jtv> rockstar: no secret that I'm crazy.
#launchpad-reviews 2009-10-23
* rockstar changed the topic of #launchpad-reviews to: on call: - || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/
<thumper> https://code.edge.launchpad.net/~thumper/lazr-js/multi-line-edit-class/+merge/13753
<thumper> https://code.launchpad.net/~thumper/launchpad/bmp-inline-commit-message/+merge/13814 for the bored
* adeuring changed the topic of #launchpad-reviews to: on call: adeuring || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/
<allenap> adeuring: Fancy a very short review? https://code.edge.launchpad.net/~allenap/launchpad/log-statement-none/+merge/13828
<adeuring> alsure
<adeuring> allenap: yes
<allenap> adeuring: Thanks.
<adeuring> allenap: this look good, overall, so r=me, with one condition: From time to time it is useful to have all SQL statements logged. I think we have some script which directly use a the DB API module. So: could you add the "SET log_statement..." stuff to the debugging wiki page?
<allenap> adeuring: Sure :)
<allenap> Thanks!
<henninge> adeuring: Hallo Abel!
<adeuring> hi henninge!
<henninge> adeuring: I have a shord little branch here for you. Can you please have a look at it?
<henninge> https://code.edge.launchpad.net/~henninge/launchpad/bug-458924/+merge/13831
<henninge> short
<adeuring> henninge: klar!
<henninge> ;-)
<adeuring> henninge: could you change the description of :param advance: in the __init__ doc string so that we know that the value N means N seconds?
<henninge> adeuring: sure
<adeuring> henninge: thanks. I am also wondering if we should add some tests.
<henninge> adeuring: testing test code?
<adeuring> henninge: sure ;) I have branch ready for review that allows to run docstring tests in this file.
<henninge> adeuring: so add a docstring test?
<henninge> adeuring: I thought we were removing these in-line doctests?
<henninge> maybe it's different for test code.
<adeuring> henninge: do we? I think the come very handy for these test helpers: YOu can see how they are working and that they are working
<henninge> adeuring: there is lib/lp/testing/tests
<henninge> adeuring: maybe we should add lib/lp/testing/doc ?
<adeuring> henninge: OK, that's the other option. Would be fine for me.
<henninge> adeuring: there is only one other docstring test in the file.
<adeuring> henninge: but soon another one (assuming that the doc string test is accepted by a reviwerr): https://code.edge.launchpad.net/~adeuring/launchpad/bug-458160-fix-submissionparser-checkconsistency/+merge/13827
<adeuring> henninge: there is also code that invokes the doc string tests. lib/lp/testing/tests/test_inlinetests.py#
<henninge> adeuring: ok, I'll add the docstring test because that is easier now. I may be wrong about docstring tests being deprecated but if you reviewer complains, let me know about it.
<adeuring> henninge: OK. Or we both wait for barry to review our branches ?
<henninge> adeuring: or that.
<henninge> adeuring: I am in no hurry about this branch.
<adeuring> henninge: OK, so let's wait ;)
<henninge> adeuring: ok, I added the docstring test and ran it sucessfully using your branch.
<henninge> pushing it now
<adeuring> henninge: great!
<salgado> adeuring, looks like you need a review yourself?
<adeuring> salgado: yes, as well as henninge. (his branch could benefit from the file lib/lp/testing/tests/test_inlinetests.py in my branch. which allows to run doc test strings in lp.testing)
<leonardr> adeuring, would you take a look at https://code.edge.launchpad.net/~leonardr/launchpad/test-launchpadlib/+merge/13834 ?
<adeuring> leonardr: sure
* adeuring changed the topic of #launchpad-reviews to:  on call: adeuring || reviewing: leonardr || queue: [] || This channel is logged: http://irclogs.ubuntu.com/
<adeuring> leonardr: r=me
* adeuring changed the topic of #launchpad-reviews to: on call: adeuring || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/
<salgado> adeuring, ok, I'll do yours
<adeuring> salgado: thanks!
<salgado> it's huge
<salgado> adeuring, what diff should I review?  the one in your cover letter or the one auto generated?
<adeuring> salgado: the one in the cover letter. The auto generated one contains changes from a previous, not-yet-landed branch.
<salgado> ok, cool
<adeuring> (But the previous branch has benn reviewed)
<leonardr> allenap, here's the merge proposal: https://code.edge.launchpad.net/~leonardr/lazr.restfulclient/set-credentials/+merge/13837
<allenap> leonardr: Cool.
<gmb> adeuring: Do you have time to review https://code.edge.launchpad.net/~gmb/launchpad/make-me-super-bug-438985/+merge/13840 for me?
<adeuring> gmb: sure
<gmb> adeuring: Thanks.
* sinzui changed the topic of #launchpad-reviews to: on call: adeuring || reviewing: - || queue: [sinzui] || This channel is logged: http://irclogs.ubuntu.com/
<sinzui> adeuring: do you have time for a branch that moves a lot of code to make two small UI changes? https://code.edge.launchpad.net/~sinzui/launchpad/package-link-validation-3/+merge/13844
<adeuring> sinzui: just started another review, but when that one is finished, sure
<sinzui> beuno: Converting the pacakging Delete link button to an image was easy: http://people.canonical.com/~curtis/dsp-packaging.png
<beuno> sinzui, neat
<EdwinGrubbs> adeuring: I have an easy and fairly urgent mp: https://code.edge.launchpad.net/~edwin-grubbs/launchpad/bug-458169-distroseries-timeout/+merge/13851
<bac> adeuring: scary trivial and not urgent at all:  https://bugs.edge.launchpad.net/launchpad-registry/+bug/436508
<mup> Bug #436508: Incorrect capitalisation in "SSH Keys" on person index <post-3-ui-cleanup> <Launchpad Registry:In Progress by bac> <https://launchpad.net/bugs/436508>
<adeuring> EdwinGrubbs: I'm currently review a branch from Curtis; might take some time... After this, taht's fine, but you also ask salgado-lunch or barry
<adeuring> bac: ^^^
<bac> EdwinGrubbs: i'm on it!
<EdwinGrubbs> bac: thanks
<bac> EdwinGrubbs: trade?  mine is really an rs
<EdwinGrubbs> bac: sure
<EdwinGrubbs> bac: r=me
<bac> EdwinGrubbs: i get a test failure on productseries-views.txt
<bac> let me make schema to ensure i have a clean starting point
<EdwinGrubbs> that should fix it
<bac> EdwinGrubbs: still failing
<bac> can you re-run
<bac> ./bin/test -vv -t 'distribution-soyuz.txt|productseries-views.txt'
<bac> http://pastebin.ubuntu.com/299844/
<EdwinGrubbs> bac: it passed for me. Let me run make schema.
<bac> EdwinGrubbs: hmm.  karmic?
<EdwinGrubbs> bac: I'm on karmic now
<adeuring> sinzui:  r=me
<bac> EdwinGrubbs: shouldnt' make a difference.  me too btw
<sinzui> fab
<leonardr> adeuring, your attention to https://code.edge.launchpad.net/~leonardr/launchpadlib/web-root/+merge/13856, please
<adeuring> leonardr: sure
<EdwinGrubbs> bac: it passed for me after running make schema. Oh, did you merge my branch into trunk. I haven't done that since yesterday.
* adeuring changed the topic of #launchpad-reviews to:  on call: adeuring || reviewing: leonardr || queue: [] || This channel is logged: http://irclogs.ubuntu.com/
<bac> EdwinGrubbs: i did
<EdwinGrubbs> bac: ok, it fails for me now.
<bac> it looks like ProductSeriesUbuntuPackagingView has changed.  i wonder if that was sinzui or thumper?
<sinzui> me
<sinzui> I updated the view to prevent it from creating duplicate packages.
<bac> sinzui, EdwinGrubbs: so not it looks like it is having fits with the cached currentseries
<sinzui> bac: EdwinGrubbs: I discovered that method while writing the test to verify that the view offers a new link when the current series changes.
<EdwinGrubbs> bac, sinzui: oh, it's because the test is changing the status of a distroseries instead adding a new series, which clears the cache.
<bac> EdwinGrubbs: yep
<sinzui> right.
<sinzui> EdwinGrubbs: This happen every 6 months when we switch the focus of development. The forms control the order that series progress through their status. Something cannot leap into a state
<EdwinGrubbs> bac, sinzui: I could have the distroseries status setter call a new self.distribution.clearCache() method. Would that be too kludgy?
<sinzui> EdwinGrubbs: no, that is the same pattern we use the preferredemail.
<bac> EdwinGrubbs: are there other circumstances where the cache needs clearing?
<EdwinGrubbs> sinzui, bac: Actually, I should just take off the cache on currentseries, since serieses is cached, currentseries will just loop through the list in memory, so that shouldn't be too expensive.
<sinzui> EdwinGrubbs: is serieses sorted the same way?
 * bac sends evil thoughts to whoever first used 'serieses'
<EdwinGrubbs> sinzui: serieses is sorted by the version attribute.
<EdwinGrubbs> bac: don't you mean that you send evil thoughtses?
<bac> eviles
<sinzui> shut'Up!
<bac> sinzui: was it you?
<bac> no, it pre-dates all of us
 * sinzui thought bac was doing a vicky quote
<bac> nah, i don't quote VP much.  too much to type
<adeuring> leonardr: could you add shorts for the new functions lookup_service_root() and loookup_web_root()?
<adeuring> ...i mean shorts tests...
<bac> sinzui: can i get an rs=sinzui for http://pastebin.ubuntu.com/299864/
<sinzui> Is that good grammar I see?
<bac> real good
<bac> i did good
<adeuring> leonardr: what is the reason that you simply return service_root or web_root, if these avlues don't appear in the dictionaries?
<sinzui> bac r=me
<bac> sinzui: you serious?
<sinzui> bacs good example has enbiggened all of us
<sinzui> bac: yes
<sinzui> there may be a bug for that
 * bac runs the tests and takes cover under the sofa
<bac> EdwinGrubbs: in your branch could you make two small fixes to browser/productseries.py?
<bac> 1) s/OBSOLETE"/OBSOLETE  - the extra quote messes up emacs fiercely
<bac> 2) s/staic/static
<EdwinGrubbs> bac: I've pushed up a version with the productseries changes and I removed the currentseries cache, which makes the test pass.
<bac> EdwinGrubbs: ok.  i'll grab it
<leonardr> adeuring: i do. if the value doesn't appear in the dictionary, it returns the 'default', which is service_root or web_root, depending
<adeuring> leonardr: yes, but why? What is the reason for having "something" == lookup_web_root("something")? Wouldn't an exception make sense?
<leonardr> adeuring: i'm sorry, i misunderstood your question
<leonardr> an exception doesn't make sense because the most common value to fail a lookup is a URL
<adeuring> leonardr: could you explain a bit?
<bac> EdwinGrubbs: r=bac.  thanks!
<leonardr> sure, here's a test i just wrote
<leonardr>         self.assertEqual(
<leonardr>             uris.lookup_service_root("edge"), uris.service_roots['edge'])
<leonardr>         self.assertEqual(
<leonardr>             uris.lookup_service_root("http://some-other-server.com"),
<leonardr>             "http://some-other-server.com")
<adeuring> leonardr: Ah, I see. Nevertheless, I am a bit concerned about this: Let's assume that we have a LP client where you can choose the sever via an option, like "-s egde". If this is passed to lookup_web_root, the nonsensical value is simply returned.
<leonardr> if you want me to make sure that the incoming string is either a valid alias or a url, i can do that
<leonardr> it won't make much difference
<adeuring> leonardr: yes, that would be great. I suspect that you'd have other wise to chase errors with bad URLs elsewhere in the a program that uses lplib
<leonardr> adeuring, take another look
 * adeuring is looking
<leonardr> adeuring: the diff doesn't show the difference, pull the branch
<adeuring> leonardr: did that already ;)
<leonardr> ok
<adeuring> leonardr: thanks for the changes! r=me
<leonardr> all right
* adeuring changed the topic of #launchpad-reviews to: - || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/
<deryck> rockstar, ping
<rockstar> deryck, yo.
<deryck> rockstar, hey, man.  Could I ask a review of you?  *Very* small Windmill test update.  I just have a pastebin diff.
<rockstar> deryck, I'm on the phone right now, but yes, I can get to it when I'm off.
<deryck> rockstar, thanks.  http://pastebin.ubuntu.com/299958/
<deryck> rockstar, fix for Bug #459144 
<mup> Bug #459144: test_bug_privacy_settings failing in Windmill <Launchpad Bugs:In Progress by deryck> <https://launchpad.net/bugs/459144>
<rockstar> deryck, is there an mp for it?
<deryck> rockstar, no.  if you want me to file one I can, but I branched from a large branch of windmill test moving around, so diff will be off.
<rockstar> deryck, that's fine, since I have the pastebin.  I just want somewhere to record my approve I guess.
<deryck> rockstar, yeah, I prefer doing mp's, but in the case, it just seemed easy and out of mp
<deryck> rockstar, so you're cool with the change then?
<rockstar> deryck, yes.
<deryck> rockstar, cool, thanks
<abentley> flacoste: Thanks for your review.  Could you also look at https://code.edge.launchpad.net/~abentley/launchpad/parse-binary/+merge/13600 ?
<EdwinGrubbs> sidnei: ping
<flacoste> abentley: done
<EdwinGrubbs> :q
<abentley> flacoste: Thanks again!
<sidnei> EdwinGrubbs: aye
<EdwinGrubbs> sidnei: I haven't run "make" on lazr-js in a long time, so I may be missing something. I get an error that it can't find a distribution for zc.recipe.testrunner.
<sidnei> EdwinGrubbs: yeah, tis not in the download-cache. which begs the question: where is the download-cache for lazr-js supposed to be?
<sidnei> EdwinGrubbs: i ran bin/buildout buildout:install-from-cache=false manually to get it downloaded
<sidnei> EdwinGrubbs: i guess it's a question for BjornT really, since it's his branch that buildoutified it
<EdwinGrubbs> sidnei: now, I get this error: 
<EdwinGrubbs>   File "bin/build", line 41, in <module>
<EdwinGrubbs>     import lazr.js.build
<EdwinGrubbs> sidnei: is this just an intermediate branch, or should it be possible to get the examples to work?
<sidnei> EdwinGrubbs: yeah, that's a problem from BjornT's branch too. if you remove any python-lazr* packages installed system-wide it should work
<sidnei> EdwinGrubbs: i just managed to get the examples to work yes, if you have the latest rev.
<EdwinGrubbs> sidnei: I got it to work. Is there a bug open for the conflicts it has with a locally installed lazr?
<sidnei> EdwinGrubbs: there's some comments on the review for the buildoutification branch. i assume BjornT will fix that when monday comes, before he merges his branch.
<EdwinGrubbs> sidnei: I also was curious if there was any current problem we are trying to solve by calling renderUI with Y.after(), or is this just a new standard for YUI3.
<sidnei> EdwinGrubbs: yes, there is. renderUI is called before the WidgetStdMod extension is initialized, and we were trying to override the WidgetStdMod.BODY. it's much easier and cleaner to use after and do it after it has been initialized.
<EdwinGrubbs> ahhh
<sidnei> EdwinGrubbs: also, if you do that, subclasses dont have to call superclass.<method>.apply(), apparently.
<sidnei> EdwinGrubbs: since calling on .after() just chains the call to after the previous call, following the class hierarchy.
<EdwinGrubbs> sidnei: r=me
<sidnei> EdwinGrubbs: thank you!
<sinzui> bac: did your distribution.series banch succeed?
<bac> sinzui: which is that?
<bac> sinzui: oh, my serieseses -> series branch?  no i had some test failures in registry which i fixed
<bac> sinzui: i was running the bugs tests to see how they fared
<bac> sinzui: then i was going to wait for edwin's more important branch to land so there would be no conflicts
<bac> sinzui: i'm still waiting on my branch from yesterday to land.  i'm at #5 now
<bac> sinzui: argh.  i see there are three more CPs in PQM gumming up the works.
<bac> sinzui: care to do a quick review of https://code.edge.launchpad.net/~bac/launchpad/bug-436259-add-member/+merge/13875
<sinzui> bac: I can do the review in 30 minutes. I need to deliver my daughter to soccer practice
<bac> sinzui: per my whining above you'll see i'm in no rush.  have fun.
#launchpad-reviews 2009-10-25
 * kfogel is away: zzz
#launchpad-reviews 2010-10-25
* 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
<allenap> henninge: Fancy an easy one to start?
<allenap> https://code.edge.launchpad.net/~allenap/launchpad/remove-propertycache-adapters-bug-628762/+merge/39249
<henninge> allenap: sure! ;)
<allenap> henninge: Thanks :)
<henninge> allenap: He! 872 lines ...
<henninge> ;-)
<allenap> henninge: It's nearly 900 lines, but almost all of it is search-n-replace.
<allenap> henninge: The interesting files are propertycache.py and propertycache.txt.
<henninge> allenap: np, I'll be fine
<henninge> allenap: You are the native speaker but is the "in" not misplaced here:
<henninge> 557	+The property cache for an object can be cleared by passing in the
<henninge> 558	+object to `clear_property_cache()`.
<henninge> "by passing the object into ..."
<henninge> ?
<allenap> henninge: I think both work, but your suggestion is better.
<henninge> allenap: Also, I first thought this was the same test twice.
<allenap> henninge: I think I can trim it to make it look a little different.
<henninge> allenap: or just make it clearer in the narrative that this is an alternative use of the same function.
<allenap> henninge: Okay.
<henninge> Wow, the new code looks much simpler. Good job!
<allenap> henninge: Cheers :) I should have done it this way from the start. Never mind, I learnt a lot... the hard way.
<henninge> allenap: that really was an easy one. Thanks! r=me
<allenap> henninge: Cool, thanks
<bac> hi henninge
<henninge> Hey bac! ;)
<bac> henninge: hey we're in testfix mode and i wonder if you'd sanity check this patch: http://pastebin.ubuntu.com/519603/
<bac> normally i'd think the geo data has changed out from under us.  but best i can tell it hasn't, as we're getting the GeoIP Lite data from our PPA
<bac> henninge: so, do you think the approach here of accepting both until we sort it out is sane?
 * henninge looks at the failure
<henninge> bac: oh yes, I think that is sane.
<bac> ok, well i'll just land it rs=bac then and not implicate you
<henninge> bac: I have never worked with geopip before.
<bac> only minimal for me
<henninge> but if you say "I know we may get one of these two values and we have yet to figure out why", I say your patch is a good way to get us going again.
<henninge> bac: r=me in that case.
<bac> henninge: that is what i'm saying
<bac> thanks
<bac> hi jtv.
<jtv> hi bac!
<jtv> happy end-of-buddhist-lent day
<henninge> Hi jtv!
<jtv> Hi henninge.  I'm not here.
<lifeless> henninge: https://code.edge.launchpad.net/~lifeless/launchpad/edge/+merge/39233
 * henninge looks
<henninge> 186	-On launchpad.net, the version and revision numbers are presented only in an
<henninge> 187	-HTML comment.
<henninge> lifeless: How is that handled now? Are they always visible?
<henninge> I don't mind, just curious.
<lifeless> they are in a <-- section
<lifeless> hit ctrl-U on launchpad.net
<lifeless> down the bottom
<lifeless> but also I've put the revno on prod after chatting with mpt
<lifeless> (by taking it outside a tal:condition)
<henninge> Ok, I knew that. I just thought it had changed because the test is deleted. But now that I looked closer at it, the test is not about them being in a comment but about them *not* being displayed.
<henninge> So that's ok. ;)
<henninge> lifeless: thanks for removing the "bleeding edge" ... ;-)
<lifeless> I haven't done a full test run yet, this may have some breakage, but shoud be minor
<henninge> lifeless: this still mentions edge. Is that OK?
<henninge> 921	-         - is_edge/is_lpnet etc (thunks through to the config)
<henninge> 922	+         - server.is_edge/is_lpnet etc (thunks through to the config)
<lifeless> bah, I missed that; thanks
<lifeless> it should be server.lpnet
<lifeless> IIRC
<henninge> lifeless: also, you seem to have removed all references to "staging" from the on-edge script but left edge in there.
<henninge> I think that script needs a complete renaming now.
<lifeless> henninge: I filed a bug about it
<lifeless> uhm, I think you're reading the hunks wrongly
<henninge> ah, cool ;)
<lifeless> We should, i think, delete the script entirely
<henninge> possibly
<henninge> why am I reading them wrongly?
<henninge> or rather "how"
<lifeless> it still calls staging_revision = on_staging()
<lifeless> its just that theres no longer a use case for 'just edge'
<lifeless> because edge is approximately gone
<lifeless> + staging_revision = get_staging_revision()
<henninge> yes, I saw that but the command line option for staging is still there.
<lifeless> - '--staging-only', action='store_true',
<lifeless> 1022	- help="Only show revisions not on staging. Do not consult edge.")
<lifeless> its gone :)
<henninge> 1023	+        '--edge', action='store_true',
<henninge> 1024	+        help="Show revisions on edge.")
<lifeless> thats for edge
<henninge> While we still have it?
<lifeless> right
<henninge> ah!
<lifeless> first we remove all the differences
<lifeless> then we put a redirect in place
<lifeless> and a that point we need qa stuff to move from edge to qastaging
<lifeless> so theres future tweaks to do here
<henninge> So, this script was just about easily figuring out how far you revision had propgated through the various branches.
<lifeless> yeah
<henninge> that may not be needed anymore now.
<lifeless> so devel -> stable -> deploy qastaging -> qa'd -> lpnet
<lifeless> and db-devel -> db-stable -> staging -> ??? -> monthly release
<henninge> Speaking of which: I just created a draft for the 2011 release calendar (see your mail). We will still be having monthly releases, right?
<lifeless> yes
<lifeless> until we get db flexability in place
<henninge> ok
<henninge> lifeless: r=me
<lifeless> thank you
<allenap> henninge: Do you fancy another easy one?
<henninge> allenap: please! ;)
<allenap> henninge: https://code.edge.launchpad.net/~allenap/launchpad/ditch-get-bug-notifications-recipients-bug-659085-devel/+merge/39277
<allenap> henninge: It's big... at first glance.
<allenap> henninge: But it's actually almost all reviewed. Just a little diff (in the description) to review.
* abentley changed the topic of #launchpad-reviews to: On call: henninge, abentley || Reviewing: -, - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<henninge> allenap: I have never seen a doc test import from a unit test. Is that a good(tm) practice?
<allenap> henninge: I don't think it's a problem, but it is a bit odd. Certainly BugTaskSearchBugsElsewhereTest.__init__() smells bad.
<allenap> henninge: Arguable those helper methods should have been put in a mixin or just a separate class, but as it is it's reasonably easy to figure out what's going on. I don't think there's much harm in it.
<henninge> Argh! Now I get what that is doing.
<henninge> allenap: Luckily it's not part of this review ... ;-)
<allenap> henninge: Yes, that's what I thought :)
<henninge> allenap: could you call the result "naked_bugtasks" here?
<henninge> +        bugtasks = [removeSecurityProxy(bugtask) for bugtask in bugtasks]
<allenap> henninge: Sure.
<henninge> I think there was an agreement to clearly mark naked entities as such ...
<henninge> allenap: r=me for the handy diff. ;-)
<allenap> henninge: Thanks :)
<rockstar> henninge, abentley, can I send a review to one of you?
<henninge> rockstar: I am almost done. Sorry.
<henninge> In fact, I am done ... ;)
<rockstar> henninge, it's VERY short.  :)
* henninge 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
<henninge> rockstar: sure
<henninge> ;-)
<henninge> short & fun - always!
<leonardr> abentley, pretty easy branch for your consideration: https://code.edge.launchpad.net/~leonardr/lazr.restful/fix-unicode-error/+merge/39279
<abentley> leonardr: Okay.
* abentley changed the topic of #launchpad-reviews to: On call: abentley || Reviewing: leonardr || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<abentley> leonardr: you have conflicts.
<rockstar> abentley, can you also review my branch?
<rockstar> abentley, https://code.edge.launchpad.net/~rockstar/launchpad/fix-queue-permissions/+merge/39278 plz.
<abentley> rockstar: Sure.  I thought henninge was looking at it.
<henninge> rockstar, abentley: I am!
<rockstar> henninge, oh!  I thought you were being sarcastic.  :)
<henninge> rockstar: not my nature ... :)
<leonardr> abentley: argh, coordinating with benji
<abentley> leonardr: I am a bit confused by line 187; shouldn't that have a unicode escape, not \xc3\xa7?
<henninge> rockstar: Have you started your own Canonical spin-off? A bit obvious, don't you think?
<henninge> 136	-# Copyright 2009-2010 Canonical Ltd.  This software is licensed under the
<henninge> 137	+# Copyright 2009-2010 aanonical Ltd.  This software is licensed under the
<henninge> ;-)
<rockstar> henninge, huh.  I'll fix that...
<leonardr> abentley: no, because the test suite (i think, benji might know better) decodes the unicode string
<abentley> leonardr: Did you consider making this a unit test?
<benji> actually it's the other way around; it doesn't attempt to decode the string, so you just get the bytes
<henninge> rockstar: r=me ;-)
<rockstar> henninge, thank you sir!
<abentley> benji: so that's a platform-specific value, then?
<leonardr> abentley, that's a huge pain, but i'll try
<abentley> benji: looks like utf-8 to me, not utf-16, which AIUI is the internal representation on Ubuntu python.
<benji> abentley: theoretically it could vary, but practically shouldn't
<abentley> benji: I don't think pretty sure it's the plain bytes of the unicode string, because on Ubuntu, the internal representation is utf-16, and that's utf-8.
<abentley> benji: I think it's more likely that it was encoded as utf-8.
<leonardr> if i'm able to make it a unit test i should also be able to control the encoding
<allenap> salgado: Are you available to do UI reviews? It's not urgent.
<abentley> leonardr: true, but as a unit test, you can just compare against a unicode string.
<benji> abentley: I forget the exact details, but Python doesn't use utf-8 or -16 internally, it uses 16 or 32-bits per character (depending on how it was compiled)
<salgado> allenap, not really, I'm attending some UDS sessions remotely while I wait for my flight to the US
<allenap> salgado: Okay, thanks anyway. Have a good trip :)
<salgado> thanks. :)
<abentley> benji: In this bug report, Adam Olsen asserts that it uses utf-16, not UCS2: http://bugs.python.org/issue3297
<benji> abentley: yep I was thinking of UCS-2/UCS-4, but it does indeed use UTF-16/UTF-32
<abentley> leonardr: I'm not saying you have to use a unit test, but in Launchpad, the policy is that doctests should be testable documentation.
<gary_poster> abentley: five-line-diff version-bump review when you have a moment: https://code.edge.launchpad.net/~gary/launchpad/storm-0.18/+merge/39286
<abentley> gary_poster: r=me
<gary_poster> thanks abentley
<leonardr> abentley: behold: http://pastebin.ubuntu.com/519760/
<abentley> leonardr: r=me.
* 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
<leonardr> benji: i named launchpadlib 1.7.0 in anticipation of your kwallet branch. you don't need to call this branch 1.8.0
<leonardr> we usually do 'import cStringIO as StringIO'
<benji> leonardr: (assuming that comment was for me) ewww  :)
<leonardr> benji: actually, we do 'from cStringIO import StringIO', if that makes you feel better
<benji> much better
<leonardr> benji, why do you have callback_called as a list instead of a boolean? i imagine it has something to do with the possibility that the callback might be called multiple times?
<leonardr> but if so, wouldn't you check the list to make sure it was only called once?
<benji> leonardr: nope, it's because I can't rebind callback_called, just mutate it
<leonardr> ah, ok
<benji> perhaps a note to that effect is warrented
<leonardr> benji: the code is weird. bool([False]) is True
<leonardr> i think you should check that the list is [True]
<benji> mmm, yeah that sounds like a bug
<leonardr> bool([]) is False, but your use of booleans _inside_ the list strongly implies that you are checking those values
<leonardr> you could have the callback append object() to the list, and make sure the list was nonempty
<benji> leonardr: oh, well it's not a bug per se; I'm signaling truth by there being something lin the list
<benji> yeah, I'd say appending None would be the most straight-forward
<leonardr> +1
<leonardr> benji: can you run this code in bin/py on your kwallet branch and see if it works for you? i get an error but it might be a version mismatch
<leonardr> >>> from launchpadlib.launchpad import Launchpad
<leonardr> >>> f = Launchpad.login_with("bar baz", "edge")
<benji> sure
<benji> leonardr: I get this: TypeError: __init__() takes at most 5 arguments (6 given)
<benji> I'll dig into it.
<leonardr> ok, cool
<leonardr> benji, review sent. i'm very glad that you were able to fix keyring so that we could use it
<benji> leonardr: oh, I meant to mention that I was able to do away with the Qt widget bit (I still had to create a Qt "application" but that's not too bad)
<leonardr> benji: and that's in keyring, not in our code, right?
<benji> right
<benji> I expect Tarek to make a release of keyring this week that includes my changes.
<leonardr> great, where's it coming from now?
<leonardr> ie. if i didn't get that TypeError would i be using your code?
<leonardr> or what would happen?
<leonardr> benji -^
<benji> leonardr: you'd be using the version before my changes (because that's what buildout gets from PyPI), which probably couldn't build the GNOME bits, and it probably couldn't build the encyption bits, so more than likely you'll just be using a file
<leonardr> benji: ok, sometime tomorrow i'd like to get the whole thing running with your code so i can do an end-to-end test
<benji> sounds good
<jcsackett> abentley: have time for a review?
<jcsackett> https://code.edge.launchpad.net/~jcsackett/launchpad/hidden-project-configuration-636000/+merge/39306
* 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: Prolly not.
<jcsackett> abentley: fair. i wouldn't be able to get in to merge today anyway.
* jcsackett 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
* StevenK changed the topic of #launchpad-reviews to: On call: abentley || Reviewing: - || queue: [StevenK] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<StevenK> abentley: ^ If you have time
<abentley> StevenK: Sorry, I've EODed.
* 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
* 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
<StevenK> No fair dropping me from the queue
* StevenK changed the topic of #launchpad-reviews to: On call: - || Reviewing: - || queue: [StevenK] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<abentley> StevenK: It's an on-call review queue.  No one has agreed to review you, so you can't be in the queue.
<thumper> StevenK: paste the link
<StevenK> thumper: https://code.edge.launchpad.net/~stevenk/launchpad/cronscript-idsjob-testfix/+merge/39321
#launchpad-reviews 2010-10-26
<thumper> https://code.edge.launchpad.net/~thumper/launchpad/fix-mailman-excessive-queries/+merge/39335 anyone?
<bac> hi thumper, i'll look at it
<thumper> bac: thanks
<thumper> check out this: 6278  OOPS-1759XMLP1  MailingListApplication:MailingListAPIView
<thumper> 6278 statements
<bac> hi thumper -- really interesting branch...amazing savings
<thumper> bac: should be
<bac> i got a little caught up with  acknowledgeMessagesWithStats thinking it updated all in the message_set
<bac> but then i figured it out
<bac> only one issue.  i think you meant  acknowledgeMessagesWithStats to be  acknowledgeMessagesWithStatus
<thumper> ah... yeah
<thumper> copy and paste FTW, at least the code is consistent
<thumper> fixed typo, and pushing
<thumper> bac: thanks
* jtv changed the topic of #launchpad-reviews to: On call: - || Reviewing: - || queue: [StevenK, jtv] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jtv> adeuring: perhaps you could review my branch?  I'd prefer not to wait for an OCR because I believe the next OCR is me.  :-)
<adeuring> jtv: give me 30-40 minutes for lunch, then I'll look
<jtv> adeuring: that's wonderful, thanks!
* leonardr changed the topic of #launchpad-reviews to: On call: leonardr || Reviewing: - || queue: [StevenK, jtv] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jtv> leonardr: I'm expecting Abel back in a few; he says he can look at mine if needed.
<leonardr> jtv: ok, thanks
<leonardr> StevenK, which branch do you want reviewed? i don't see it
<StevenK> leonardr: It already has been, sorry.
<leonardr> ok, np
<leonardr> jtv, want me to take care of yours?
* leonardr changed the topic of #launchpad-reviews to: On call: leonardr || Reviewing: - || queue: [jtv] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jtv> leonardr: time is short now, so yes please!
<StevenK> I threw it at ec2 from davidm's hotel room, but was then unable to connect to IRC from my own room
<leonardr> jtv: ok
<jtv> thanks
<adeuring> leonardr, jtv: I'm more or less done with the review
<jtv> ah!
<jtv> In that case, thanks leonardr but your services are no longer required.  :-)
<leonardr> great
* leonardr changed the topic of #launchpad-reviews to: On call: leonardr || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<adeuring> jtv: only a minor issue: doc/potmsgset.txt has the new text "+The "suggestive templates" cache is up to date." (line 21). shouldn't this be something like "update the cache"?
<jtv> adeuring: well it's supposed to be narrative, not code comments.  (And for a code comment, that would be a bad one. :-)
<jtv> It's really just stating the expected situation, not anything interesting that the test does.
<adeuring> jtv: well, ok. But I inderstand this as a statement "the cache indeed up to date", while you define a helper function which does an update. What about a comment inside the definition od the function?
<jtv> adeuring: any ideas on what it should say?
<jtv> adeuring: ah, this is the one doctest where I need the function more than once.
<adeuring> jtv: argh, I missed the existing doc string... never mind. Let's leave it as it is
<jtv> Well _of course_ there's a docstring.  :-)
<adeuring> jtv: yeah... doesn'r seem to be my best day...
<jtv> Well you're helping me out with a review.  That's definitely a good deed for the day.
<adeuring> anyway, r=me then. Just a parnoid question: This cache is already populated via cron job or whatever?
* leonardr changed the topic of #launchpad-reviews to: On call: leonardr || Reviewing: jcsackett || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<leonardr> jcsackett, i'm going to review https://code.edge.launchpad.net/~jcsackett/launchpad/hidden-project-configuration-636000/+merge/39306
<jcsackett> leonardr: sounds good. i was going to throw it in the queue sometime this morning anyway. :-)
<leonardr> all right
<jtv> adeuring: thanks again
<adeuring> jtv: welcome. just a paranoid question:; this cache is updated in production on a regular basis, I assume?
<jtv> adeuring: yes, daily.
<jtv> We had a replication problem with it last cycle, so good thing we phased it.
<jtv> Turns out if you don't have foreign keys pointing into the replication set (or the other way, I guess) your table is not replicated by default.
<adeuring> interesting
<jtv> Not the word I used at the time.  :-)
<leonardr> jcsackett: i'm sure you have a good reason (?), but i'm honor-bound to ask why you seem to have copied a bunch of javascript into pillar.js. is there any way we can refactor more than toggle_collapsible?
<jcsackett> leonardr: you mean refactor more than activate_collapsibles? toggle_collapsible was the one i didn't touch and simply sought to use with divs.
<leonardr> jcsackett: i think we're saying the same thing. you're reusing Y.lp.toggle_collapsible
<leonardr> but that code is at the end of a huge chunk of code that looks copied from somewhere else (eg. it's got a gmb XXX from over a year ago in it)
<leonardr> what changes did you make to the code after copying it? can we refactor those changes?
<jcsackett> leonardr: there's another function called simply activate_collapsible in lp.js that this is borrowed from; i tried refactoring it initially, but it's very specific to one set of expectations and my attempts to refactor led to horror.
<jcsackett> the majority of changes in this are to set it to work with divs, not fieldsets, and then simplify based on the small set of expectations this has. much of the comments were left b/c they were still applicable.
<leonardr> i well know the horror of refactoring javascript
 * jcsackett smiles.
<jcsackett> it reached the point where i had a choice of two similar and parsible functions or one terrifying (to me at least) one.
<leonardr> jcsackett: at the least, we should track the technical debt. can you file a bug mentioning the two places where there's similar code, and add an XXX? that way if we figure out how to refactor in the future it'll be clear where the work lies
<jcsackett> leonardr: certainly i can do that; i'll probably assign myself the bug for honor's sake too--i wasn't satisfied with this, but it seemed the best of bad alternatives.
<leonardr> jcsackett: indulge my laziness and tell me what tal:condition="view/configuration_links" does? why would that condition be false?
<leonardr> jcsackett: it looks like it would be false if you had a view of a pure IPillar rather than anything more specific?
<jcsackett> jcsackett: predates this branch; i can't return false but it can return none--it provides the configuration links if you can see them. if you can't see them then you can't edit things, i think.
<jcsackett> or actually not None, but an empty list.
<jcsackett> i think it also sets up some stuff about what you should see (e.g. configuration status).
<leonardr> ok, permission, that makes sense
<leonardr> jcsackett, similar refactoring question about head_epilogue. how much work is it to reuse the collapsible and collapsed styles (making them general styles instead of <div>-specific)?
<jcsackett> leonardr: ah, that shouldn't be too hard; i just thought based on some other pages if it only applied to one page that's where we put it. i'll move it.
<leonardr> jcsackett: it won't apply to one page if you reuse it for the fieldset collapsing code?
<leonardr> if i'm wrong, then don't move it
<jcsackett> leonardr: the fieldset already has styles set that aren't quite the same.
<leonardr> jcsackett: ok, don't bother, but add it to the bug so we can take another look later
<jcsackett> leonardr: dig.
<benji> leonardr: the exception you showed me on the keyring branch yesterday also happens on launchpadlib trunk
<leonardr> benji: well, damn
<leonardr> i'll look into it after i finish this review
<leonardr> jcsackett: this question may expose my ignorance more than anything else, but...
<leonardr> lines 387-388
<leonardr> client.click(link=u"Configuration links") triggers some javascript. does it then do anything to call waits.forPageLoad? surely the page has already loaded?
<leonardr> or does forPageLoad do something else?
<leonardr> maybe mars knows better than me -^
<leonardr> jcsackett: i had a question while you were out
<leonardr> <leonardr> jcsackett: this question may expose my ignorance more than anything else, but...
<leonardr> <leonardr> lines 387-388
<leonardr> <leonardr> client.click(link=u"Configuration links") triggers some javascript. does it then do anything to call waits.forPageLoad? surely the page has already loaded?
<leonardr> <leonardr> or does forPageLoad do something else?
<leonardr> also, do the windmill tests run in a deterministic order? can you be sure that test_configured_starts_collapsed runs after test_not_fully_configured_starts_shown?
<jcsackett> for the first question: so, there's no generic "wait" thing in windmill that i could find, and the test would fail because the class wasn't actually removed (or added depending on test) when it then went to test it.
<jcsackett> there's wait.sleep() but that's documented as pausing the browser, which isn't really appropriate.
<jcsackett> i can add a comment explaining that it's a generic wait, and as i look at it i should decrease the timeout.
<leonardr> jcsackett: sounds good. i bet mars has the answer
<jcsackett> for the second question: it doesn't matter (or it shouldn't) which order they run in; since the product is created in setup, each test should start with a product that's not configured.
<leonardr> jcsacket: if that's the case, it looks like both products are created with the same name ('hidden-configs'). how is that possible?
<jcsackett> leonardr: in my experience all tests seem to have a built in teardown that destroys things created in setUp.
<leonardr> jcsackett: ok, as long as it works
<jcsackett> it works in many other unittests; i'm not sure about windmill.
<jcsackett> would mars be the person to check that with?
<leonardr> mars: i guess. my concern, now that i've dug through the code, is that the windmill testing layer doesn't inherit from FunctionalLayer, which is where i'd imagine such cleanup code to be
<leonardr> it's not in TestCaseWithFactory, unless the act of creating a LaunchpadObjectFactory() in setup obliterates any objects created by any other factory
<leonardr> jcsackett: sorry, misaddressed to mars -^
<jcsackett> leonardr: i can add a teardown method to be sure.
<lifeless> leonardr: old objects are cleaned up by the db reset at the end of the test
<jcsackett> score.
<lifeless> so DatabaseLayer
<leonardr> jcsackett: and i've confirmed that setUp is run before each test
<leonardr> so you're fine
<lifeless> jcsackett: btw tearDown is (roughly speaking) deprecated
<lifeless> jcsackett: its generally much better to use addCleanup (but not always - explore them both, use your judgement)
<jcsackett> lifless: dig. in my experience so far it's all just nicely taken care of.
<jcsackett> lifeless, rather. ^
<leonardr> jcsackett: this is the kind of thing you get when i review launchpad branches because most of the time i'm off in my own little world
<jcsackett> leonardr: all good. it helps to have someone examining my assumptions. :)
<leonardr> jcsackett: r=me with minor changes
<jcsackett> leonardr: cool, thanks!
<jcsackett> i'll make those changes shortly.
* leonardr changed the topic of #launchpad-reviews to: On call: leonardr || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<leonardr> mars, do you want a review of https://code.edge.launchpad.net/~mars/launchpad/add-profiling-feature-flag/+merge/39179 or do you need to do some work to address lifeless's comment?
<bigjools> leonardr: hi, I've got a small branch up for review that exposes /builders on the API if you can take it?
<leonardr> bigjools, sure
* leonardr changed the topic of #launchpad-reviews to: On call: leonardr || Reviewing: bigjools || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<bigjools> leonardr: thank you
<leonardr> bigjools: "(usually a member of the buildd-admins team)" that seems specific to the way we use launchpad rather than the code itself. is that okay?
<bigjools> leonardr: buildd-admins are a celebrity in LP
<leonardr> bigjools: ok, you're good then
<leonardr> i just have some minor wording suggestions
<bigjools> sure thing
<leonardr> bigjools: r=me with very minor changes, https://code.edge.launchpad.net/~julian-edwards/launchpad/api-expose-builders/+merge/39379
* leonardr changed the topic of #launchpad-reviews to: On call: leonardr || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<bigjools> leonardr: great, thanks
<bigjools> leonardr: can you mark your review approved, I think you just landed a comment only.
<leonardr> oops
<bigjools> thanks for the suggestiond
<bigjools> leonardr: FWIW it looks like lp_attributes already sorts in a good way
<leonardr> bigjools: oh, i guess i used to be smarter than i am now
<bigjools> :)
<bigjools> it puts the boilerplate stuff first then the sorted attributes
<leonardr> rockstar, can i get a quick review from you?
<leonardr> https://code.edge.launchpad.net/~leonardr/launchpadlib/fix-token-authorization/+merge/39391
<rockstar> leonardr, it may be lossy, as I'm at UDS.
<leonardr> rockstar: ok, if mars responds in the next 5 minutes and says he can do it never mind. otherwise, it doesn't have to be super quick
<leonardr> (or if someone else can take it)
<rockstar> leonardr, the code looks good, reading the description now.
<mars> leonardr, ?
<leonardr> mars, just seeing if you want to take a review, since i don't think you're at uds
<mars> leonardr, sure
<mars> I'm in and out, but I'll look - 38 lines, that's a good size :)
<rockstar> mars, I've got it.
<rockstar> (If it was any bigger, I would just get it now)
<leonardr> ok, rockstar has it
<mars> k
<mars> thanks rockstar
 * rockstar high fives mars and yells "TEAMWORK!"
<mars> rockstar, btw, if you see Martin Pool, ask him if he has a moment to review my Feature Flags Test Fixture branch - people would love to see that land
<rockstar> mars, okay.  I'll keep an eye out.
<mars> thank you
<benji> sounds good
<benji> leonardr: almost forgot: https://code.edge.launchpad.net/~benji/wadllib/missing-files/+merge/39400
<leonardr> benji: r=me
<benji> leonardr: cool; shall I do a release?
<leonardr> benji: yes, please. let me know if you're missing anything from the process
<benji> k
* leonardr 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
<benji> leonardr: I've responded to your keyring question/issue.
<leonardr> benji, thanks
<leonardr> benji: that makes a lot of sense, thanks
<leonardr> i'm eod but i'll review your changes in a bit
<leonardr> benji: how tough would it be to test the storage and retrieval of the same application name w/two different service roots?
<benji> leonardr: trivial; shall I add that?
<leonardr> benji: yes please
<benji> leonardr: test added
<benji> leonardr: oh, I need to be granted access to wadllib on PyPI to complete the release
<leonardr> benji: sure
<leonardr> benji, you are benji on pypi?
<benji> leonardr: yep
<leonardr> benji, the role is yours
<benji> thanks, the upload worked
<leonardr> benji, i can't parse "(this assertion is of the test mechanism, not a test assertion)."
<leonardr> this assertion is veriyging the test mechanism?
<benji> exactly; I'm verifying my assumption that there will only be two sets of credentials stored in the fake keyring
<leonardr> benji: let me run some additional assertions by you
<leonardr> 1. the fake keyring starts out empty
<leonardr> 2. the application key has a specific form incorporating the application name and the hostname (?)
<leonardr> i was hoping for #2 but i'm willing to be convinced it's not necessary/ not appropriate to test here
<benji> 1 seems reasonable
<benji> at first I thought 2 was overconstraining things, but we do want to be sure the keys remain reable between releases of launchpadlib, so it makes sense
<benji> I also want to add an assertion that the application name is include in the key
<leonardr> isn't that part of 2?
<benji> indirectly, but I think all three (app name, server name, overall structure) are important
<benji> and it's cheap to test them all, so...
<leonardr> ok, go for it
<benji> leonardr: done
 * leonardr looks
<leonardr> benji: r=me, though i don't think we should merge this yet
<benji> leonardr: k
#launchpad-reviews 2010-10-27
<bac> hi henninge
<henninge> Hey bac! ;)
<bac> henninge: so did you get UI graduated?
<henninge> bac: so rockstar tells me. Is it his call alone?
<bac> henninge: no, he talked to me and sinzui.  i just wasn't sure.  the discussion of it in the meeting notes last week was a little anti-climatic
<bac> henninge: so, congratulations!
<henninge> bac: Thank you!
<bac> henninge: thanks to you.  i've updated the wikis to show it.
* jtv changed the topic of #launchpad-reviews to: On call: jtv || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jtv> hi bac, do you feel like reviewing my branch?  24 lines of diff but a huge cover letter.  :-)  https://code.launchpad.net/~jtv/launchpad/bug-662552-defer-potmsgset-filter/+merge/39426
<bac> jtv: ok
<jtv> thanks bac
<bac> jtv: who is UCP?
<jtv> Universitat PolitÃ¨cnica de Catalunya
<jtv> Which I believe means Universidad PolitÃ©cnica de CataluÃ±a.
<jtv> Great database professor there.
<jtv> Made things really accessible, and got his team to implement a real-world innovation in postgres instead of messing about with theory and simulations.
<bac> that's cool
<bac> thanks for the branch and explanation.
<jtv> thanks for the reviewâ"mysterious" is about right.  :-)
<henninge> hey jtv, I am unsure atm: Will this code also return existing languages or throw an exception if the code exists?
<henninge> http://paste.ubuntu.com/520708/
<henninge> from LangugeSet
<jtv> henninge: well it constructs a Language.
<jtv> So there's your answer.  :)
<henninge> jtv: you are not helping
<jtv> It'll create a new Language object, and if one of the same code or englishname already existed, then you'll get a unique violation when it gets flushed to the database.
<henninge> jtv: now you are helping ;-)
<henninge> I suspected that but forgot...
 * jtv regrets it already :)
<jtv> It's just a constructor.  Constructors don't look for existing objects.
* jtv 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 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
<henninge> jtv: It's a pitty you just eod'ed. You would have liked this one ... ;-)
<jtv> ?
<henninge> https://code.edge.launchpad.net/~henninge/launchpad/devel-productseries-views-tests/+merge/39437
 * jtv checks channel name
<henninge> But I will go to lunch myself now ...
<jtv> yes, I just eod'ed.  :)
<henninge> :-P
<lifeless> https://code.launchpad.net/~lifeless/launchpad/zope.testing/+merge/39423
<jtv-afk> henninge: someone who probably isn't here right now just came by and spontaneously reviewed your branch
<henninge> jtv-afk: that is awfully nice of him (or her?) !
* 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
<jcsackett> is someone available to do a ui review on https://code.edge.launchpad.net/~jcsackett/launchpad/hidden-project-configuration-636000/+merge/39306
<leonardr> as long as we're asking for reviews, i need a code review for https://code.edge.launchpad.net/~leonardr/lazr.restfulclient/enhanced-consumer/ (writing mp now)
<leonardr> jcsackett, i'd swap reviews with you if i were rated for ui reviews
<jcsackett> leonardr: i'd take you on that if i were a reviewer. :-P
<leonardr> like two ships passing in the night
 * jcsackett laughs.
<henninge> jcsackett: I'll take it.
<jcsackett> henninge: thanks!
<leonardr> salgado, maybe you want to take a minute and review https://code.edge.launchpad.net/~leonardr/lazr.restfulclient/enhanced-consumer/+merge/39447 ?
<henninge> rockstar: when you get up, can you please add me to launchpad-ui-reviewers? ;-)
<rockstar> henninge, ah!
<henninge> Hi rockstar! ;)
<rockstar> henninge, hello sir.
<rockstar> henninge, done.
<henninge> rockstar: great, thank you! Now I can claime that review ...
<henninge> s/claime/claim/
<henninge> jcsackett: That looks very good, thank you! I just wonder if "Configuration links" is the best title.
<henninge> jcsackett: how about "Configuration steps"?
<jcsackett> not sure; that implies a sequence, and you don't really have to do any of them in sequence.
<jcsackett> in all truth, we just really *want* people to set things up. if they don't care they don't have to do any of it.
<henninge> "Configuration details"
<jcsackett> henninge: yeah, i can go with that.
<henninge> It's just that "links" are all over the place on a webpage
<jcsackett> I was also thinking "Configure services"
<henninge> Or that.
<jcsackett> preference, oh ui reviewer? :-)
<jcsackett> i'm good with either, personally.
<jcsackett> does one seem better or more in keeping with convention?
<henninge> what was it you copied from?
<henninge> ah, "extra options"
<henninge> May "Configuration options"?
<jcsackett> that works. and that does seem to follow the convention.
<jcsackett> i'll make that change push it up. thanks, henninge.
<henninge> jcsackett: cool, let's use that.
<henninge> jcsackett: there is something else
<henninge> I just ran the branch and it looks different to the screenshot.
<henninge> s/to/from/ ?
 * jcsackett looks alarmed.
<henninge> There is the blank line missing after "Configuration links"
<jcsackett> ah, yes. i killed the <br> from the code review and didn't update the screenshots.
<jcsackett> sorry.
<jcsackett> you prefer with the blank line?
<jcsackett> i wasn't sure either way to go on that.
<henninge> Actually, that was a bit too much. Could we have half a line?
 * henninge hacks
<jcsackett> henninge: i'm good with half a line, but i'll confess my css-fu is weak.
<henninge> jcsackett: turn "legend" into a div and give it a "margin-bottom: 0.5em;" . That looks fine.
<jcsackett> henninge: dig. will do.
<henninge> jcsackett: you will have to script your js code because this breaks it.
<henninge> s/script/fix/
<jcsackett> henninge: yeah, i think that javascript should just be #legend not span#legend anyway.
<henninge> jcsackett: or maybe just use a class?
<jcsackett> henninge: your right. ".legend" it is.
<henninge> jcsackett: call it ".configuration_optoins"
<jcsackett> henninge: okay.
<henninge> although ...
<henninge> yeah, do that. I think it's better to give such specific classes specific names.
<henninge> jcsackett: .config-options
<henninge> last word, promised ... ;-)
<henninge> r=me
<jcsackett> thanks henninge. i'll make those changes. :-)
* EdwinGrubbs changed the topic of #launchpad-reviews to: On call: Edwin || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jtv> Edwin: free for a review?  https://code.launchpad.net/~jtv/launchpad/bug-662552-get-tm-or-dummy/+merge/39467
<jtv> A lot of it is lint.
<StevenK> jtv: I have a branch too, shall I add you to queue?
<jtv> StevenK: yes please
* StevenK changed the topic of #launchpad-reviews to: On call: Edwin || Reviewing: - || queue: [jtv, StevenK] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* noodles775 changed the topic of #launchpad-reviews to: On call: Edwin || Reviewing: - || queue: [jtv, StevenK, noodles775] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<noodles775> Hi Edwin-afk, could you please review the following when you've time: https://code.edge.launchpad.net/~michael.nelson/launchpad/627608-p3a-token-race/+merge/39469
<EdwinGrubbs> jtv: sorry, I didn't see your comment. I'll start reviewing it now.
<jtv> EdwinGrubbs: cool, thanks
<noodles775> (one-line change with a new test)
<EdwinGrubbs> noodles775: sure. I guess I'll look at that first then.
<noodles775> EdwinGrubbs: thanks!
<EdwinGrubbs> noodles775: what happens if the scripts fails to run? For example, if it can't connect to the database but the next minute it runs fine.
<noodles775> EdwinGrubbs: ah, you mean if date_started *is* set, even though it didn't complete... good point.
<noodles775> I'll check script-activity and ensure we have a successful record.
<noodles775> EdwinGrubbs: OK, I need to update to ensure date_completed is not null in the query. I'll update the test, make sure it fails and fix.
<leonardr> Edwin: pls ad https://code.edge.launchpad.net/~leonardr/lazr.restfulclient/enhanced-consumer/+merge/39447 to the queue?
<noodles775> EdwinGrubbs: actually, afaics, script activity is only ever recorded on success (IScriptActivitySet has only a recordSuccess method).
<noodles775> EdwinGrubbs: Assuming that's true, the current code is fine as is.
<EdwinGrubbs> noodles775: ok. Is the htaccess file the only thing that this script updates, so you are completely dependent on the ArchiveAuthToken.date_created and the last_success.date_started running on machines whose clocks are in sync?
<noodles775> EdwinGrubbs: yes (actually the htpasswd). And again, that's a good point about needing to be in sync. Thinking.
<mwhudson> oh eh, i just reviewed that branch :-o
<noodles775> Thanks mwhudson - do you know if the issue Edwin mentiones above could be a problem? (ie. the token is created on an app server, but the script activity will be on germanium). Can we trust the timestamps?
<mwhudson> noodles775: i *think* i've asked is about this before, and we can rely on ntp running
<mwhudson> noodles775: you might want to check though :-)
 * noodles775 checks
<mwhudson> noodles775: there is also a chance that all times will be from the db
<noodles775> mwhudson: date_started and date_completed are required args to IScriptActivitySet.recordSuccess, so I don't think they will be set by the db.
<mwhudson> yeah, indeed
<mwhudson> noodles775: you _could_ probably pass UTC_NOW for date_completed
<mwhudson> but we don't
<noodles775> mwhudson: yeah, and we're checking date_started here.
<mwhudson> also true
<mwhudson> noodles775: i guess a fudge would be to check all tokens created >= date_started - 1
<mwhudson> second
<EdwinGrubbs> noodles775: I think mwhudson fudge factor is reasonable. I'll let you two decide on the final implementation and start reviewing something else.
<noodles775> EdwinGrubbs: so we can assume NTP is running (ie. minimal skew), but I'll add a 1-second....
<noodles775> :)
<noodles775> Thankyou EdwinGrubbs.
<StevenK> EdwinGrubbs: Are you still reviewing
<StevenK> ?
<EdwinGrubbs> StevenK: yes, I'll get to yours after jtv's.
* EdwinGrubbs changed the topic of #launchpad-reviews to: On call: Edwin || Reviewing: - || queue: [jtv, StevenK] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<StevenK> EdwinGrubbs: Excellent, thank you. I have two. :-)
<EdwinGrubbs> jtv: review sent
<jtv> thx
* EdwinGrubbs changed the topic of #launchpad-reviews to: On call: Edwin || Reviewing: - || queue: [StevenK, leonardr, StevenK] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<EdwinGrubbs> StevenK: which branch would you like me to review first.
<StevenK> EdwinGrubbs: The qafail
<jtv> EdwinGrubbs: I'm definitely eod, but just put up another one: https://code.edge.launchpad.net/~jtv/launchpad/bug-662552-view-permissions/+merge/39481
<EdwinGrubbs> jtv: I doubt that I will have time to get to it.
<jtv> no worries
<EdwinGrubbs> leonardr: wouldn't you want the OAuthAuthorizer.user_agent_params to include the oauth_consumer, especially if the application_name isn't set?
<leonardr> OAuthAuthorizer: application_name defaults to oauth_consumer
<EdwinGrubbs> ok, I missed that
<leonardr> maybe we also want the oauth_consumer, i dunno. gary, maybe you can weigh in when you get a minute? (poss. in comments on https://code.edge.launchpad.net/~leonardr/lazr.restfulclient/enhanced-consumer/+merge/39447)
<gary_poster> looking
<StevenK> EdwinGrubbs: Thank you for the review!
<EdwinGrubbs> leonardr: it looks like you have a merge conflict in src/lazr/restfulclient/docs/operations.txt, although it will be trivial to fix.
<leonardr> ok
<gary_poster> argh must run
<gary_poster> leonardr: I'm sorry, I have to run.  Do you need me to check back in later, or is it alright?
<leonardr> gary, it's ok
<gary_poster> ok thanks
<gary_poster> g'night
<jcsackett> leonardr: i added support for qastaging to launchpadlib if you would like to take a look: https://code.edge.launchpad.net/~jcsackett/launchpadlib/add-qastaging-to-uris/+merge/39483
<jcsackett> having to manually throw in the url when qaing was bugging me.
* EdwinGrubbs changed the topic of #launchpad-reviews to: On call: Edwin || Reviewing: - || queue: [StevenK] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<EdwinGrubbs> StevenK: I'll finish your other branch later tonight.
* EdwinGrubbs changed the topic of #launchpad-reviews to: On call: - || Reviewing: - || queue: [StevenK] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<StevenK> EdwinGrubbs: Excellent, thank you
<leonardr> jcsackett, i'll take a look tomorrow. remind me if i forget
#launchpad-reviews 2010-10-28
<leonardr> jcsackett: r=me if you add it to the NEWS.txt
<thumper> https://code.edge.launchpad.net/~thumper/launchpad/less-wip-emails/+merge/39489
<thumper> anyone
<thumper> StevenK: are you on?
<StevenK> thumper: I am now.
<thumper> StevenK: want to look at that review?
<thumper> StevenK: we can get someone else to mentor your review
<thumper> StevenK: but you can have the practice
<StevenK> thumper: It is also currently 11pm, and I'd like to sleep so I can be useful in the 9am session tomorrow, unlike today where I didn't feel awake until 10.
<thumper> StevenK: oh, forgot
<thumper> sorry, feel free to leave it
<StevenK> thumper: Ah, you thought I was in .au?
<thumper> yeah
<StevenK> Heh, yes.
<thumper> wallyworld: I'm looking at your review
<wallyworld> thumper: thanks. actually, any idea why firefox  keeps asking me to enter a username/password when it is being ised by windmill to run the tests?
<thumper> no, just ignore it
<wallyworld> the popup says: "The site says: 'bookmark-user-auth blah blah blah'
<wallyworld> yeah, but i have to click cancel 100000 times since there's so many of them and sometimes if i don't the test fails :-(
<wallyworld> and i also get "invalid security certificate"
<wallyworld> and it leaves zombie firefox instances eating all my cpu :-(
<wallyworld> and sometimes i get that page where you need to click to add a security exception
<wallyworld> makes it very hard to run the tests
<wallyworld> is it just me/my setup or is it a general problem?
<thumper> I don't get that....
<wallyworld> hmmmm. what should have been a 5 minute test took way longer dealing with that stuff cause the test kept failing :-(. i'll have to look into it then
<thumper> :(
<thumper> I gave you the wrong info
<thumper> str(e) isn't good enough
<thumper> <Product at 0xa592f10> has no linked branch.
<thumper> is kinda crap
<wallyworld> ah bugger. i didn't see that when i ran it up.
<wallyworld> probably cause i had fake products foo and bar
<wallyworld> i'll make the test better and fix the problem
<wallyworld> leave it with me
<thumper> wallyworld: cool
<wallyworld> everything else ok though?
<thumper> looks reasonable
* jtv changed the topic of #launchpad-reviews to: On call: - || Reviewing: - || queue: [StevenK, jtv] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<thumper> jtv: hi
<jtv> hi thumper
<jtv> I'll trade you.
<thumper> jtv: https://code.edge.launchpad.net/~thumper/launchpad/less-wip-emails/+merge/39489
<thumper> jtv: what do you have?
<jtv> thumper: https://code.edge.launchpad.net/~jtv/launchpad/bug-662552-view-permissions/+merge/39481
<thumper> nice size for a review :)
<jtv> Took enough research, I can tell you.
 * jtv looks at Snapshot
<thumper> jtv: Snapshot is what the LaunchpadFormView uses, and lazr.restful
<thumper> jtv: the code I changed was using the delta object in error
<thumper> it just happened to work
<jtv> nod
<jtv> thumper: so far so good, but I just have to find something.  Could you replace "there is no email job created" (test_branchmergeproposal.py) with something easier to read?
<thumper> sure
 * jtv suddenly realizes the danger of picking nits while doing mutual reviews
<jtv> thumper: wouldn't test_nominateReview_no_job_if_work_in_progress clearer if it just did a "with person_logged_in(bmp.registrant): bmp.nominateReviewer(...)" instead of logging in at what looks like a somewhat arbitrary point?
<jtv> *be* clearer, that is
<thumper> jtv: probably, I was primarily just copying existing tests
<jtv> (pot, kettle)
<jtv> There's one other thing in that method: in the IStore(...).find(), the last line of the condition isn't indented enough.  I think it'd help to keep BMPJobType.REVIEW_REQUEST_EMAIL in a variable.  One with a short name.
<thumper> jtv: where is that first line you want changed?
<jtv> test_nominateReview_no_job_if_work_in_progress as well, top comment, 2nd line.
<jtv> Oh!  How about putting the IStore(...).find() in a helper method, so that the positive test can check for the exact same thing?
 * thumper was looking in old branch
<thumper> jtv: because the positive test wants the one
<thumper> does store.one return None or raise if not found?
<jtv> Raises.
<jtv> But any() does what you want here, no?
<thumper> no
<thumper> because if there was 4
<thumper> it would pass
<jtv> Simple solution: have your helper return the result set.
<wallyworld> thumper: can you ping me when you are done.
<jtv> thumper: there are a few other places in the test that could potentially be nicer with a "with person_logged_in()" but I'll leave that up to you.  Approving.
<thumper> ta
<thumper> wallyworld: whazzup?
<wallyworld> the issue is that NoLinkedBranch and friends use %r for their error messages
<wallyworld> so i will have to catch each type and recreated the error message with %s
<wallyworld> any idea why the exceptions don't use %s
<thumper> not all types have a string representation
<wallyworld> i guess it's to give a better indication of what the object is
<thumper> hmm...
<wallyworld> well, i would have to check but surely Product et al would have a string representation
<thumper> don't expect that
<jtv> thumper: and you're done.
<thumper> use something like 'name' or display anme
<thumper> display_name
<thumper> jtv: used with person_logged_in(bmp.registrant) in those tests now
<jtv> cool
<wallyworld> yeah, so i have to catch the exceptions and recreate a new instance of the same type with display_name as the construction  arg
<wallyworld> ?
<wallyworld> that way i can then do a str() to get the error message
<wallyworld> thumper: sound ok^^^^^^^^^^ ? i have to duck out and pick up the kid from school and buy more tonic water :-)
<thumper> wallyworld: I think it would make more sense to have a better error string
<thumper> wallyworld: perhaps another method on the exception
<thumper> wallyworld: don't recreate a different one
<thumper> wallyworld: the exception has all the info we need
<thumper> wallyworld: it just isn't showing it to us in a nice way
<wallyworld> i don't mean to create a new exception type
<thumper> don't create a new object
<wallyworld> ack. i didn't think we would want to dick with the exception code. but i can do that np
<wallyworld> ie add a new method etc
<wallyworld> creating as new object wasn't what i really wanted to do :-)
* jtv changed the topic of #launchpad-reviews to: On call: - || Reviewing: - || queue: [StevenK] || 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 || Reviewing: - || queue: [StevenK] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<bac> hi allenap can i add a trivial branch to your queue?
<allenap> bac: Certainly :)
<bac> great.  LP is still processing the MP but it should show up shortly
* bac changed the topic of #launchpad-reviews to: On call: allenap || Reviewing: - || queue: [StevenK, bac] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<bac> i'm going to go walk around the block. brb
* allenap changed the topic of #launchpad-reviews to: On call: allenap || Reviewing: bac || 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 || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<adeuring> allenap: couls you please review this MP: https://code.edge.launchpad.net/~adeuring/launchpad/bug-594247-unittests-for-searchtasks-3/+merge/39523 ?
<allenap> adeuring: Sure.
<adeuring> thanks!
* allenap changed the topic of #launchpad-reviews to: On call: allenap || Reviewing: adeuring || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<rockstar> allenap, can I jump in your queue?
<allenap> rockstar: Sure, I've had my eyes on your merge queue proposal already.
<rockstar> allenap, ooh, kinky.  :)
<rockstar> allenap, for easy lookup: https://code.edge.launchpad.net/~rockstar/launchpad/merge-queue-index/+merge/39488
* rockstar changed the topic of #launchpad-reviews to: On call: allenap || Reviewing: adeuring || queue: [rockstar] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<adeuring1> allenap: thanks for the review! may I ask for another review? Your remark about the assert() let me realize that I forgot to "bzr commit" the latest changes...
<allenap> adeuring1: Sure.
* allenap changed the topic of #launchpad-reviews to: On call: allenap || Reviewing: rockstar || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<adeuring1> allenap: thanks, and sorry for the confisuon...
<allenap> adeuring1: Have you pushed the branch?
<adeuring1> allenap: a few seconds ago...
<rockstar> allenap, hey, my password fix doesn't actually clobber the password argument.
<allenap> rockstar: Really? I must have misread.
<rockstar> allenap, so, the only place that password gets used is creating a user if the user isn't specified.  Since we then get a naked user (ooh la la), we're essentially getting the password back from the user, so in some cases, it's really a no-op.
<allenap> rockstar: Okay, cool. I'll add a comment to the mp.
* allenap changed the topic of #launchpad-reviews to: On call: allenap || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<leonardr> EdwinGrubbs, https://code.edge.launchpad.net/~leonardr/lazr.restfulclient/enhanced-consumer/+merge/39447 now incorporates the changes you wanted
<EdwinGrubbs> leonardr: that looks good.
<leonardr> ok, great
* allenap 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
<flacoste> lifeless: feel free to call me
<leonardr> edwingrubbs: as long as you're reviewing my branches, want to look at a short follow-up?
<leonardr> https://code.edge.launchpad.net/~leonardr/lazr.restfulclient/system-wide-consumer/+merge/39552
<EdwinGrubbs> leonardr: I'm headed to lunch now, but I could look at it later today.
<leonardr> ok, i'll let you know if someone else takes it up
<leonardr> EdwinGrubbs: one other minor question. i was thinking of changing OAuthAuthorizer to be able to accept a Consumer object as well as a consumer name (so that I can create it with a SystemWideConsumer), but i could just create it with no consumer and then set the consumer afterwards. let me know which you would rather
<EdwinGrubbs> leonardr: I really don't know which way would be better.
<leonardr> edwingrubbs: i'll create it with no consumer. it's simpler, and i can change it later
<leonardr> Edwin: can you look at that branch? i assumed you'd done the review since you asked that question, but i don't see a review from you
<EdwinGrubbs> leonardr: I'll start on the review now.
<leonardr> thanks
<EdwinGrubbs> leonardr: I did a little looking into the platform module, since I was curious. It appears that some older versions of platform.dist() don't sort the results of listdir('/etc'), so it may randomly get its info from /etc/debian_version or /etc/lsb-release. That seems like it could cause some really confusing bug reports eventually, if authentication breaks. Maybe, you should check if /usr/bin/lsb_release exists and use
<EdwinGrubbs> "lsb_release -si"  before trying dist().
<leonardr> EdwinGrubbs: my gut reaction to that news is to get rid of the dist() altogether and go right to system()
<EdwinGrubbs> leonardr: ok, if "Linux" is distinct enough, that sounds fine.
<leonardr> "Linux" is fine
<EdwinGrubbs> leonardr: btw, I already submitted my review, but it just rehashes this issue for the record.
<leonardr> ok, thanks
<jcsackett> lifeless: ping
<jcsackett> nm.
<lifeless> hi
#launchpad-reviews 2010-10-29
* 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
* adeuring changed the topic of #launchpad-reviews to: On call: adeuring || Reviewing: - || queue: [jtv] || 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: jtv || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jtv> hi adeuring!
<jtv> got an oversized one for you I'm afraid.
<jtv> It's really only two very simple changes, but the diff worked out rather large.
<jtv> All context, very little actual change: 989 lines (+199/-169) 42 files modified
<adeuring> jtv: yeah, I've noticed that. Sorry for the dealy with this repsonse -- needed to make a cup of coffee
<adeuring> s/dealy/delay/
<adeuring> jtv: 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
<jtv> adeuring: thanks!
<jtv> adeuring: up for another one?   https://code.launchpad.net/~jtv/launchpad/bug-668194-fix-api/+merge/39615
<adeuring> jtv: sure
<jtv> thanks
* adeuring changed the topic of #launchpad-reviews to: On call: adeuring || Reviewing: jtv || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<adeuring> jtv: r=me
<adeuring> On call: adeuring || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jtv> adeuring: thanks once more!
<henninge> adeuring: Hallo! ;)
<adeuring> moin henninge
<henninge> adeuring: WÃ¤rst du so nett, bitte? https://code.edge.launchpad.net/~henninge/launchpad/devel-bug-638920-private-branch/+merge/39540
<henninge> ;-)
<adeuring> henninge: klar!
<henninge> adeuring: I'll be switching locations now but will be back here soon.
<adeuring> henninge: ok
<henninge> adeuring: Hi, I am back!
<henninge> adeuring: Is the topic lying or are you still working on one of jtv's branches?
<jtv> henninge: the topic is outdated
<jtv> feel free to change it
<adeuring> henninge: ouch, I'm looking at your branch...
* adeuring changed the topic of #launchpad-reviews to: On call: adeuring || Reviewing: henninge || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<henninge> adeuring: cool ;-)
<adeuring> henninge: I've sent a comment about your  branch
<jtv> adeuring: did you accidentally post your comment on henninge's branch on my MP?
<adeuring> jtv: gaahhh, thanks for the head-up ...and sorry for the noise!
<henninge> adeuring: oh sorry, sound turned off and not paying attention.
<henninge> adeuring: you are raising a basic question there that will need discussion on the mailing list, I think.
<henninge> adeuring: Are you ok with the branch if I just add the comments as you requested and leave it to a future branch to reflect the outcome of that discusstion?
<adeuring> henninge: yes, that's fine, I think
* 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
<henninge> adeuring: cool, thanks.
<henninge> adeuring: Maybe for symmetrie I should turn has_exports_enabled and uses_bzr_sync into properties, too?
<adeuring> henninge: yes, that would be great!
<henninge> adeuring: done. Thank you very much for the review. ;-)
<adeuring> henninge: welcome :)
* EdwinGrubbs changed the topic of #launchpad-reviews to: On call: adeuring || Reviewing: - || queue: [Edwin] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<EdwinGrubbs> adeuring: can you review https://code.edge.launchpad.net/~edwin-grubbs/launchpad/bug-663861-product-series-timeout-part2/+merge/39638
<adeuring> Edwinsure
<adeuring> EdwinGrubbs: sure
* adeuring changed the topic of #launchpad-reviews to:  On call: adeuring || Reviewing: Edwin || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<adeuring> EdwinGrubbs: can the strings that are soted contain caracters outside the usual ASCII set? (like 'Ã¶')?
<EdwinGrubbs>  adeuring: I'll check, but I don't think the productseries.name is even allowed to contain those characters.
<EdwinGrubbs> adeuring: the valid_name() constraint only allows a-z, 0-9, "+", ".",  and "-".
<adeuring> EdwinGrubbs: ah, OK. That makes the sorting more robust :)
<adeuring> EdwinGrubbs: but I think your example sorting [devfocus, 1, c] doesn't work. I believe the sort order will be [devfocus, c, 1]
<adeuring> EdwinGrubbs: never mind... misunderstood "devfocus"
<adeuring> EdwinGrubbs: but how is the code in the file interfaces/branchmergeproposal.py related to the other changes?
<EdwinGrubbs> adeuring: doh, I made the merge-proposal against devel, when it should be against db-devel. That's why you see branchmergeproposal.py, which I didn't touch. Let me redo the mp.
<adeuring> EdwinGrubbs: Ahh, ok :)
<EdwinGrubbs> adeuring: here is the new mp: https://code.edge.launchpad.net/~edwin-grubbs/launchpad/bug-663861-product-series-timeout-part2/+merge/39643
<adeuring> EdwinGrubbs: thanks!
<adeuring> EdwinGrubbs: r=me; one minor suggestion
* 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
<EdwinGrubbs> adeuring: thanks
* 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 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
<henninge> Anybody up for a 25 lines review?
<henninge> https://code.edge.launchpad.net/~henninge/launchpad/devel-bug-666660-poimport-oops/+merge/39653
<StevenK> henninge: I've +1'd it, but I need to be mentored
<henninge> StevenK: thanks ;)
<henninge> StevenK: how is you mentor?
<henninge> hang on
<StevenK> thumper, the problem is it's Saturday for him
<henninge> nm
<henninge> ;)
* 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> StevenK: thanks for taking care of this. I will start my weekend now. Enjoy your Saturday! ;)
<StevenK> henninge: I'm at UDS, and it's 3pm :-)
<StevenK> So my Saturday will involve stacks of airports and I won't enjoy it.
<henninge> StevenK: Oh, then enjoy Marks closing remarks and the last night out with the guys ... ;)
<henninge> StevenK: say hello to Danilo for me ;)
<StevenK> henninge: I shall
