#launchpad-reviews 2009-12-07
<thumper> https://code.edge.launchpad.net/~thumper/launchpad/lp-code-errors/+merge/15724
<thumper> and now https://code.edge.launchpad.net/~thumper/launchpad/claim-review-into-model-attempt2/+merge/15728
<adiroiban> henninge: hi. is the submission now open?
* henninge changed the topic of #launchpad-reviews to:  on-call: henninge || reviewing: -  || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<henninge> adiroiban: yes, channel topic on #lp-dev says "PQM is open for srs bsns" ;-)
<henninge> Good morning adiroiban :)
<adiroiban> srs bsns ? Serious Business ?
<henninge> correct
<adiroiban> does it mean any kind of MP ?
<henninge> our losas, aren't they just a funny bunch ? ;-)
<henninge> adiroiban: yes, the other states are "closed", which was last week, and "testfix" which may happen anytime when the test suite breaks on the buildbot.
<adiroiban> henninge: I see. That make sense now :) 
<adiroiban> henninge: will you "land" my branch , or is there something I need to do?
<henninge> adiroiban: hang on, let me check on the situation
<henninge> adiroiban: please merge the current devel into your branch and push it again.
<henninge> adiroiban: like this: http://paste.ubuntu.com/336365/
<adiroiban> henninge: much appreciated :)
<henninge> watch for merge conflicts
<adiroiban> henninge: and I should create a new MP ?
<henninge> adiroiban: no
<henninge> adiroiban: if you didn't have any serious merge conflicts, we can just land it.
<henninge> adiroiban: what was the branch again?
<adiroiban> https://code.edge.launchpad.net/~adiroiban/launchpad/bug-97293/+merge/15391
<adiroiban> there were only 2 lines of code :)
<adiroiban> from bzr merge -> All changes applied successfully.
<henninge> cool
<adiroiban> branch was pushed
<henninge> adiroiban: a, I remember
<henninge> adiroiban: yup got. Ok, nothing else for you to do now, I will land it.
<adiroiban> henninge: thanks. An for the other one I should do the same merge and ping the person who told me that will land the branch?
<henninge> adiroiban: just ping me
<henninge> after the pushing
<adiroiban> ok
<adiroiban> henninge: https://code.edge.launchpad.net/~adiroiban/launchpad/bug-193750 merged...
<adiroiban> should I give you the full link to the branch  or to the MP, or just the MP number?
<henninge> adiroiban: all good, I got it.
<henninge> ;)
<adiroiban> henninge: :) ... another one https://code.edge.launchpad.net/~adiroiban/launchpad/bug-487970/+merge/15250
<adiroiban> henninge: last one: https://code.edge.launchpad.net/~adiroiban/launchpad/bug-135008/+merge/15347
<henninge> adeuring: wow, you have been busy! Great job!
<henninge> adiroiban: ah, one more thing you can do: Add a commit message to each mp!
<henninge> that way I don't have to make them up  ... ;)
<adiroiban> henninge: hm... how?
<adiroiban> :)
<henninge> adiroiban: simply click on "Set commit message" :)
<henninge> It's a new feature!
<henninge> adiroiban: It's a green line starting with a (+) icon.
<adiroiban> Ah... it was just my nose covering that link :)
<henninge> lol
<adiroiban> got it
<adiroiban> and what should I write there?
<henninge> it's just a bzr commit message to use when your  branch is merged and commit into devel.
<henninge> so you can probably just copy something you used for your local commits.
<adiroiban> yes. thanks
<henninge> "copying plural values will also enable the required fields"
<henninge> adiroiban: But make it proper sentences, please. ;)
<henninge> "Copying plural values will also enable the required fields."
<adiroiban> henninge: done for all 4 MPs
<henninge> adiroiban: cool, thanks. You are, done. I'll keep you posted on the proress during the day.
<adiroiban> henninge: thanks. I will also need a pre-MP review for https://code.edge.launchpad.net/~adiroiban/launchpad/bug-431249
<adiroiban> should I ask it on lp-reviews or via the bug tracker?
<henninge> adiroiban: no, using the bug tracker for code reviews is not what we do.
<henninge> adiroiban: asking here or directly one of the lp developers is what you should do.
<adiroiban> henninge: here or on lp-dev ?
<henninge> adiroiban: Incidently I am an on-call reviewer (OCR) today.
<henninge> ;)
<henninge> adiroiban: "here" is #lp-reviews
<henninge> but I am also Help contact and currently busy landing some great contributions submitted by a cool community member ... ;-)
<adiroiban> OCRs are also doing pre-MPs reviews ?
<adiroiban> sorry for all these questions... but I'm lost in this process
<henninge> adiroiban: first one's in! http://bazaar.launchpad.net/~launchpad-pqm/launchpad/devel/revision/9977
<henninge> adiroiban: the others will take longer, though. I did not run this through the test suite since it does not yet run javascript tests.
<henninge> I just ran the tests locally and submitted directly.
<adiroiban> henninge: ok. np. thanks
<henninge> adiroiban: was this your first contribution to Launchpad?
<adiroiban> henninge: yep. 2 lines ... like 0.02$ :)
<henninge> adiroiban: Well, Congratulations!
<henninge> ;-)
<danilos> adiroiban, henninge: hi :)
<adiroiban> danilos: hi :D
<henninge> hi danilos!
<adiroiban> danilos: I would need a quick review for this branch so see if I should push it to a MP https://code.edge.launchpad.net/~adiroiban/launchpad/bug-431249
<danilos> adiroiban, I've seen you looked into a gazillion of bugs :) I'll be going through them and commenting; in general, I suggest you talk about the bugs beforehand (i.e. before starting on them), just to make sure you are not wasting your time if we disagree what the solution should be
<danilos> adiroiban, sure, looking at bug 431249
<mup> Bug #431249: Put navigation links in a seperate template/macro. <post-3-ui-cleanup> <Launchpad Translations:In Progress by adiroiban> <https://launchpad.net/bugs/431249>
<adiroiban> danilos: i agree... but the goal of all those bugs was to get into the code. Even if that work will not be used, I still learnt how to hack LP
<danilos> adiroiban, sure :)
<danilos> adiroiban, I am not saying it's not great, just giving an advance notice :) it's possible they are all doing exactly as desired :)
<adiroiban> danilos: so you can consider this is the pre implementation discussion. I was aware of this risk
<adiroiban> there is no problem if that code will not be used... as you can see, I start working on some bugs that were not even confirmed
<danilos> adiroiban, right, bug 431249 fix looks reasonable; I'd still keep the page ID something like "translation-macros", even though it's registered only on TranslationsLayer, because this might hide a generic +macros if it exists
<mup> Bug #431249: Put navigation links in a seperate template/macro. <tech-debt> <ui> <Launchpad Translations:In Progress by adiroiban> <https://launchpad.net/bugs/431249>
<danilos> (or if it's created in the future)
<danilos> adiroiban, otherwise, it looks like a good approach, so, provided there are sufficient tests, go for it :)
<adiroiban> danilos: ok. I can change that
<adiroiban> danilos: should I add more test ? 
<danilos> adiroiban, not necessarily, only add a test if there's not any one testing the generic presence of these links on both pages - do not go through all the conditional tests etc.
<adiroiban> those changes should not touch any âvisibleâ part
<adiroiban> aha. ok
<danilos> adiroiban, right, so, it's two options: find existing test or write one :) former is usually simpler :)
<danilos> adiroiban, btw, did you get help from Henning about landing your already approved branches?
<adiroiban> danilos: yep. everhing is ok.
<adiroiban> danilos: the other feedback I'm asking is about this one: https://code.edge.launchpad.net/~adiroiban/launchpad/bug-406477
<danilos> adiroiban, looking
<danilos> adiroiban, in general, it's fine; however, I'd rather structure those permissions differently, and make use of launchpad.TranslationsAdmin permission for this role instead of using launchpad.Admin (though, note that you'd have to change .zcml in that case as well, perhaps even some templates something/required:launchpad.Admin conditions)
<danilos> adiroiban, but, if you want to keep it simple, just go with what you've got (what I am suggesting is simply because the current state of privileges/permissions in LP is a bit of a mess, and I'd like to clean it up :)
<adiroiban> danilos: I can look into that. This was my first look at LP privilege system, and I just wanted to make sure I got it right.
<adiroiban> danilos: thanks. This should could keep me bussy for some time :)
<danilos> adiroiban, heh, nah, thank you :)
<salgado> henninge, trivial one for you: http://paste.ubuntu.com/336479/
<henninge> salgado: what is that file for?
 * henninge is reading the file ... ;)
<salgado> yeah, the heading there says it all
<henninge> salgado: so, I wonder why this was able to land, then?
<henninge> salgado: r=me, of course ;)
<salgado> henninge, because PQM doesn't run the testsuite?
<henninge> salgado: ;) I see it now ...
<henninge> salgado: did you see my r=me ?
<salgado> henninge, yep, thanks!
<henninge> adeuring: Hi!
<adeuring> hi henninge
<henninge> adeuring: Did you land a branch for Adi this morning?
<adeuring> henninge: yes
<adeuring> lp:bug-192750
<henninge> adeuring: ok, I have the same branch in ec2 for him. I think he wasn't aware that you were going to do that, too.
<adeuring> henninge: then you can kill the ec2 instance, I think
<henninge> adeuring: oh yeah, np with that
<henninge>  I was just wondering what happened
<henninge> adeuring: it's bug 19-3-750, though. 
<mup> Bug #19: "Display settings" forgotten when "Save" is pressed <Launchpad Translations:Fix Released by daf> <https://launchpad.net/bugs/19>
<henninge> mup: bug 193750
<mup> Bug #193750: remove references to +addticket <trivial> <ui> <Launchpad Translations:In Progress by adiroiban> <https://launchpad.net/bugs/193750>
<adeuring> henninge: yes
<salgado> henninge, I have yet another trivial one for you: http://paste.ubuntu.com/336490/
<salgado> this one reverts a couple changes I committed to mainline by accident
<henninge> salgado: I guess it's too early in the morning for you! ;)
<henninge> s/is/was/
<salgado> yeah, today didn't start very well
<henninge> salgado: shouldn't this patch be the other way round?
<henninge> I just compared it to the patch you want to revert.
<salgado> henninge, yes, the other way around is what I'm committing, sorry
<salgado> I applied it with -R, but gave you the original diff, where the change was introduced
<salgado> http://paste.ubuntu.com/336495/ is the correct one
<henninge> salgado: cool, r=me
<salgado> thanks henninge-lunch 
<henninge> Hi mrevell !
<mrevell> hi henninge
<henninge> mrevell: Do you know what help@lp.net points to?
<mrevell> henninge, yes, it points to feedback@launchpad.net
<henninge> mrevell: sure? I got an email this morning addressed to me an to help but I only saw one copy.
<henninge> I am subscribed to feedback.
<mrevell> henninge, let me checl
<mrevell> s/checl/check
<henninge> mrevell: nm, I checked, too. The other one'sin the bin ...
<mrevell> henninge, I don't see anything in the queue
<henninge> sorry
<henninge> argh!
<henninge> no, that's not it
* sinzui changed the topic of #launchpad-reviews to:  on-call: (henninge, also on CHR) || reviewing: -  || queue [sinzui, sinzui] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
* abentley changed the topic of #launchpad-reviews to:  on-call: (henninge, also on CHR), abentley || reviewing: -  || queue [sinzui, sinzui] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<EdwinGrubbs> abentley: can you review https://code.edge.launchpad.net/~edwin-grubbs/launchpad/bug-433809-picker-search-slash/+merge/15674
<abentley> EdwinGrubbs: Okay
* abentley changed the topic of #launchpad-reviews to:  on-call: (henninge, also on CHR), abentley || reviewing: -, EdwinGrubbs  || queue [sinzui, sinzui] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<noodles775> henninge or abentley: could either of you review an incremental to an MP which was approved last cycle (but not landed?). See my last comment at:
<noodles775> https://code.edge.launchpad.net/~michael.nelson/launchpad/443353-api-builds-from-private/+merge/14896
* henninge changed the topic of #launchpad-reviews to:  on-call: abentley || reviewing: EdwinGrubbs  || queue [sinzui, sinzui] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<henninge> noodles775, abentley: Sorry, got my hands full on CHR.
<noodles775> np, it's not urgent.
<abentley> noodles775: I'll queue it.
<noodles775> Thanks abentley 
* abentley changed the topic of #launchpad-reviews to:  on-call: abentley || reviewing: EdwinGrubbs  || queue [sinzui, sinzui, noodles775] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<abentley> EdwinGrubbs: In the future, you can set db-devel as the prerequisite branch and still propose merging into devel.
<noodles775> oh, abentley, on that note, I've got a pipeline of MPs that were approved, but are all proposed for db-devel (as they were dependent on schema changes). I'm keen to land them now in devel. What would you recommend as the best way?
<noodles775> (I'm about to just aggregate all the changes into a new devel branch, propose it as an MP and link to the previous approvals before ec2 land'ing it).
<abentley> noodles775: Personally, I would land the last pipe on devel using pqm-submit.
<abentley> noodles775: I'm not sure whether ec2 land will allow you to override the target.  I would assume not.
<noodles775> abentley: ok, I'll try that. Thanks!
<noodles775> (once I've QA'd it on dogfood etc.)
<abentley> noodles775: No problem.  (this implies you'll need to run the tests if you haven't already)
<noodles775> abentley: yes, I'll re-merge db-devel, run the tests, QA on dogfood, and if all goes well land that branch on devel.
<abentley> noodles775: I would recommend merging devel rather than db-devel.
* abentley changed the topic of #launchpad-reviews to:  on-call: abentley || reviewing: noodles775 || queue [sinzui, sinzui, EdwinGrubbs] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<abentley> noodles775: I'm concerned because this diff includes a lot of changes that don't seem to be in scope, e.g. YUI -> LPS.
<noodles775> abentley: I'm not sure why you say not in scope? The core change of the branch was to getBuildsForSources() - which affected the UI as outlined in the incremental?
<abentley> noodles775: Your incremental diff doesn't show the YUI -> LPS change.
<noodles775> abentley: LPS?
<abentley> noodles775: Look that the preview diff.  It has a bunch of YUI -> LPS changes in it.
<noodles775> abentley: ah, you mean the generated diff - I'm assuming that's because I've just merged devel and pushed as you suggested (but the MP is for db-devel).
<noodles775> The actual diff for this change is in the comment.
<abentley> noodles775-otp: I see.  I didn't realize you'd be asking for review on any of those.
<noodles775-otp> abentley: just the incremental. But yes, after I did that, I thought a better option would have been to add a new pipe and do it there.
<abentley> noodles775-otp: So to be clear: you actually want to land this on devel, not db-devel.
<noodles775-otp> abentley: correct. I had to develop it using db-devel last cycle, but want to land it in devel now.
<noodles775-otp> Is there a way to re-target the MP at devel?
<abentley> noodles775-otp: No, that would be a different proposal, which makes sense because there are different criteria for landing on devel.
<noodles775-otp> abentley: OK (although I can see there would be cases where it matters, but in this case it's really just the diff being reviewed).  I just thought I had memories of using hitchhiker to do it once before.
<abentley> noodles775-otp: No, hitchhiker only controls branches.  It doesn't provide a way to manipulate merge proposals.
<noodles775-otp> OK. Let me know if I should revert r9893 on that branch then.
<abentley> noodles775-otp: Don't worry about it, I'm regenerating the preview diff locally.
<noodles775-otp> ok, thanks.
<abentley> noodles775-otp: Are both of the finds necessary, or would the second find (for copied binaries) work even if SourcePackagePublishingHistory.archiveID == Build.archiveID ?
<noodles775-otp> abentley: hrm, I thought so when I wrote that a few weeks ago, but looking at the two finds now, I'm struggling to see why.
<noodles775> BTW: If you're willing to re-review the entire MP, I could create a new MP against devel.
<abentley> noodles775: Okay, I don't think it's a blocking issue, but if we can simplify by removing the first find, that would be great.  So r=me, do what you will.
<noodles775> abentley: OK, I'll try it and see. Thanks!
* abentley changed the topic of #launchpad-reviews to:  on-call: abentley || reviewing: sinzui || queue [sinzui, EdwinGrubbs] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<abentley> EdwinGrubbs: In case you didn't see earlier, I've bumped your review to the queue because you weren't around to answer my questions.
<EdwinGrubbs> abentley: I was only gone for a few minutes and then you were offline so I couldn't respond.
<abentley> EdwinGrubbs: Sorry about that, my IRC client doesn't make that very obvious.
* abentley changed the topic of #launchpad-reviews to:  on-call: abentley || reviewing: EdwinGrubbs  || queue [sinzui, sinzui] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<abentley> EdwinGrubbs: Okay, so is the conditional needed?
<EdwinGrubbs> abentley: do you mean this conditional:     if '/' in query:
<abentley> EdwinGrubbs: No, I mean in patch line 100, "if result is not None".
<abentley> EdwinGrubbs: It looks like the code should actually read "result = IStore(...).find(...).one(), so that you can raise lookuperror if there were no results.
<EdwinGrubbs> abentley: well, I assume the original purpose of that is to provide the LookupError exception as opposed to something more generic. A try/except clause could also be used, but that would be less efficient.
<abentley> EdwinGrubbs: I thought find could never return None.  Am I wrong?
<EdwinGrubbs> abentley: oh, I see what you are saying. I didn't think about the differences in the return values between sqlobject and storm. I'll test that I'm testing the right return value for an unsuccesful find().
<abentley> EdwinGrubbs: Great.  I'll leave the review alone for now.  Please ping me when you've examined that.
<EdwinGrubbs> ok
* abentley changed the topic of #launchpad-reviews to:  on-call: abentley || reviewing: sinzui  || queue [sinzui] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<salgado> abentley, I have a one-liner for a testfix branch; can I skip the queue? http://paste.ubuntu.com/336670/
<abentley> salgado: I don't know what this LPS thing is.
<salgado> abentley, I replaced all YUI() calls with LPS in our templates
<salgado> LPS is just a YUI instance, stored globally to be used in templates
<abentley> salgado: very well, r=me.
<salgado> thanks abentley 
* abentley changed the topic of #launchpad-reviews to:  on-call: abentley || reviewing: -  || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
* abentley changed the topic of #launchpad-reviews to:  on-call: abentley (on lunch) || reviewing: -  || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<sinzui> abentley: your understanding of translatables not being batched is correct.
<adiroiban> salgado: I got an email from Buildbot, but I can not access this link  https://lpbuildbot.canonical.com/builders/lp/builds/405
<adiroiban> can you please copy the part describing the build problem?
<salgado> adiroiban, no need to worry about it; you got that because my changes caused a test to fail
<salgado> I've already fixed the test and submitted it to pqm
<adiroiban> salgado: ok. thanks. I was not sure what to do with it :)
<danilos> salgado, hey, got a time to take a peek at https://code.edge.launchpad.net/~danilo/launchpad/bug-493629/+merge/15758 (it's a one line change which hopefully fixes the JS buildbot for good, patch by mars actually) - I know you are not OCR, but just so I don't have to wait for abentley to come back from lunch and I can go for dinner :)
 * salgado checks the diff
<salgado> danilos, r=me
<danilos> salgado, thanks
* abentley changed the topic of #launchpad-reviews to:  on-call: abentley || reviewing: -  || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<adiroiban> abentley: hi, Can you please review https://code.edge.launchpad.net/~adiroiban/launchpad/bug-431249/+merge/15767 ?
<abentley> adiroiban: Okay, I'll have a look in a minute
* abentley changed the topic of #launchpad-reviews to:  on-call: abentley || reviewing: -  || queue [adiroban] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<adiroiban> abentley: thanks.  no hurry :)
* abentley changed the topic of #launchpad-reviews to:  on-call: abentley || reviewing: adiroban  || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<abentley> adiroiban: Has this branch been through a complete test suite run?
<adiroiban> abentley: nope. Only the translation tests.
<adiroiban> on my computer
<abentley> adiroiban: It's approved, so let me know when it's been through a complete test suite run, and I'll land it.
<adiroiban> abentley: should I run the full test on my computer?
<abentley> adiroiban: Yes, that's fine.
<adiroiban> on the previous one Henning launched the test on some supercomputer 
<adiroiban> running all tests on my computer would take many hours... 
<abentley> adiroiban: I run all the tests on my computer.
<adiroiban> abentley: thanks. 
<abentley> adiroiban: There's no supercomputer, he would have just used Amazon's Elastic Compute Cloud.
<adiroiban> can I run the complte test on a remote computer with no X libs? I have a shell account on a remote computer I can configure it for testing my branches
<adiroiban> but I'm not sure about testing Javascript
<abentley> adiroiban: Yes, I don't think the test suite uses x libs.  The javascript testing isn't integrated into test runs anyhow.
<adiroiban> abentley: ok. thanks. Then I'll start configuring that machine.
<EdwinGrubbs> abentley: I fixed the handling of the return value from find() and I pasted the incremental diff into https://code.edge.launchpad.net/~edwin-grubbs/launchpad/bug-433809-picker-search-slash/+merge/15674
<abentley> EdwinGrubbs: Cool.  On the phone.
* sinzui changed the topic of #launchpad-reviews to:  on-call: abentley || reviewing: adiroban  || queue [sinzui] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<thumper> rockstar: shall I send a few reviews your way?
<rockstar> thumper, sure.  Are they urgent?
<thumper> thumper: isn't everything?
 * thumper facepalms
<thumper> rockstar: I'll fire the ui one your way
<thumper> rockstar: I just have others that you may be interested in
<rockstar> thumper, okay, fire them my way.
<thumper> rockstar: this is the ui one https://code.edge.launchpad.net/~thumper/launchpad/hiding-review-fields/+merge/15722
<thumper> rockstar: that is just a "get the branch and look at it" type review
<thumper> rockstar: and others around the claim/delete review arena:
<thumper> https://code.edge.launchpad.net/~thumper/launchpad/claim-review-into-model-browser-integration/+merge/15719
<thumper> https://code.edge.launchpad.net/~thumper/launchpad/claim-review-into-model-attempt2/+merge/15728
<thumper> https://code.edge.launchpad.net/~thumper/launchpad/delete-pending-reviews/+merge/15739
* abentley changed the topic of #launchpad-reviews to:  on-call: || reviewing: -  || queue [sinzui] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<bac> sinzui: still around?
#launchpad-reviews 2009-12-08
<thumper> rockstar: how's it going?
<rockstar> It's going.
<thumper> rockstar: I'd like to talk to you about claim/reassign review when you have a mental break
<rockstar> thumper, okay.
<mwhudson> thumper: want to review this bzr-svn-ui branch?
<thumper> mwhudson: sure
<mwhudson> thumper: https://code.edge.launchpad.net/~mwhudson/launchpad/bzr-svn-ui/+merge/15792
<mwhudson> thumper: were you waiting on any reviews from me?
<thumper> mwhudson: no
<thumper> mwhudson: rockstar got to them before you :)
<mwhudson> thumper: :-)
<mwhudson> thumper: it seems your conditional tag on your approve vote confuses ec2 land :(
<jml> sorry.
<mwhudson> jml: i think it's thumper's fault really ;-)
 * mwhudson eods
<jml> mwhudson, ec2 land should probably embrace Postel's Law.
<jml> mwhudson, g'night.
<thumper> mwhudson: why is it my fault?
<mwhudson> thumper: just being silly
<jml> hello 
<jml> simple patch up for review
<jml> https://code.edge.launchpad.net/~jml/launchpad/db-revision-bug-487628/+merge/15805
<jml> danilos, it fixes a bug you filed, so perhaps you might like to do that.
<henninge> jml: did you check that the exception text does not appear in any doctest?
<henninge> ... and would need to be adapted there.
<jml> henninge, I checked the class name, but not the text itself. I'll check that now.
<jml> henninge, all good.
<adiroiban> danilos, can you please take a look at this review https://code.edge.launchpad.net/~adiroiban/launchpad/bug-406477/+merge/15793 ?
<adiroiban> it's about changing launchpad.Admin to launchpad.TranslationsAdmin
<adiroiban> but only if you have time
* gmb changed the topic of #launchpad-reviews to: on-call: || reviewing: -  || queue [sinzui] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
* gmb changed the topic of #launchpad-reviews to: on-call: gmb || reviewing: -  || queue [sinzui] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<gmb> *sigh*
<allenap> gmb: Fancy a js review? https://code.edge.launchpad.net/~allenap/launchpad/no-display-name-from-api-bug-491334/+merge/15808
<gmb> allenap: Sure.
<allenap> gmb: Thank you.
* gmb changed the topic of #launchpad-reviews to: on-call: gmb || reviewing: allenap  || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
 * gmb hates at the whole typeof() != "undefined" bollocks.
<gmb> Grr.
<gmb> allenap: r=me
* gmb changed the topic of #launchpad-reviews to: on-call: gmb || reviewing: lunch  || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
 * gmb takes this opportunity to truffle for nourishment
<allenap> gmb: Thank you!
<allenap> gmb: Agreed, typeof should be taken out back, shot in the head and thrown in the wheelie bin.
<allenap> gmb: Actually, I'm going to remove the typeof() again, and replace it with window.LP !== undefined. Referencing it via the window stops it from blowing chunks down the inside of my monitor.
* bac changed the topic of #launchpad-reviews to: on-call: gmb,bac || reviewing: lunch,-  || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
* bac changed the topic of #launchpad-reviews to: on-call: gmb,bac || reviewing: lunch,jpds  || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
* bac changed the topic of #launchpad-reviews to: on-call: gmb,bac || reviewing: lunch, jpds  || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
* gmb changed the topic of #launchpad-reviews to: on-call: gmb, bac || reviewing: -, jpds  || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<gmb> Hah, accidentally the longest lunch break evar.
<gmb> Morning bac.
<bac> hey gmb
* gmb changed the topic of #launchpad-reviews to: on-call: gmb, bac || reviewing: sinzui, jpds  || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<jpds> Afternoon bac, gmb. :)
<gmb> Hi jpds.
<bac> hi jpds -- just looking at your branch now
<jpds> Awesome.
 * gmb is amazed that sinzui's StructuralSubscriptions change is only 641 lines.
<gmb> Unless there's a zero missing in the linecount...
<sinzui> I was too. I was expecting 2000 lines.
<sinzui> gmb: I think the structuralsubscription link mixin that intellectronica added two releases ago dramatically reduced the number of callsites.
<gmb> sinzui: Yeah, I was thinking along the same lines.
<gmb> sinzui: r=me, anyway.
<sinzui> faboo
<bac> jpds: i just tried running your branch and registering a new mirror.  it throws an exception because newMirror() doesn't take a whiteboard argument.
* gmb changed the topic of #launchpad-reviews to: on-call: gmb, bac || reviewing: -, jpds  || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
* gmb changed the topic of #launchpad-reviews to: on-call: gmb, bac || reviewing: -, jpds  || queue [gmb] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<gmb> bac: When you're free, will you be able to do a JS / Code review for https://code.launchpad.net/~gmb/launchpad/ajax-dupefinder-has-risen-from-the-grave/+merge/15599?
<bac> gmb: sure
<gmb> bac: Thanks.
* bac changed the topic of #launchpad-reviews to: on-call: gmb, bac || reviewing: -, gmb  || queue [-] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
* gmb changed the topic of #launchpad-reviews to: on-call: bac || reviewing: gmb  || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<gmb> bac: I'm going to go afk for a while now but I'll be back later. Feel free to ask any questions you may have in the review and I'll get back to you when I can.
<bac> gmb: ok.  sorry i didn't get to it yet.
<gmb> bac: No worries.
* bac changed the topic of #launchpad-reviews to: on-call: bac || reviewing: lunch, then gmb  || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<danilo_> bac, hi, can you take a look at https://code.edge.launchpad.net/~danilo/launchpad/bug-494106/+merge/15822?
<danilo_> bac, when diff shows up, at least :)
<adeuring1> bac: can you please review this MP: https://code.edge.launchpad.net/~adeuring/launchpad/bug-494075/+merge/15823 ?
<danilos> bac, actually, https://code.edge.launchpad.net/~danilo/launchpad/bug-494106/+merge/15824 for me (the former one was against a wrong LP branch, diff too big)
* danilos changed the topic of #launchpad-reviews to: on-call: bac || reviewing: lunch, then gmb  || queue [danilo, adeuring] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<danilos> gmb, how about you? :)
* bac changed the topic of #launchpad-reviews to: on-call: bac || reviewing: gmb  || queue [danilo, adeuring] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<bac> danilos, adeuring: will get to them shortly
<bac> hi gmb
* bac changed the topic of #launchpad-reviews to: on-call: bac || reviewing: adeuring  || queue [gmb,] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<bac> adeuring: your MP has a diff against db-devel at 4K lines.  this should be against devel, no?
<adeuring> bac: yes, sorry for the mess. The real diff should be quite short, 100 lines or so.
<bac> adeuring: i've had quite a lot of problem lately with MPs getting targeted to the wrong branch.
<bac> i wish it was possible to retarget the MP
<adeuring> adeuring: ah, thanks for letting me know! I already began to question my abilities to properly select radio buttons on web pages ;)
<bac> adeuring: actually i always use 'bzr send' and only use the web ui to make corrections when the wrong target is used
<adeuring> bac: yeah, I should get used to that too
<adeuring> bac: anyway, this is the dif again teh devel branch_: http://paste.ubuntu.com/337502/
<bac> adeuring: thanks
<jpds> bac: Can you look at the changes I've made at https://code.edge.launchpad.net/~jpds/launchpad/fix_196173/+merge/15807 when you have time? Thanks!
<adeuring> bac: thanks for your review!
<bac> np abel.  thanks for the branch
<bac> jpds: the changes look good.  i'd like to see doc/distribution-mirror.txt updated to show the whiteboard being set.
<jpds> bac: Something like http://pastebin.ubuntu.com/337541/ ?
<bac> jpds: that's good.
<jpds> bac: Pushed.
<bac> jpds: do we need to discuss with the mirror administrators the fact that registrants can now see the whiteboard?  i assume there are no embarassing comments on the whiteboards but it might be a good heads up that the policy has changed.
<jpds> bac: I checked with spm last night to make sure there was nothing... unpolitical there.
 * jpds â msh.
<jpds> s/h$/g/
<bac> jpds: right, but a message for people going forward might be nice.  is there a mailing list for the team you can post to?
<jpds> bac: The interested parties are subscribed to the bug report. :)
<bac> jpds: and now there is a comment...
<bac> jpds: nice work.  i'll go update the MP to show it approved.  do you need me to land it for you?
<jpds> I don't think I can operate PQM.
<bac> ok. i'll do it
<jpds> Thanks!
<bac> sinzui: will you take a glance at: http://pastebin.ubuntu.com/337544/?  let's call it a mid-flight rather than review.
* bac changed the topic of #launchpad-reviews to: on-call: bac || reviewing: -  || queue [gmb,] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<bac> gmb are you around?
<jpds> bac: Oh, and we've operated on the belief that they could already see it: http://spooky.ubuntuwire.org/~jpds/screenshot_002.png - the irony. :(
<sinzui> bac: I like the work done so far.
<sinzui> bac: how to do propose to test it? verify that our mime-types knows what bz2 is?
<bac> sinzui: oh, i already added a test but forgot to add it to bzr
<bac> sinzui: http://pastebin.ubuntu.com/337558/
<sinzui> bac: looks good.
<bac> sinzui: great.  i'll make a MP and let you rubber stamp it!
<sinzui> bac: Yes. I will approve this to land. Do you want to treat the data fix as a separate bug since it needs separate deployment?
<bac> sinzui: yes
<bac> i'll open that now
<bac> sinzui: i'm beginning to wonder if i took the right approach on the mimetypes fix
<sinzui> bac: do you want to load a mime.types file?
<bac> sinzui: no, i think i'd rather provide a method that adds the extra bits as i do now but have it invoked "early" so that everyone who imports the standard mimetypes will get the modified version.
<sinzui> I think that is kind of magic
<sinzui> bac: considering that we are designing for multiple distros and pythons, I like explcit
<sinzui> bac: Consider how difficult debugging will be if it is not clear which mimetypes you are using
<bac> sinzui: consider what's happening in model/bug.py
<bac> sinzui: they muck with mimetypes but don't explicitly use it.  it is used by zope.contenttype later
<sinzui> I think the bug example should be using your approach. bug 229040 illustrates we may have to live this this a long time
<mup> Bug #229040: debdiff shown as html <tech-debt> <Launchpad Bugs:Fix Released by allenap> <mime-support (Ubuntu):New> <https://launchpad.net/bugs/229040>
<bac> sinzui: i can move the add_type for the deb diff into my lp.services.mime but then getting it invoked is a problem
<bac> i've chatted with gary some and what i'd like to do is convert what i have in lp.services.mime into an initialization method
<bac> and then call that method from site.py, which will ensure everyone who then uses the standard mimetypes will get one that has been customized
<sinzui> bac: okay
<bac> sinzui: the mod is easy so i'll run it by you shortly
#launchpad-reviews 2009-12-09
<adiroiban> bac: can you please check the status of this MP https://code.edge.launchpad.net/~adiroiban/launchpad/bug-193750 ?
<adiroiban> does it needs any action from me?
<bac> adiroiban: if you've made the changes requested by abel you just need to catch up with him and ask him to give it a once-over and land for you.  sorry i don't have the time right now to look at it in detail.
* bac changed the topic of #launchpad-reviews to: on-call: - || reviewing: -  || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<adiroiban> bac: np :) just wanted to know what is the next step . thanks!
<bac> adiroiban: np.  thanks for the fix and run down abel to get it landed.
<noodles775> Hi thumper: I can't merge your branch without conflicts? http://pastebin.ubuntu.com/337836/
<thumper> noodles775: I'll fix the conflicts
<noodles775> k
<thumper> noodles775: pushed
<noodles775> ta.
<thumper> noodles775: mechanical Y.get -> Y.one changed
<thumper> hope everything still works
<thumper> :)
<wgrant> Can somebody please review https://code.edge.launchpad.net/~wgrant/meta-lp-deps/lp-soyuz-deps/+merge/15836?
<noodles775> wgrant: allenap should be around soon (see https://dev.launchpad.net/ReviewerSchedule).
<allenap> noodles775: Hello there :)
* allenap changed the topic of #launchpad-reviews to: on-call: allenap || reviewing: -  || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<noodles775> ah, timing :-)
<allenap> wgrant: Hello, I'll take a look.
<allenap> wgrant: r=me, but I'd like you to ask someone from soyuz to take a quick look too. Thanks :)
<wgrant> bigjools: Can you give https://code.edge.launchpad.net/~wgrant/meta-lp-deps/lp-soyuz-deps/+merge/15836 a glance, as discussed earlier?
<wgrant> allenap: Thanks.
<bigjools> wgrant: sure, but it won't be for ~3 hours
<wgrant> bigjools: OK.
<gmb> allenap: Morning. Care to take a look at code / js for https://code.edge.launchpad.net/~gmb/launchpad/ajax-dupefinder-has-risen-from-the-grave?
<allenap> gmb: I'd love to :)
<gmb> Ta :)
* allenap changed the topic of #launchpad-reviews to: on-call: allenap || reviewing: gmb  || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<allenap> gmb: I've merged Frankendupe into a recent devel and I get a syntax error in +filebug.
<gmb> Poo
<gmb> allenap: I'll look into that.
<allenap> gmb: filebug-dupefinder.js line 260
<gmb> allenap: Ah, I see it. My bad; didn't lint after merging. I'll fix it presently.
<gmb> allenap: Fixed and pushed
<allenap> gmb: Cool.
<gmb> Argh. Bloody DB patches.
<adeuring> allenap: could you please review this MP: https://code.edge.launchpad.net/~adeuring/launchpad/bug-344054/+merge/15857 ?
<allenap> adeuring: Sure. I'm doing gmb now, but you're next.
* allenap changed the topic of #launchpad-reviews to: on-call: allenap || reviewing: gmb  || queue [adeuring] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<gmb> 0.o
<adeuring> allenap: thanks!
<allenap> gmb: What does that mean?
<gmb> allenap: It's the equivalent of a raised eyebrow or surprise or... something. Was responding to something that I think I've managed to do wrong... gimme a sec here...
<allenap> gmb: Ah, okay.
<gmb> Argh, looks like the last merge broke something.
<gmb> Wow, heroic amount of suck from chromium this morning...
<allenap> gmb: Shall I do Abel's while you're fixing that, or shall I just review what's there now?
<gmb> allenap: Well, quick question: do you know what LPS is as opposed to using YUI?
<gmb> Cos I think that's what's broken.
<allenap> gmb: Ah, yes, instead of using new YUI(), just use LPS.
<gmb> allenap: But then can I still use YUI.add() in filebug-dupefinder.js?
<gmb> I'm seeing lots of "YUI is not defined" errors
<allenap> gmb: Yeah, that should work.
<allenap> gmb: How odd.
<gmb> Hmm.
<gmb> allenap: Okay, feel free to take a look at abel's. I'll get back to you shortly.
<gmb> Argh. "LPS is not defined" Gaaaah.
<allenap> gmb: Yeah. I didn't see anything on the list about this LPS change, and it's quite a big change, grumble, grumble :-/
<gmb> allenap: Yes. This is... suboptimal.
<noodles775> gmb: make clean, make should fix that I think.
<gmb> noodles775: Ah, thanks.
* allenap changed the topic of #launchpad-reviews to: on-call: allenap || reviewing: adeuring  || queue [gmb] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<gmb> noodles775: Incidentally, did you see my reply to your UI review?
<noodles775> gmb: I did get to skim through it yesterday, I'll take a better look now. Sorry :/
<gmb> noodles775: No worries. I'd like to get this landed today though, even if I have to do a lot of small branches to get it prettified for release. It's a big delta to be carting around between cycles.
<noodles775> gmb: one thought I did have (not ui, but related to the code), there should be a better way to avoid the 'hitting enter causing the search to be re-run'.
<noodles775> You should be able to catch the onsubmit event and disable the default behaviour?
<noodles775> er, that should be the other way around... not avoid, but ensure.
<gmb> noodles775: ISTR trying that several cycles and it failing miserably, which is why I went straight for the keypress. But you're right; I'll give it another shot.
<gmb> *cycles ago
 * noodles775 remerges the branch.
<allenap> adeuring: My gut feeling is that regular_attachments and patches should be view properties rather than model properties. Did you consider that?
<adeuring> allenap: right, good idea. didn't think about that
<gmb> allenap: YUI() changed to LPS... the latest revision should work fine now.
<allenap> adeuring: My feeling is that they should not be exported in any case. If you're a consumer of the API, I think it's better to get the list of all attachments and split it up from there; the criteria for a patch are unambiguous and easy to test. Exporting two (well, three) attachment fields, we also encourage people to make multiple API calls.
<allenap> gmb: Cool. I should have said that it was working for me earlier, and very nice it is too :)
<adeuring> allenap: yeah; I'll use browser methods
<allenap> adeuring: Thanks, and sorry for being a downer.
<adeuring> allenap: np ;)
<gmb> allenap: Well, I needed to update YUI -> LPS anyway, so no worries :)
<gmb> noodles775: You're right; I've no idea what  Iwas doing wrong before. I've fixed that.
* allenap changed the topic of #launchpad-reviews to: on-call: allenap || reviewing: gmb || queue [adeuring] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<noodles775> Great gmb !
<gmb> allenap: Please pull the latest revision again; I've stopped using the button's onClick event and started catching the form's onSubmit instead as noodles775 suggested.
<allenap> gmb: Cool, will do.
<noodles775> gmb: ui=me, thanks for all the improvements!
<gmb> noodles775: Thanks!
<bac> hi gmb
<bac> gmb: i looked at your branch yesterday but you'd already left.  i couldnt' get anything to work.  perhaps that's what youv'e been discussing here.
<gmb> Hi bac.
<bac> none of the advertised JS stuff did anything
<bac> twisties didn't twist
<gmb> bac: Possibly. There was a syntax error in the JS after a merge.
<gmb> Apologies for that; I didn't lint after merging.
<bac> gmb: ok.  sorry i had to punt on it
<gmb> No worries.
<jpds> bac: It seems that there was a syntax error in lp:~jpds/launchpad/fix_196173 that I didn't catch yesterday, I've pushed a fixed revision.
<bac> jpds: ok, i haven't looked at the results from the tests yet
<adeuring> allenap: http://paste.ubuntu.com/337919/
<jpds> bac: I've made sure that all registry tests pass on the branch, not just the distributionmirror ones.
<allenap> adeuring: Cool, I'll look at that in a bit.
<bac> jpds: ok.  i'll get your new version and submit it shortly
<bac> jpds: your branch was submitted to ec2 about 30 minutes ago
<jpds> bac: Neat, thanks.
 * gmb -> lunch
* allenap changed the topic of #launchpad-reviews to: on-call: allenap || reviewing: gmb || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<allenap> gmb: I'm still doing your review. I'll finish it when I'm back, but I've done the first 250 lines here if you want to take a look: http://pastebin.ubuntu.com/337964/
<gmb> allenap: Awesome, thanks
<adiroiban> henninge: hi. is there anything I need to do for https://code.edge.launchpad.net/~adiroiban/launchpad/bug-193750/+merge/15292 ?
<henninge> adiroiban: was it not merged yet?
<adiroiban> henninge: the status is still Aproved
<henninge> adiroiban: didn't you get emails like "success" for your branches?
<henninge> adiroiban: also you could just update your devel and diff your branch against it to see if it is merged.
<henninge> adiroiban: because I see that it was merged. Dunno why the MP did not pick up on it.
<henninge> it may have to do with the fact that I had to re-submit it directly to PQM because of a testfix mode bounce.
<henninge> adiroiban: can you change the status of the MP to merged, or can only I do that?
<gmb> allenap: I've taken care of those items; If you post that part of your reviewto the MP I'll reply to it.
<adeuring> noodles775, beuno: could one of you please ui-review this branch. https://code.edge.launchpad.net/~adeuring/launchpad/bug-344054/+merge/15857 ? (now asked in the right channel...)
<noodles775> adeuring: I can, but it's worth checking with sinzue or other UI reviewers first (so we all get to learn/practise).
<noodles775> s/sinzue/sinzui
<adeuring> ah, right, so, sinzui: ^^^
<sinzui> adeuring: I can do that in 15 minutes
<adeuring> sinzui: thanks!
<al-maisan> hello allenap, could you please take a look at https://code.edge.launchpad.net/~al-maisan/launchpad/ejdt-484819/+merge/15872 when you get to it?
* al-maisan changed the topic of #launchpad-reviews to: on-call: allenap || reviewing: gmb || queue [al-maisan] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<allenap> al-maisan: Sure, it will be my pleasure :)
<al-maisan> allenap: thanking you.
* EdwinGrubbs changed the topic of #launchpad-reviews to: on-call: allenap, Edwin || reviewing: gmb || queue [al-maisan] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<EdwinGrubbs> allenap: do you have any reviews waiting that you would like me to take?
<allenap> EdwinGrubbs: al-maisan has a branch that I haven't looked at yet. I'm sure he'd appreciate it.
<EdwinGrubbs> sure
<al-maisan> EdwinGrubbs: it's here: https://code.edge.launchpad.net/~al-maisan/launchpad/ejdt-484819/+merge/15872
<al-maisan> Thank you very much for looking at it!
<gmb> allenap: How's that branch looking?
<allenap> gmb: Sorry, I've been distracted by phoning Philips about the very large number of expired light bulbs I have.
<allenap> gmb: I'm back on it now.
<gmb> allenap: Okay. Can you post the part of the review you did earlier to the mp so I can reply to it? Would help me keep track a bit, I think.
<allenap> gmb: Sure, good idea!
<gmb> allenap: So, that's *just* for the JS, right? Just being clear since there are two reviews requested there.
<allenap> gmb: Yes, I've just added another review for the first part of the code.
<gmb> Cool
<adeuring> sinzui: did ou forget the ui review ;)?look at the 
<EdwinGrubbs> al-maisan: in Build.createBuildQueueEntry() will it always create a BuildQueue with job_type=BINARYBUILD, or is this just temporary since this is an intermediate branch?
<sinzui> adeuring: I am playing with it now
<adeuring> sinzui: ah, OK, thanks
<sinzui> adeuring: I was distracted for 30 minutes
<adeuring> sinzui: no problem
<al-maisan> EdwinGrubbs: it will always do it .. other job type classes will have their own equivalents of createBuildQueueEntry() that set the job type and the estimated job duration as is appropriate to them
<al-maisan> other job type classes are forthcoming and will be something along the lines of "Create source package from recipe"
<al-maisan> .. or translations import
<EdwinGrubbs> al-maisan: should the Build class be renamed BinaryBuild then?
<al-maisan> EdwinGrubbs: yes, eventually it will be.
<al-maisan> likewise for the db table that underpins it
<allenap> gmb: Ah, it seems the merge proposal system doesn't recognise my approval for both js and code.
<gmb> allenap: Hah.
<gmb> allenap: Well, thanks anyway :). I'll make the necessary changes and land it posthaste.
<allenap> EdwinGrubbs: Could you add a "code" approved review to https://code.edge.launchpad.net/~gmb/launchpad/ajax-dupefinder-has-risen-from-the-grave/+merge/15599, saying that I reviewed it. It doesn't seem to recognise my approval for both js and code.
<EdwinGrubbs> allenap: I can do that, but I usually just see reviewers enter "ui code" when they are reviewing both.
<allenap> EdwinGrubbs: I reviewed js before I did code, so I wasn't able to do that. Sounds like I need to file a bug.
<EdwinGrubbs> allenap: You can submit another review, and it will overwrite the review type at the top of the page.
<gmb> allenap: Does the frontpage +filebug form still exist? I can't remember how to get to it.
<allenap> gmb: I don't think so.
<gmb> allenap: Cool.
<EdwinGrubbs> allenap: do you still want me to add a separate review?
<sinzui> abel: I replied with a suggestion. We can talk about it if you have misgivings
<gmb> allenap: Just do a quick review with the message type to code js
<gmb> It will overwrite your old one.
<gmb> I'll delete the second request.
<gmb> Except that I can.
<gmb> 't
<gmb> Gah.
<gmb> allenap: All that I care about is that you mark the MP as approved ;)
<allenap> gmb: Done.
<gmb> Thanks.
<EdwinGrubbs> al-maisan: is the _delayCausedByPendingJobsAhead() called very often? I'm wondering if it is worthwhile to have it perform a more efficient query. It doesn't look like pendingJobsQuery() is called anywhere else. Am I missing something?
<allenap> EdwinGrubbs: I submitted js and code reviews separately at first. I just edited my review at the top to be "code js", but it still shows a review request for ~launchpad. As long as it doesn't show as needing a review in https://code.edge.launchpad.net/launchpad-project/+activereviews then it's fine. Thanks.
<allenap> EdwinGrubbs: I think there is a bug there; your review is showing as Pending.
<EdwinGrubbs> allenap: do you mean my review of muharem's branch? I just claimed the review five seconds ago.
<allenap> EdwinGrubbs: Did you also claim the review of gmb's branch? https://code.edge.launchpad.net/~gmb/launchpad/ajax-dupefinder-has-risen-from-the-grave/+merge/15599
<gmb> allenap: Does that review matter any more? The branch should be merged before EoD.
<allenap> gmb: No, the review itself doesn't; it's all good. But I'm interested in figuring out if there's a bug to report.
<EdwinGrubbs> allenap: oh, yes, I did that, but when I went to +activereviews, I saw that it was in the right place, so I didn't go back to the mp. I'll add my fake review to avoid any more confusion.
<allenap> EdwinGrubbs: Okay, sorry for the noise.
<al-maisan> EdwinGrubbs: _delayCausedByPendingJobsAhead() will be called for individual, pending build jobs
<al-maisan> the idea re. pendingJobsQuery() is that each BuildFarmJob class implements it
<al-maisan> then the "central" estimation logic can collect the SELECT clauses from each of them and hit the db with a single SELECT/UNION query
<EdwinGrubbs> al-maisan: so this will just be called in a cronjob, it won't be called each time a certain page is loaded?
<al-maisan> EdwinGrubbs: the latter
<al-maisan> EdwinGrubbs: here's an example: https://edge.launchpad.net/ubuntu/+source/darkroom/1.5.0~svn1037635-0ubuntu2/+build/1387612
<al-maisan> I was trying to put in place a general job dispatch time mechanism/scheme in which all build farm job types can participate ..
<al-maisan> .. provided they implement 3 callback functions
<al-maisan> 1: pendingJobsQuery
<al-maisan> 2: processor
<al-maisan> 3: virtualized
<EdwinGrubbs> al-maisan: Are you worried about this slowing down the page when their are a lot of pending build requests?
<al-maisan> EdwinGrubbs: maybe .. 2 other build farm job types will be added in the near future
<al-maisan> i.e. what is 1 query now will become "SELECT .. UNION SELECT .. UNION SELECT .."
<al-maisan> EdwinGrubbs: I am obviously open to suggestions how to make the estimation of job dispatch times perform better
<EdwinGrubbs> al-maisan: I'm wondering if it would be feasible to pass in the id, virtualized and processor.id values of the current build into pendingJobsQuery so that the query can sum up the durations for you. Of course, I don't know if the other unimplemented job types would complicate this.
<adeuring> beuno, noodles775: could one of you please have a ui-look at this mp: https://code.edge.launchpad.net/~adeuring/launchpad/bug-344054/+merge/15857 ?
<noodles775> adeuring: I can do it first thing tomorrow if beuno doesn't beat me to it.
<al-maisan> EdwinGrubbs: I am not sure this will work because 1 - the final order of the jobs is only established by an ORDER BY clause on the entire "SELECT .. UNION SELECT .. UNION SELECT .." query
<adeuring> noodles775: thanks!
<beuno> adeuring, noodles775, I can take care of it if it has a screenshot
<adeuring> beuno: just a second...
<al-maisan> EdwinGrubbs: 2 - even though a job is ahead in the queue wrt our job of interest, it may not compete with the latter for builders i.e. it should be ignored for the purpose of estimating the dispatch delay
<al-maisan> EdwinGrubbs: what I could probably do is: pump the "SELECT .. UNION SELECT .. UNION SELECT .." result set into a temporary table and continue to query that as opposed to performing calculations in the python domain
<al-maisan> EdwinGrubbs: However, I'd like to do that only if and when we run into performance issues
<EdwinGrubbs> al-maisan: Regarding #1, why does the order of the results matter? Your for-loop over the results doesn't seem to depend on the order, since the pendingJobsQuery already filters out jobs with a lower score.
<al-maisan> EdwinGrubbs: good question, let me think about that for a moment
<adeuring> beuno: http://people.canonical.com/~adeuring/attachments-patches.png (new portlet "patches"; changes link text "add attachment or patch" at the bottom
<beuno> adeuring, I think sinzui did a great review, I'll approve as well
<adeuring> beuno: great, thanks!
<EdwinGrubbs> al-maisan: Regarding #2, the query can ignore the right rows if you pass in the id, processor.id and virtualized parameters. For example: SELECT SUM(estimated_duration) FROM .. WHERE .. AND BuildQueue.id != my_id AND (my_processor IS NULL OR Build.processor IS NULL OR (Archive.require_virtualized = my_virtualized AND Build.processor = my_processor)
<al-maisan> EdwinGrubbs: these are very good ideas! Let me think about them a bit more and I'll come back to you. Thank you drilling down into these queries!
<al-maisan> *for drilling..
* allenap changed the topic of #launchpad-reviews to: on-call: Edwin || reviewing: al-maisan || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<bigjools> allenap: lol, and thanks for the review
<allenap> bigjools: Welcome :)
<bigjools> allenap: it's a shame optparse can't deal with non-option args (or can it?)
<allenap> bigjools: You can use a callback to absorb more than one argument, but I think it still needs a named arg to trigger it.
<al-maisan> EdwinGrubbs: thank you again for your thought-provoking questions, I am retracting the MP and will rework the branch.
<EdwinGrubbs> ok
<jpds> salgado, bac: https://code.edge.launchpad.net/~jpds/launchpad/fix_50580/+merge/15892
<wgrant> bigjools: Are you going to have time to look at the lp-soyuz-deps branch today?
<allenap> wgrant: I was going to badger him about it earlier but decided against it, so bigjools, consider yourself retrospectively poked ;)
<wgrant> I'd reeeally like to avoid this slipping to week 4, as then it's bound to slip to January.
<bigjools> wgrant: it looks ok to me
* EdwinGrubbs changed the topic of #launchpad-reviews to: on-call: Edwin || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<wgrant> bigjools: Thanks.
 * wgrant merges.
<wgrant> Actually, not sure if I'm allowed to.
<lfaraone> EdwinGrubbs: hey, I have a very trivial fix to bug 494055 in http://bazaar.launchpad.net/~lfaraone/launchpad/bugfix494055/revision/10008. Per PatchSubmission, I'm poking you.
<mup> Bug #494055: "Report bug upstream" link uses literal "&amp;" in URL. <Launchpad Bugs:New> <https://launchpad.net/bugs/494055>
<EdwinGrubbs> lfaraone: I can look at that.
<lfaraone> EdwinGrubbs: awesome, thanks.
* sinzui changed the topic of #launchpad-reviews to: on-call: Edwin || reviewing: - || queue [sinzui] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<sinzui> EdwinGrubbs: do you have time to review https://code.launchpad.net/~sinzui/launchpad/false-packages/+merge/15901
<lfaraone> EdwinGrubbs: do I need to propose the branch for merging or something similar?
<EdwinGrubbs> lfaraone: yes, you need to do that. Sorry, I hadn't looked at your link yet. If it is a simple fix, you don't have to worry about a big description.
<EdwinGrubbs> sinzui: sure
<lfaraone> EdwinGrubbs: set to merge into lp:launchpad/devel, ~edwin-grubbs as reviewer.
<EdwinGrubbs> lfaraone: the change looks good, but you will definitely need to fix the test lib/lp/bugs/doc/bugtracker.txt. You can run that test with the command:   ./bin/test -vvt bugtracker.txt
<EdwinGrubbs> lfaraone: it would also be a good idea to run all the bug tests with the command: ./bin/test -vvt lp/bugs
<EdwinGrubbs> lfaraone: I can help you get this merged after you make the changes.
<lfaraone> EdwinGrubbs: oh, right.
#launchpad-reviews 2009-12-10
<EdwinGrubbs> sinzui: did you already test your query's performance on staging?
<sinzui> No. I have not
<sinzui> I am happy to test rather than land this and fail it in QA
 * sinzui has already failed the first effort
<sinzui> EdwinGrubbs: I am trying to force the data to look like the old test. I have had no success.
<EdwinGrubbs> sinzui: according to launchpad_dev, DistroSeriesPackageCache.name does not have an index. It would be good to test the query with and without the index on staging.
<sinzui> oh wow
<sinzui> We search on that. I guess we only search on the fti field
<sinzui> okay. I will test before considering a landing
<lfaraone> EdwinGrubbs: pushed up rev 10009 on lp:~lfaraone/launchpad/bugfix494055, it should fix your concerns.
<EdwinGrubbs> lfaraone: looking at it now
 * thumper looks for a reviewer
<thumper> EdwinGrubbs: still on?
* EdwinGrubbs changed the topic of #launchpad-reviews to: on-call: - || reviewing: - || queue [sinzui] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<EdwinGrubbs> thumper: well, if it's really small I can take it.
<thumper> EdwinGrubbs: heh, it isn't but it is pretty cool
<thumper> EdwinGrubbs: feel free to pass for now
<EdwinGrubbs> thumper: sorry, my day really ended a while ago.
<sinzui> thumper: I can do in a few minutes
<thumper> sinzui: awesome - https://code.edge.launchpad.net/~thumper/launchpad/revisions-in-conversation/+merge/15910
<thumper> sinzui: the diff should be arriving shortly
<sinzui> fab
<thumper> sinzui: it is there now
<thumper> sinzui: I'm just popping out to collect the girls from school
<thumper> sinzui: back shortly
<EdwinGrubbs> lfaraone: your test fix looks good. I just am experiencing a weird error in the test data. I will land your branch tomorrow.
<lfaraone> EdwinGrubbs: cool, thanks. 
<thumper> sinzui: back
<thumper> sinzui: are you checking it out?
<thumper> sinzui: I have to head off in about 5-10 minutes
<thumper> sinzui: so any questions you have, ask now
<sinzui> thumper: I am reading it, but my son insists on a new diaper
<thumper> heh
<adiroiban> abentley: hi. is there anything more I need to do for having this branch landed: https://code.edge.launchpad.net/~adiroiban/launchpad/bug-431249/+merge/15767
* noodles775 changed the topic of #launchpad-reviews to: on-call: noodles775(UI) || reviewing: - || queue [sinzui] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<danilos> adiroiban, btw, I've sent you another review for that permissions branch
<danilos> adiroiban, I can also help you land bug-431249 if you want me to :)
<adiroiban> danilos: thanks . I'll look at it
<adiroiban> feel free to land that branch :)
<danilos> adiroiban, there's still some work to be done on it, but I am around for any questions (it's probably going to be faster if you ask me here :)
<adiroiban> danilos: ok
<danilos> adiroiban, ok, I am landing your branch as well
<adiroiban> danilos: thanks. still reading the review :)
<danilos> adiroiban, heh, right :)
<adiroiban> danilos: how is the inheritance working for classes from security.py ? I think's I'm missing a python mechanism
<adiroiban> checkAuthenticated return True 
<adiroiban> and if it returns None, it will call super ?
<danilos> adiroiban, well, it's working just like everywhere else... it inherits the methods code, allows overriding, but if you call super-class methods and pass in "self", you are calling super-class methods on current object, which must be compatible then
<danilos> adiroiban, you have to call super-class checkAuthenticated manually
<adiroiban> ok
<adiroiban> I don't understand why you need to use inheritance
<adiroiban> since you can call that method anyways
<danilos> adiroiban, well, you don't, it's just how most other classes in there do it
<danilos> adiroiban, you can use AuthorizationBase as the base class everywhere, and just construct whatever other classes you need
<adiroiban> I'm looking at class AdminDistributionTranslations(OnlyRosettaExpertsAndAdmins, EditDistributionByDistroOwnersOrAdmins):
<danilos> adiroiban, I'd say that's a much cleaner approach where you pass in only the relevant information to the auth class, but the current approach seems a de-facto standard
<adiroiban> and I wonder why it is not AdminDistributionTranslations(AuthorizationBase):
<danilos> adiroiban, it's just a pattern which works the same if you are not checking self.obj (i.e. context) in any of the permissions you are deriving from; it keeps the code shorter
<adiroiban> ok
<danilos> adiroiban, it's just a trick which involves knowledge of python OO model :)
<adeuring> noodles775: fancy a small ui/code review: https://code.edge.launchpad.net/~adeuring/launchpad/bug-172507/+merge/15925 ? (the real diff is two added/changed lines, the remaining 1000 lines are just an added SVG file ;)
<noodles775> adeuring: Sure! I'll do the ui review, but you'll need to grab someone else for the code (beuno's policy about not doing both code/ui for the one review)
* noodles775 changed the topic of #launchpad-reviews to: on-call: noodles775(UI) || reviewing: - || queue [sinzui, adeuring] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<adeuring> noodles775: yeah... but please have a look at the "code" first ;)
<noodles775> ah, ok, it's like that then :-)
<noodles775> No worries.
<noodles775> I see, literally two lines! It's mine I tell you!
<bigjools> al-maisan or noodles775, do you want to review my CP candidate
<al-maisan> bigjools: I can do it if noodles775 is busy
<al-maisan> bigjools: will do.
<noodles775> ta, I'll do adeuring's branch.
<bigjools> thanks guys
<bigjools> al-maisan: ah crap
<al-maisan> ?
<bigjools> I need to push another revision, I left a stale method, ignore the "assertNoBuildQueue" test
<al-maisan> bigjools: no problem -- just let me know when it's done.
<bigjools> diff will regenerate itself
<bigjools> al-maisan: https://code.edge.launchpad.net/~julian-edwards/launchpad/die-buildqueue-die--bug-492632/+merge/15927
<al-maisan> bigjools: thanks!
* al-maisan changed the topic of #launchpad-reviews to: on-call: noodles775(UI), al-maisan || reviewing: -, bigjools || queue [sinzui, adeuring] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<bigjools> grrrr the person chooser on the MPs gives you no chance to review what you clicked on
<bigjools> BjornT_: can you review the above MP for r-c please
* noodles775 changed the topic of #launchpad-reviews to: on-call: noodles775(UI), al-maisan || reviewing: adeuring, bigjools || queue [sinzui] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<al-maisan> bigjools: r=me
<bigjools> ta
<noodles775> Sorry adeuring - I should have just typed it here... I think you forgot to update the icon in the comment itself? (see the screenshot linked from my comment on the MP)
<adeuring> noodles775: right, just gor remonder from deryck :) I'm adding that now
<noodles775> Great.
* danilos changed the topic of #launchpad-reviews to: on-call: noodles775(UI), al-maisan || reviewing: adeuring, bigjools || queue [sinzui, danilos] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<danilos> al-maisan, another branch in the queue :) noodles775, or you if you fancy a short OOPS fix :)
<danilos> https://code.edge.launchpad.net/~danilo/launchpad/bug-480652/+merge/15931
<noodles775> danilos: yeah, I'll take it (I think al-maisan is on lunch)
<danilos> noodles775, cool, thanks
* noodles775 changed the topic of #launchpad-reviews to: on-call: noodles775(UI), al-maisan || reviewing: danilos, - || queue [sinzui] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<adiroiban> danilos: how should we display an inactive-template, what css atributes should be changed ? 
<noodles775> beuno: have you got time to take a peek at a screenshot? https://bugs.edge.launchpad.net/soyuz/+bug/211008/comments/1
<mup> Bug #211008: Add visual indicator for obsolete (superseded) packages in PPA <oem-services> <ppa> <ui> <Soyuz:In Progress by michael.nelson> <https://launchpad.net/bugs/211008>
<danilos> adiroiban, I am not exactly sure, perhaps we can ask noodles775 for an idea? :)
<noodles775> is there a bug? or on which page?
<adiroiban> https://translations.launchpad.dev/ubuntu/hoary/+templates
<danilos> noodles775, we've got a big list of templates (in a table) where some of them are inactive (we are showing them for admins only); what'd be your suggestion to do that on eg. https://translations.launchpad.dev/ubuntu/karmic/+templates page
<danilos> s/karmic/hoary/ of course, thanks adiroiban :)
<bigjools> noodles775: should it go in the +packages page instead?
<danilos> noodles775, also https://translations.edge.launchpad.net/openobject-addons/trunk/+templates
<noodles775> bigjools: I was going to put it on both.
<noodles775> What do you think?
<bigjools> ah fair enough
<adiroiban> danilos: maybe I can leave the css for the cleanout task
<bigjools> yes that sounds good - I'd be tempted to write the version that supersedes it as well
<noodles775> aha
<adiroiban> cleanup
<danilos> adiroiban, at least 'color: grey' should do for now, or a light background
<adiroiban> color: grey ... I think that any other background will just hilight the row 
<noodles775> adiroiban: How can I set one of the templates on that page to inactive? there's no option... or is it the 'Accept translations' admin option, or something in your branch?
<adiroiban> noodles775: the first one disabled-template is not active
<adiroiban> yes
<adiroiban> is "Accept translations" 
<adiroiban> but right now there is no style or class for it
<noodles775> adiroiban: Right, but is the behavior affected at all in the UI (ie. I can still edit/upload/download etc. it seems?)
<adiroiban> yes
<adiroiban> the idea is to give a hint about the state of a template
<adiroiban> just a small one
<adiroiban> only admins will see those templates that are not active
<noodles775> ok, and those templates are only visible to admins.
<adiroiban> yep
<noodles775> Yep, I personally wouldn't go for color: gray - as that indicates to me that it's disabled (ie. I can't interact with it), but everything in the ui tells me I still can. Hrm.
<danilos> noodles775, you can interact with it only as an admin... perhaps we can use an icon or something, but I'd rather just use a CSS style
<danilos> noodles775, at the moment, set of users who deal with these (i.e. who get to see disabled templates) is very small (LP translations team + Ubuntu Translations coordination team, total ~10 people)
<noodles775> danilos: yeah, so the person who sees that it is grayed out can interact with it. I was wondering whether it might be simpler to say Template name: disabled-template (inactive)
<danilos> noodles775, that would be harder to notice in a big list IMHO; how about a different TR background?
<danilos> noodles775, not that that wouldn't work, I just want to make it more visible
<danilos> well, I want adiroiban to make it more visible :))
<noodles775> danilos: yeah, in a big list something like that would be good.
<noodles775> So
<adiroiban> :D
<noodles775> if the background color of the row is shaded slightly and, so that people don't think, 'huh, why is it shaded', we indicate the inactive state in the name as above?
<danilos> noodles775, +1
<danilos> adiroiban, how do you like that? :)
<adiroiban> it's ok
<adiroiban> i'll do it :)
<danilos> adiroiban, if you have any better/different suggestions, feel free to tell them :)
<noodles775> adiroiban: feel free to do something that you like more...
<danilos> noodles775, adiroiban: cool, thanks!
<noodles775> +`
<noodles775> +1
<adiroiban> uh... color ?
<adiroiban> colorcode
<adiroiban> #DCDCDC
 * noodles775 looks in style-3.0.css
<noodles775> #747474 is what's being used for a disabled tab, but it might be too dark. See what you think.
<adiroiban> nope. to dark
<adiroiban> that was my starting point 
<adiroiban> let me try some colors and show you some screenshots
<danilos> adiroiban, don't worry about it too much, if it works for you, it'll work for the remaining 9 people who are going to see it :)
<adiroiban> ok
<adiroiban> danilos: should I rename the other xx-rosetta-pofile stories to xx-pofile?
<danilos> adiroiban, sure
<danilos> adiroiban, again, some of it may break tests (i.e. some tests are loaded through python files), so you don't have to, but if you want to fight more problems... :)
<adiroiban> danilos: or they can create name conflicts?
<danilos> adiroiban, yeah, that too
<adiroiban> with stories from other products?
<danilos> adiroiban, so, if you want to get your branch in, I wouldn't worry about fixing all the worlds problems in it :)
<adiroiban> :)
<beuno> noodles775, still need that peak?
<noodles775> beuno: yeah, if you wouldn't mind, then I can finish it and put an MP together :-) Thanks!
<beuno> noodles775, where does "newer version" link?
<noodles775> beuno: https://launchpad.dev/ubuntu/warty/+source/iceweasel/1.1
<beuno> noodles775, tha tonly thing that worries me a bit here
<beuno> is that if a PPA is there for say, backports, you'll have that on every single row
<BjornT_> bigjools: is there any test for code that calls BuildQueue.destroySelf()?
<noodles775> beuno: Erm, I don't think so, one tick...
<bigjools> BjornT_: I dunno without looking
<danilos> noodles775, I am out for lunch: my branch should be pretty simple, but if you've got any questions, feel free to tackle somebody else's branch so we can deal with mine when I am back :) thanks
<noodles775> beuno: actually, yes, I think you're right. As long as it's in one of the distributions archives.
<bigjools> BjornT_: the best we have is lib/lp/soyuz/doc/buildd-slavescanner.txt
<noodles775> danilos: yes, it looks straight forward, sorry, I've just had other things coming up.
<danilos> noodles775, don't worry, it's not a huge rush, when you get to it, great :)
<beuno> noodles775, in light of that, what do you think about making it less prominent?
<noodles775> beuno: sure (I just checked, anything published in primary, partner or debug is included).
<beuno> noodles775, it seems more informational than a warning in this case
<BjornT_> bigjools: why does it call destroySelf() in the test itself? what code actually calls destroySelf() in production?
<noodles775> beuno: yep, I'll switch the icon, but other than that, I'm not sure how to make it less prominent while still conveying enough info (and a link) to identify what it is?
<BjornT_> bigjools: btw, on line 514 you try to call destroySelf(), but the isn't prefixed with >>>
 * beuno thinks
<bigjools> BjornT_: heh, there's a few without the >>>
<bigjools> that's an old old test
<bigjools> BjornT_: see the bottom few methods of lib/lp/buildmaster/buildergroup.py
<bigjools> that's the code that handles builds returned from the buildds
<beuno> noodles775, maybe less verbose?  "(i) _Newer version_ available"
<noodles775> beuno: OK - I'd thought it necessary to somehow indicate that it was a newer version in the distroseries itself. But you're right. I'll go with that. Thanks!
<BjornT_> bigjools: ok. i assume that you have, or will test this patch on dogfood?
<bigjools> BjornT_: I have yes - it's the right thing to do anyway, it was omitted in the recent refactoring
<beuno> noodles775, something better may come to mind once the caffeine starts flowing to my brain  :)
<salgado> BjornT_, care to review a trivial lazr-js branch for me? https://code.edge.launchpad.net/~salgado/lazr-js/bug-482235/+merge/15940
<BjornT_> salgado: sure
<salgado> BjornT_, in fact, I'm more interested in feedback about the feature itself, as I'll ask mars for another review later, when he's around
<BjornT_> salgado: ok
<BjornT_> salgado: can you give an example in LP code, where you would use this? i.e., where we use the Picker, and where you plan to specify this new argument
<salgado> BjornT_, the vocabulary picker
<salgado> in Launchpad
<salgado> I enter a person's name but am not sure whether or not it's correct, so I click 'Choose...' to search for it
<salgado> the overlay pops up and I have to type the name again
<BjornT_> salgado: can you point me to a file? i want to see how it's connected to the text field
<salgado> BjornT_, canonical/widgets/templates/inline-picker.pt
<BjornT_> salgado: not vocabulary-picker.js?
<salgado> BjornT_, could be; that's not quite clear to me
<BjornT_> salgado: yeah. i'll add a comment to the MP. i have some thoughts on the API
<bigjools> BjornT_: interesting point about the attributes in my test
<bigjools> I'll change it to use another variable "just in case"
<bigjools> it'll be the fifth time I've re-written the test :)
<BjornT_> bigjools: yeah, it's better to be on the safe side
<bigjools> I agree
<salgado> BjornT_, did you forget about that comment to the MP?
<BjornT_> salgado: no, just got distracted :) it's there now
<adiroiban> danilos: the branch with changes to launchpad.TranslationsAdmin privileges is not ready. It looks like I need to also look at the db schema
<danilos> adiroiban, why's that?
<adiroiban> danilos: http://paste.ubuntu.com/338672/
<adiroiban> danilos: I assume that as an UTC i only got an launchpad.TranslationsAdmin "token"
<adiroiban> danilos: maybe I'm talking nonsenses
<danilos> adiroiban, I am trying to look it up, my connection to canonical servers is slow today, not sure why :(
<danilos> adiroiban, that's right, it just means that you missed a spot where you should have changed the permission to TranslationsAdmin instead of Admin, are all your latest changes in a branch I can take a look at?
<adiroiban> yes. in the MP one
<danilos> adiroiban, cool, let me get it here
<adiroiban> danilos: here is a "clean" error http://paste.ubuntu.com/338675/
<adiroiban> danilos: you get that when trying to save a template as UTC
<danilos> adiroiban, look at translations/configure.zcml, search for POTemplate, and look what launchpad.Admin is required for
<danilos> adiroiban, changing that to launchpad.TranslationsAdmin makes the test pass for me
<adiroiban> danilos: thanks. I was looking only at translations/browser/configure.zcml
<danilos> adiroiban, np
<danilos> adiroiban, btw, this change will introduce some more privileges to you guys, you'll get some privileges on language pack pages and such
 * danilos switches locations
* henninge changed the topic of #launchpad-reviews to: on-call: noodles775(UI), al-maisan || reviewing: danilos, - || queue [sinzui, henninge] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
* sinzui changed the topic of #launchpad-reviews to: on-call: noodles775(UI), al-maisan || reviewing: danilos, - || queue [henninge] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<henninge> hi al-maisan, noodles775:  I added my name to the queue for a branch that started out as a "quick fix" but got a bit longer because of tests. The original reviewer asked if somebody else could finish this.
<henninge> noodles775: are you doing UI exclusively?
<al-maisan> henninge: where is that branch?
<henninge> al-maisan: https://code.edge.launchpad.net/~henninge/launchpad/access-to-translationgroup-table/+merge/15643
 * al-maisan looks
<noodles775> henninge: if possible, only because I've already done lots of UI reviews this week, so keen to minimise any more time spent reviewing.
<al-maisan> henninge: I'll take care of it.
* al-maisan changed the topic of #launchpad-reviews to: on-call: noodles775(UI), al-maisan || reviewing: danilos, henninge || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<henninge> noodles775: ok, I was just curious, und auÃerdem ^
<henninge> al-maisan: Danke schÃ¶n!
<al-maisan> henninge: Bitte, nichts zu danken :)
<allenap> noodles775: Fancy doing a UI review of https://code.edge.launchpad.net/~allenap/launchpad/show-me-too-counts/+merge/15947? I haven't got screen-shots, but can get some if you'd like.
<noodles775> allenap: np, I'll want to run it anyway.
* noodles775 changed the topic of #launchpad-reviews to: on-call: noodles775(UI), al-maisan || reviewing: allenap, henninge || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<allenap> noodles775: Cool, thanks :)
<noodles775> allenap: np. I'll take a look in 15m or so.
<al-maisan> henninge: quick question: should the new/revised tests in test_translationimportqueue.py fail if the security.cfg changes are not applied to the database?
<henninge> al-maisan: yes, some of them.
<henninge> three, I think
<henninge> one for each of the three tables
<allenap> al-maisan: Are you up for a code and/or js review of https://code.edge.launchpad.net/~allenap/launchpad/show-me-too-counts/+merge/15947?
<henninge> allenap: do we still split js and code?
<allenap> henninge: I don't know... I got a merge proposal yesterday with separate code and js reviews.
<allenap> henninge: Most if not all of us are js reviewers, so I guess it makes little sense.
<al-maisan> allenap: I could do the code review once I am done with henninge's branch .. how does that sound?
<al-maisan> allenap: if needs be I can look at the js piece as well.
<allenap> al-maisan: That's great, thanks.
<henninge> allenap: I never formally graduated as js reviewer but I think to be remembering that at one point it was said that all reviewers can do js reviews now.
<adeuring> noodles775: http://people.canonical.com/~adeuring/patch-icons1.png
<noodles775> Great, thanks adeuring !
<allenap> henninge: Okay, perhaps something to bring up on the list or at the next reviewer meeting?
<henninge> allenap: yup, next reviewers meeting on the 6th.
<adeuring> noodles775: just noticed that it is trivial to enable the "patch" checkbox in the "add attachment" form. Let me add that too
<noodles775> adeuring: that'd be great!
<henninge> allenap: https://dev.launchpad.net/ReviewerMeetingAgenda
<allenap> henninge: Fantastic :)
<adiroiban> danilos: hi. I pushed the latest change, and after running bin/test -vvc -m translations --layer=PageTestLayer , everhing seems fine.
<danilos> adiroiban, cool, sounds good, I'll take a look at it asap
<adiroiban> danilos: there's no hurry :)
<al-maisan> henninge: is there no test needed that the 'queued' user may access a translationgroup?
<henninge> al-maisan: that test is included in the ubuntu tests.
<adeuring> noodles775: r10020 of the branch  contains the improved link. Now ready for a re-review :)
<al-maisan> henninge:  ubuntu tests?
<noodles775> adeuring: great, thanks!
<henninge> al-maisan: if you look at the full file, you will see that there is a special test when the target is a sourcepackage in ubuntu
<al-maisan> henninge: ah, I see.
<al-maisan> henninge: r=me
<henninge> al-maisan: that is when translation group is accessed
<henninge> al-maisan: Danke!
* al-maisan changed the topic of #launchpad-reviews to: on-call: noodles775(UI), al-maisan || reviewing: allenap, - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<al-maisan> allenap: sorry, need to finish something else first. Hopefully back to review your stuff soon.
<allenap> al-maisan: It's okay, I'm not in a rush. Thanks for letting me know.
<al-maisan> :)
<noodles775> allenap: wow, that feature is great!
<noodles775> allenap: I was wondering whether there was a decision behind *not* displaying the count for anon users?
<allenap> noodles775: No deliberate decision, just didn't actually think about it, doh.
<allenap> noodles775: I should probably add it!
<noodles775> allenap: that'd be great!
<allenap> noodles775: I'll make it simply say "This bug affects 27 people".
<noodles775> Perfect
<bac> hi jpds
<jpds> Hey bac.
<bac> jpds: since you are becoming so prolific (!) you may want to have a look at https://edge.launchpad.net/lpreview-body .  with it you can do 'bzr send' and it'll help you write a good merge proposal cover letter and get it submitted.  a good side effect is it assigns the whole launchpad team as a reviewer, which may give you much better turn-around.
<jpds> bac: Aha, that's neat, thanks for the tip. :)
<bac> jpds: oh, and it runs 'make lint' for you too.  that saves me all the time from doing something silly.
<jpds> bac: Installed, will use it next time.
<bac> jpds: great.  hope it works well for you.
<jpds> bac: Can you feed my pending branch to PQM? :)
<bac> jpds: i can later.  you may want to see if noodles775 or al-maisan can do it, since they are on-call reviewers today.  what is the URL for the MP?
<jpds> https://code.edge.launchpad.net/~jpds/launchpad/fix_50580/+merge/15892
 * al-maisan is fixing the devel breakage, sorry
<adeuring> al-maisan: ar you still reviewing? If you, could you have a look at https://code.edge.launchpad.net/~adeuring/launchpad/bug-172507/+merge/15925 ? (the real diff isn't 1100 line, but ca 180)
<noodles775> rockstar: are you around soon? al-maisan and myself are finishing up, but there are a few requests coming in.
<rockstar> noodles775, yea, right after this QA meeting.
<al-maisan> adeuring: I am preparing a testfix for devel, later maybe..
<noodles775> Great! Thanks rocksar.
<adeuring> al-maisan: let me ask rockstar
* noodles775 changed the topic of #launchpad-reviews to: on-call: - || reviewing: - || queue [jpds, adeuring] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<noodles775> adeuring, jpds: I've put you both in the queue, so rockstar will get to them as soon as he's finished the meeting.
<adeuring> noodles775: ok, thanks!
<jpds> noodles775: Thanks.
<allenap> noodles775: Just thinking. Do you want the link to +affectsmetoo to appear to anon users? So they can click, login, answer?
<allenap> noodles775: I've just finished the change to just show a statement without a link!
<adiroiban> danilos: even though maybe now we have more privileges, they will not show up in the UI since all TAL conditions are for launchpad.Admin
<allenap> noodles775: The context menu only enables the link for logged-in users, so perhaps I'll just follow that.
<danilos> adiroiban, heh, but code is right there for people to see :) btw, relevant conditions should be changed as well, no reason to hide them if you ask me
<adiroiban> danilos: yep. I will try to solve in in the cleanup branch... as I will need to look at each launchpad.Admin and check if it can be replaced by launchpad.TranslationsAdmin
<danilos> adiroiban, it can't be done everywhere, unless you want to get into more of these cleanups :) your effort can be better spent elsewhere than cleaning up the entire tree, not that I'd stop you :)
<adiroiban> danilos: hm... then I should fix those problems in this branch ?
<danilos> adiroiban, well, no... the point is that launchpad.Admin and launchpad.TranslationsAdmin are not interchangeable permissions
<danilos> adiroiban, if there are problems like these (I suspect there are only one or two), we'll fix them as we find them
<adiroiban> danilos: ah. ok :)
* rockstar changed the topic of #launchpad-reviews to: on-call: rockstar || reviewing: jpds || queue [adeuring] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<rockstar> jpds, hi.
<jpds> rockstar: Hello.
<rockstar> jpds, have you contributed to Launchpad before?
<jpds> rockstar: Two branches, both in devel/
<rockstar> jpds, okay, so you have signed the contributor agreement then.
<jpds> rockstar: I work for Canonical.
<rockstar> jpds, oh.
 * rockstar hangs head in shame
 * jpds hugs rockstar.
<rockstar> jpds, ah, you started last month?
<jpds> rockstar: Yes.
<rockstar> jpds, well welcome aboard!
<jpds> Thank you. :)
<rockstar> Now I don't feel so bad.  We hire people faster than I can meet people.  :)
<rockstar> jpds, have you thought about a way to test this?
<jpds> rockstar: I ran: "make run" and viewed the mirrors page at launchpad.dev/ubuntu/+archivemirrors .
<rockstar> jpds, well, I mean using automated testing.
<jpds> Hmm, not really I'm afraid.
<jpds> It's really just colouring in text that's there in black. :)
<rockstar> jpds, I'm not sure if it really requires it, but I think it'd be a good benefit.
<rockstar> bigjools, ping
<bigjools> rockstar: yarp
<rockstar> bigjools, https://code.edge.launchpad.net/~jpds/launchpad/fix_50580/+merge/15892
<rockstar> bigjools, do you think that this patch needs tests?
<bigjools> depends how anal you wanna be
<bigjools> the question I ask, is it worth testing
<bigjools> it probably is worth a quick test I think, it's pretty easy
<rockstar> bigjools, I don't think it requires it, but if it's easy, then why not.
<bigjools> yeah, just getTagsByClass and profit
<rockstar> jpds, is this something you've done before?
<jpds> rockstar: No.
<bigjools> rockstar: why don't you write the test for him?
<bigjools> we should not block contributions on lack of tests
<rockstar> bigjools, yes, this is true.
<bigjools> jpds can see the test afterwards and learn :)
<jpds> Yeah, I've never written a test case before...
<bigjools> jpds: the tests for the web pages are in lib/lp/<project>/stories
<bigjools> they use the zope test browser
<allenap> al-maisan: If you're still busy, I can see if someone else can have a look at my branch?
<al-maisan> allenap: that would be good, I am about to finish the test fix and planned to end the day.
<rockstar> bigjools, is there an existing doctest that this might go with?  I know very little about your crazy soyuz code.
<bigjools> this is not soyuz code :)
<bigjools> it's registry
<bigjools> even before we generously donated half our code to them
<bigjools> but I can look
<bigjools> rockstar: ib/lp/registry/stories/distributionmirror/xx-distribution-mirrors.txt ?
<rockstar> bigjools, thanks.
<rockstar> It's distro mirror junk, so I figured you'd know.
<noodles775> rockstar: I'm putting one in the queue for a UI review - if you have time later in your day (not urgent).
<noodles775> https://code.edge.launchpad.net/~michael.nelson/launchpad/211008-visual-indicator-superseded-pkgs/+merge/15963
<rockstar> noodles775, great, thanks.
* noodles775 changed the topic of #launchpad-reviews to: on-call: rockstar || reviewing: jpds || queue [adeuring, noodles775] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<noodles775> Thank you!
<jpds> rockstar: Any luck?
<rockstar> jpds, I'm actually on the phone chasing something else right now.
<jpds> Ah, OK.
<rockstar> jpds, so, the code is good, but I need to write tests, and I wasn't entirely planning to write code much today (being on-call reviewer) so I need to figure out how to work out where to fit that into my day.
<jpds> rockstar: OK, right.
* rockstar changed the topic of #launchpad-reviews to: rockstar || reviewing: adeuring || queue [noodles775] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<rockstar> adeuring, I can't find your branch on +activereviews
<adeuring> rockstar: https://code.edge.launchpad.net/~adeuring/launchpad/bug-172507/+merge/15925
<rockstar> adeuring, danke
<adeuring> rockstar: thanks to _yout_, for reviewing this stuff :)
<rockstar> adeuring, holy crap, that's a big SVG...  :)
<adeuring> rockstar: well, xml is chatty...
<rockstar> adeuring, I'm sure the Ubuntu Bugs team is going to love this.  They were talking about it a lot of UDS.
<rockstar> adeuring, I'm also sure that they're going to want this through the API, so eventually you may need to move that to the model/interface
<adeuring> rockstar: maybe... But with the API you can simpy filter by the property type
<adeuring> so there is no real need, I thnk
<adeuring> ...i mean: filter by bugattamnet.type ...
<adeuring> argh... bugattachment.type....
<rockstar> adeuring, okay.
 * rockstar lunches
<bac> rockstar: i'm adding a branch to your queue for your post-lunch enjoyment.
* bac changed the topic of #launchpad-reviews to: rockstar || reviewing: adeuring || queue [noodles775, bac] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<abentley> rockstar: I can has reviews?
<rockstar> abentley, please add yourself to the queue.
* abentley changed the topic of #launchpad-reviews to: rockstar || reviewing: adeuring || queue [noodles775, bac, abentley, abentley, abentley, abentley] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<abentley> rockstar: https://code.edge.launchpad.net/~abentley/launchpad/twistedjob-enhancements/+merge/15977 https://code.edge.launchpad.net/~abentley/launchpad/twistedjob/+merge/15980 https://code.edge.launchpad.net/~abentley/launchpad/ampoulejob/+merge/15982 https://code.edge.launchpad.net/~abentley/launchpad/ampoulejob-timeout/+merge/15986
<rockstar> abentley, okay, I think you just filled my queue for the rest of the day.
<abentley> rockstar: My pleasure.
<rockstar> abentley, and I also can't guarantee I'll get to all of them, but I'll do my best.
<abentley> rockstar: cool.
<jpds> rockstar: getTagsByClass> Does this even exist?
<jpds> rockstar: Can't find any reference of it in the code anywhere...
<rockstar> jpds, we may not be using it, but it may be part of the zope test browser.
<rockstar> ...or BeautifulSoup
<jpds> Hmm, Google doesn't seem to know about it.
<rockstar> jpds, huh.  Maybe bigjools is on crack.
<jpds> rockstar: Well, soyuz... yeah.
 * jpds runs.
<bigjools> oO
<bigjools> find_tags_by_class
<bigjools> my bad
<jpds> bigjools: Ah cool, thanks.
<wgrant> Ah, it's LP-specific.
<jpds> rockstar: Tests added: 
<jpds> https://code.edge.launchpad.net/~jpds/launchpad/fix_50580/+merge/15892
<rockstar> jpds, oh great.  That makes my life easier.
<jpds> rockstar: That's always a bonus.
#launchpad-reviews 2009-12-11
<thumper> jml: https://code.launchpad.net/~thumper/launchpad/reassign-review-into-model/+merge/15991 if you can
<thumper> jml: it would be good to get this landed today
<jml> thumper, sure. I'll churn through a few action-demanding emails first.
<thumper> jml: ok, ta
* rockstar changed the topic of #launchpad-reviews to: no one || reviewing: || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<thumper> jml: can I get you to do a very boring code review?
<thumper> jml: it just moves stuff around in a template (mostly)
<jml> thumper, yeah, sure.
<thumper> https://code.edge.launchpad.net/~thumper/launchpad/comment-ui-tweaks/+merge/15920
<thumper> I'm just looking into noodles's comment about enabling
<thumper> I'm thinking he missed something
<thumper> jml: if you want a picture I'll put one up for you to look at
<thumper> jml: it is a big improvement
<jml> thumper, that'd help, actually.
<thumper> http://penhey.net/~tim/add-comment.png
<thumper> jml: just afk for a few minutes
<thumper> jml: back
<jml> thumper, you haven't made any comments on that review recently
<jml> thumper, max-width in #add-comment-form should have a space after the colon
<thumper> ok
<jml> I don't really like the text "Optional review"
<thumper> jml: it was what our ui reviewer noodles suggested, and I think it is ok
<thumper> jml: given the improvements this has in polish, I'd suggest that we land it and bikeshed about the text later
<jml> thumper, what improvements does this have in polish?
<jml> just moving the fields down below the text area?
<thumper> jml: have you looked at the page recently?
<thumper> look at the two in comparison and tell me it doesn't look more polished
<jml> thumper, it looks better, sure.
<jml> thumper, you don't mention the JS changes in the cover letter -- what are they for?
<thumper> The comment block only used to be there when JS was enabled
<thumper> I changed it to work without JS too
<thumper> but it only shows if you are logged in
<thumper> in the same way the bugs comment does
<jml> ahh cool.
<thumper> in fact the comment about being logged in came from the bugs page
<thumper> consistency FTW
<jml> great, the diff has changed since I started reviewing :(
<thumper> jml: only two spaces added
<jml> thumper, I've done a review.
<thumper> ta
<thumper> jml: actually the suggested text was "Optional review info:", do you think that is any better?
<jml> thumper, I think it's more meaningful, yes. I think "info" is a bit too informal, but I'd be willing to let it pass.
<jml> thumper, Why do you think the "optional" bit helps?
<thumper> jml: can you think of something less formal?
<jml> more formal :)
<thumper> yeah, that
<thumper> it is Friday afternoon
<jml> thumper, "Optional review information" would be the obvious
<thumper> I thought it was too informal to
<thumper> that why I removed it
<jml> yeah, I know. Fridays suck.
<thumper> lemmie look at what that looks like
<thumper> it looks very verbose
<thumper> jml: in fact removing the Optional looks good to
<thumper> just having Review there
<thumper> so the default is:
<jml> thumper, I think saying "Comment only" implies the optional, myself.
<thumper> Review: Comment only
<thumper> jml: so remove the Optional?
<jml> thumper, yep.
<thumper> ok
<jml> thumper, I just replied to your other review
<jml> aiui, if I try to assign a thing to you for review, and you are already assigned to do it
<thumper> I'm not sure I'll get that finished tonight
<jml> the error is "Tim Penhey already has an existing pending review"
<jml> is that right?
<thumper> almost: "Tim Penhey (thumper) already has an existing pending review"
<jml> ahh ok.
<jml> how about "Tim Penhey (thumper) has already been asked to review this"
<thumper> sounds fine
<thumper> better even
<jml> and likewise, "Tim Penhey has already reviewed this"
<jml> rather than 'has a personal review'
<thumper> yeah, they sound good
<jml> cool.
<jml> makes it easier to understand what the code is for, also :)
<noodles775> Hi Tim, yeah, the disabling/enabling of the review type field was nothing to do with your branch (it's been like that for a while) - I just wasn't sure if it was a decision, or a bug.
<noodles775> thumper: ^^
<thumper> noodles775: design decision and it is working as designed :)
<thumper> noodles775: and now check out the revisions pushed interleaved with the comments \o/
<noodles775> thumper: wow!
 * noodles775 looks
<thumper> revisions hyperlinked and everything
 * thumper does a little dance
 * thumper leaves
<noodles775> Enjoy your w/e thumper!
<noodles775> Hi adeuring, could you take a quick look at bug 493703 for me? I'm wondering if it might be related to changes to the LFA/LFC last cycle.
<mup> Bug #493703: LocationError raised in build page and distribution arch series binary package page <oops> <Soyuz:In Progress by michael.nelson> <https://launchpad.net/bugs/493703>
 * noodles775 looks for the mp
<adeuring> npoosure
<adeuring> noodles775: sure
<noodles775> ta
<noodles775> I don't suppose there are any ui reviewers able to do a second ui review for https://code.edge.launchpad.net/~michael.nelson/launchpad/211008-visual-indicator-superseded-pkgs/+merge/15963
<noodles775> adeuring: I just added a comment on the bug with steps to reproduce locally.
<noodles775> (but still trying to find out why this might happen)
* adeuring changed the topic of #launchpad-reviews to:  on call: adeuring || reviewing: || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<gmb> adeuring: Could you take a look at https://code.edge.launchpad.net/~gmb/launchpad/taste-the-blood-of-ajax-dupefinding/+merge/16006 for me?
<adeuring> gmb: sure
<gmb> Ta
* jelmer changed the topic of #launchpad-reviews to: on call: adeuring || reviewing: || queue [jelmer] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<adiroiban> adeuring: hi. can you please take a look at https://code.edge.launchpad.net/~adiroiban/launchpad/bug-427319/+merge/15994 ? Thanks!
<adiroiban> just add in your queue... there is no hurry
<adeuring> adiroiban: sure, one I've finished gmb's and jelmer' MP. Just add yourself to the queue :)
* adiroiban changed the topic of #launchpad-reviews to: on call: adeuring || reviewing: || queue [jelmer,adiroiban] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<adiroiban> ok
<adeuring> gmb: r=me
<gmb> adeuring: Awesome, thanks!
<jpds> adeuring: Can you land http://tinyurl.com/ydl256z for me?
<adeuring> jpds: that MP needs a review first ;) But I'll add it to my queue.
* adeuring changed the topic of #launchpad-reviews to: on call: adeuring || reviewing: || queue [jelmer,adiroiban, jpds] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<danilos> adiroiban, hi, only a few small bits remaining for your permissions branch, if you've got them ready in the next few hours, I can probably land it for you later :)
<jpds> adeuring: http://pastebin.ubuntu.com/339136/plain/ . :)
<adiroiban> danilos: hi. yep. working on that... puzzled by the python-mode indentation :D
<adiroiban> danilos: ok. got it. sorry
<danilos> adiroiban, btw, it seems our automatic script to detect commits is not working properly, can you please update the bugs you know are fix committed to point to 3.1.12 release and tag them with qa-needstesting (or qa-ok if they are already on edge and you confirmed it's all fine :)
<danilos> adiroiban, yeah, python-mode is not perfect, sometimes it just fucks it up, if you pardon my Romanian :P
<adiroiban> danilos: ok.
<adiroiban> i'll update them, no problem
<danilos> cool, ta
* adeuring changed the topic of #launchpad-reviews to: on call: adeuring || reviewing: jelmer || queue [adiroiban, jpds] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
* salgado changed the topic of #launchpad-reviews to: on call: adeuring,salgado || reviewing: jelmer || queue [adiroiban, jpds] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<salgado> adiroiban, yours is https://code.edge.launchpad.net/~adiroiban/launchpad/bug-427319/+merge/15994 ?
<adiroiban> salgado: yes.
<adiroiban> danilos: the branch is almost ready. I have added the requried test for checking template admin task for product and distribution, including disabled templates... running the full test to check that everhing is ok
<danilos> adiroiban, cool, if those tests pass, I'll be running the full test suite before landing anyway, so you don't have to
<salgado> adiroiban, are you pushing more changes to this branch or are you talking about a different branch with danilos?
<danilos> salgado, it's a different branch
<salgado> ok, cool
<gmb> on call: adeuring,salgado || reviewing: jelmer || queue [adiroiban, jpds, gmb] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<gmb> adeuring, salgado: Another one for you, when you've time https://code.launchpad.net/~gmb/launchpad/scars-of-ajax-dupefinding/+merge/16009
<salgado> gmb, sure, but only if you learn how to change the topic. ;)
<gmb> salgado: AAAAARGH.
* gmb changed the topic of #launchpad-reviews to: on call: adeuring,salgado || reviewing: jelmer || queue [adiroiban, jpds, gmb] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<salgado> there you go. :)
<gmb> salgado: Nearly 15 years I've been using IRC. Can I get it right? Can I hell.
<adiroiban> danilos: done. changed pushed. diff added as a comment
* salgado changed the topic of #launchpad-reviews to: on call: adeuring,salgado || reviewing: jelmer,adiroiban || queue [jpds, gmb] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
* adeuring changed the topic of #launchpad-reviews to: on call: adeuring,salgado || reviewing: jpds, adiroiban || queue [gmb] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<adeuring> noodles775, beuno: could you have a quick ui-look at this MP: https://code.edge.launchpad.net/~jpds/launchpad/fix_50580/+merge/15892 ?
* adeuring changed the topic of #launchpad-reviews to: on call: adeuring,salgado || reviewing: gmb, adiroiban || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<noodles775> adeuring, jpds: done, thanks!
<adeuring> noodles775: thanks! jpds: I'll run your branch through the EC2 tests
<jpds> noodles775: Yeah, I have nothing to do with the existing test mirror data...
<jpds> adeuring, noodles775: Thanks for the reviews. :)
<noodles775> jpds: np. Yes, I didn't mean updating the test/sample data, just either some steps to do, or a small python script that the reviewer can run to add some interesting data to see the results.
<jpds> noodles775: I'll look into it.
<noodles775> Just for next time... or a screenshot can sometimes be just as useful.
<adeuring> gmb: r=me
* adeuring changed the topic of #launchpad-reviews to:  on call: adeuring,salgado || reviewing: -, adiroiban || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<gmb> adeuring: Fantastisch! Danke!
<adeuring> gmb: welcome :)
<jelmer> adeuring, thanks for the reviews :-)
<adeuring> jelmer: welcome :)
* noodles775 changed the topic of #launchpad-reviews to:  on call: adeuring,salgado || reviewing: -, adiroiban || queue [noodles775] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<noodles775> adeuring: could you pls fit this one in sometime? https://code.edge.launchpad.net/~michael.nelson/launchpad/493703-location-error/+merge/16015
<adeuring> noodles775: sure
<noodles775> Ta!
* adeuring changed the topic of #launchpad-reviews to: on call: adeuring,salgado || reviewing: noodles775, adiroiban || queue [noodles775] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<adeuring> noodles775: r=me
* salgado changed the topic of #launchpad-reviews to: on call: adeuring,salgado || reviewing: noodles775 || queue [noodles775] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<mars> salgado, ping, have time to review a one-line patch?  http://pastebin.ubuntu.com/339212/
<salgado> sure
<salgado> mars, r=me
<mars> thanks salgado 
<adiroiban> salgado: thanks for the review. I'll rework it.
<salgado> adiroiban, thank you for the nice branch!
<adiroiban> salgado: for the baseclass, can I use the abstract keyword, to prevent direct instantiation ?
<salgado> adiroiban, in general we prefix the class name with Base, but I don't have a problem with abstract
<adiroiban> salgado: ah. OK. I will go for Base then
<salgado> adeuring, the queue's currently empty or is there a branch from noodles775 that has to be reviewed?
<adeuring> salgado: my fault, i reviewed it and forgot to removed it from the topic...
* adeuring changed the topic of #launchpad-reviews to: on call: adeuring,salgado || reviewing: -, - || queue [noodles] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<salgado> adeuring, it's still in the queue. ;)
<adeuring> argh... 
* adeuring changed the topic of #launchpad-reviews to:  on call: adeuring,salgado || reviewing: -, - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<adiroiban> salgado: is there an equivalent call in python for getting this string from TAL: view/translation_team/translator/fmt:link ?
<salgado> oh, yeah, I forgot to mention that
<salgado> just a sec
<adiroiban> just a hint
<salgado> these are formatters and they come from lib/canonical/launchpad/webapp/tales.py
<adiroiban> and I will look into the code. 
<adiroiban> ok. so I should just call the formater
<salgado> PersonFormatterAPI(naked_team).link(None)
<salgado> yep
<adiroiban> ok. I was not sure if this is the right way to do it... I was trying to keep those things in the template
<adiroiban> and have only logic in the view
<salgado> in this case we're just using the formatter, so we don't need to have html code in the middle of our python code.
<salgado> we try hard not to have html together with python code, but in some cases it's the least bad option
<adiroiban> ok. thanks
<deryck> adeuring or salgado -- I've got an easy Windmill fix branch.
* deryck changed the topic of #launchpad-reviews to:  on call: adeuring,salgado || reviewing: -, - || queue [deryck] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<adeuring> deryck: I'll look at it
<deryck> adeuring, thanks!
<deryck> adeuring, https://code.launchpad.net/~deryck/launchpad/windmill-also-affects-link-failure-494018/+merge/16019
<adeuring> deryck: r=me
<deryck> adeuring, excellent, thanks.  I have another easy one if you like. :)
<adeuring> deryck: sure
<deryck> adeuring, waiting on the mp to appear
<deryck> adeuring, https://code.edge.launchpad.net/~deryck/launchpad/remove-double-heading-tags-page/+merge/16020
<bac> g'morning sinzui
<sinzui> hi bac
<bac> sinzui: looking at the data from the query you ran yesterday made me realized i'd missed a case wrt bzip2
<adeuring> deryck: r=me, again
<bac> sinzui: could i get a quick review of http://pastebin.ubuntu.com/339242/
<deryck> adeuring, thanks
* deryck changed the topic of #launchpad-reviews to:  on call: adeuring,salgado || reviewing: -, - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<sinzui> bac: r=me
<bac> sinzui: thanks.  /me sends to pqm and back to bed...
<gary_poster> BjornT_: hey.  So, my only concerns are these. (1) should we be more careful to only do what we expect to be doing?  IOW, I'd feel better if we had
<gary_poster> if not safe_hasattr(sys.stdin, 'fileno'):
<gary_poster>     assert isinstance(sys.stdin, zope.testing.testrunner.runner.FakeInputContinueGenerator), 'Unexpected value for sys.stdin'
<gary_poster> (2) should we try getting this upstream?  It seems reasonable
<gary_poster> BjornT_: otherwise +1
<gary_poster> BjornT_: I have someone waiting on a call.  I'll mark this Approved: merge-conditional with those comments so you can proceed.  If you disagree with my points, ping me and I'll reply asap
<BjornT_> gary_poster: i agree with your points. 2) was the reason i asked you for a review, since i thought you might now a thing or two about zope.testing :)
<BjornT_> i'm adding the assert, it's good to have
<gary_poster> BjornT_: cool :-)
<gary_poster> approved in the MP
<BjornT_> thanks!
* noodles775 changed the topic of #launchpad-reviews to:  on call: adeuring,salgado || reviewing: -, - || queue [noodles775] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<noodles775> adeuring (or salgado): Same bug, different call-site (when you've time): 
<noodles775> https://code.edge.launchpad.net/~michael.nelson/launchpad/493703-bpr-error/+merge/16026
<adeuring> noodles775: sure, I'll look
<noodles775> Thanks again.
<noodles775> adeuring: in retrospect, I think the approach here would have been better for the build page too, but I won't get to change it before finishing today.
<adeuring> noodles775: well, I think your previous branch was strictly bad ;)
<adeuring> so, no problem if it stays for one cycle, I think
<noodles775> huh?
<adeuring> noodles775: i mean: no problem with your branch from my side 
<adeuring> ...with the provious branch...
<noodles775> ah, I was wondering why you'd approved it if you thought it was strictly bad ;)
<adeuring> noodles775: argh... now i see... i omitted a "not"... sorry...
<noodles775> :)
<allenap> adeuring: Up for a quick review? https://code.edge.launchpad.net/~allenap/launchpad/is-user-affected-performance/+merge/16027
<adeuring> allenap: sure
<adeuring> noodles775: r=me
<noodles775> Thanks adeuring 
<adeuring> allenap: nice change! r=me
<allenap> adeuring: Thanks :)
<allenap> adeuring: I noticed another performance thing that I'd like to land with the patch you just reviewed. Mind reviewing it too? http://pastebin.ubuntu.com/339296/
<adeuring> allenap: sure
<adiroiban> salgado: there is one thing, where I don't know what I should change http://paste.ubuntu.com/339303/
<salgado> adeuring, just change the narrative to make it clear you're creating that stuff just so that you can test what the page looks like when X and Z exist
<adeuring> salgado: I believe you menat adiroiban ;)
<salgado> yeah, I did
<salgado> adiroiban, ^
<adeuring> allenap: very nice! r=me
<allenap> adeuring: Thanks again :)
<adiroiban> salgado: like adding : This is requried for setting up the testing environment.
<salgado> yeah, or just "Create X and Z so that we can see what the page will look like when they exist."
<adiroiban> I'm quite a bad narrator
<adiroiban> thanks
<salgado> adiroiban, the narrative in your tests is quite good, actually; just this bit that needed some tweaks
<adiroiban> salgado: ok. I have pushed the changes and added a diff with the latest changes
<allenap> adeuring: Interesting. Looks like there's a new feature in merge proposals. It shows the change I added after your review in https://code.edge.launchpad.net/~allenap/launchpad/is-user-affected-performance/+merge/16027
<adeuring> allenap: right! I saw this berfore today but did not look that closely
<salgado> adiroiban, cool, I'll get to it in a minute
<adiroiban> salgado: ok. no hurry. I just wanted to know if I followed the right process
<salgado> adiroiban, ideally you should reply to the review, commenting on each of the points brought up by the reviewer; be it to agree, disagree, ask questions, propose a different alternative, etc
<salgado> that's a good idea as it works like a check list of the things that need to be done
<adiroiban> aha... so the review chat should be done by mail...as much as possible ?
<adiroiban> and I don't need to ping the review to look at the changes
<adiroiban> or land the branch afther it was approved
<salgado> adiroiban, yes, I (and probably all reviewers) do it all by email, but if you prefer you can use the web UI.  in general you don't need to worry about pinging them when you reply to the review or something, unless they take too long to get back to you
<adiroiban> how many days are too long ? :)
<salgado> adiroiban, when the reviewer is on call, 0.1 days is too long for me. ;)
<adiroiban> ok. so if I will no solve the problem in the same day, I will have to wait for next week?
<salgado> adiroiban, no, your reviewer should be happy to do a second round of review (or just ack your changes) when they're not on call.  it might take a bit longer but never more than 1 or 2 days
<salgado> and if your original reviewer is too busy they'll probably say so and you can ask another reviewer to continue from where the previous one left
<adiroiban> many thanks,
<adiroiban> so basically, to start a review, I only have to add myself in the IRC topic, and then communicate via email
<salgado> adiroiban, yup.  once your name is in the queue we'll look up your branch on https://code.edge.launchpad.net/launchpad/+activereviews or ping you if we can't find it
<adiroiban> great
<adiroiban> or I can just add adiroiban(bug-32323) ?
<adiroiban> or MP id?
<salgado> any of them would work
<salgado> adiroiban, can you get a couple screenshots of that translations page after your changes for a UI review?
<adiroiban> salgado: sure. It is ok if I attach them to the bugreport?
<salgado> adiroiban, sure!  then you can ask beuno or noodles775 for a UI review. :)
<beuno> I can review
<adiroiban> beuno: I'm taking the screenshots
<beuno> cool
<salgado> noodles775, has your branch been reviewed already?
<adiroiban> beuno: screenshots here https://bugs.edge.launchpad.net/rosetta/+bug/427319
 * beuno looks
<beuno> adiroiban, nice improvement
<beuno> a few questions
<beuno> why not "Your suggestions will be held for review by $TEAM"?
<beuno> there's also a missing full stop
<noodles775> salgado: yep.
<beuno> after the team's name
* noodles775 changed the topic of #launchpad-reviews to:  on call: adeuring,salgado || reviewing: -, - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<beuno> before "Templates which are..."
<beuno> other than that, ui=me
<adiroiban> beuno: ok. I will change that.
<jpds> adeuring: EC2 test go OK?
<adiroiban> how should I wrap this line: translations_contact_name = self.translation_team.translator.displayname
<salgado> adiroiban, I'd use parenthesis
<salgado> translations_contact_name = (
<salgado>     self.translation_team.translator.displayname)
<salgado> I think that's the preferred way in Launchpad
<adiroiban> salgado: many thanks. I just wanted to be sure.
* adeuring changed the topic of #launchpad-reviews to: on call: salgado || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<adiroiban> salgado: should I append the new diff to the reply email ?
<salgado> adiroiban, if you think it'd be worth me having a look, sure
<EdwinGrubbs> salgado: hey, I have a 1128 line diff, but most of it is generic changes like indenting a function converted to a method and substsitutions like s/client/self.client/.   Since I am just moving the registry windmill tests into the lp/registry/windmill directory, I could easily split up this diff, but that would only be benefiicial if somebody else would review the other half.
<salgado> EdwinGrubbs, I'll be leaving in less than 10 minutes, so I won't be able to review it, sorry
 * EdwinGrubbs cries
<EdwinGrubbs> sinzui: would you be able to review a boring 1128 line diff for the windmill tests?
<sinzui> yes, in about 20 minutes
* salgado changed the topic of #launchpad-reviews to: on call:  || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<salgado> adiroiban, just replied.  I'll step out for about an hour, but I'll be back later to submit your branch to ec2
<adiroiban> salgado: ok. I'm also out
<EdwinGrubbs> sinzui: here is the mp: https://code.edge.launchpad.net/~edwin-grubbs/launchpad/bug-495067-move-windmill-tests/+merge/16057
<sinzui> EdwinGrubbs_: r=me, land it if you can
<EdwinGrubbs_> sinzui: thanks
#launchpad-reviews 2010-12-13
* 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
<StevenK> henninge: Hi! Could you look at https://code.edge.launchpad.net/~stevenk/launchpad/archive-validate-stringfix/+merge/43496 when you have a few moments?
<henninge> StevenK: Has the TeamSubscriptionPolicy any bearing on the test or ist that just noise?
* henninge changed the topic of #launchpad-reviews to: On call:  henninge || Reviewing: StevenK || queue: [-] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<henninge> Ah, "open teams cannot have ppas" ;-)
* 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
<danilos> henninge_, one quick branch for you: https://code.launchpad.net/~danilo/launchpad/bug-680044/+merge/43515
<henninge> danilos: looking
* henninge changed the topic of #launchpad-reviews to: On call:  henninge || Reviewing: danilos || queue: [-] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<danilos> henninge, changing the topic probably took longer than reviewing it :)
<danilos> henninge, but thanks :)
 * henninge gets back from getting a drink ;)
* henninge changed the topic of #launchpad-reviews to: On call:  henninge || Reviewing: - || queue: [-] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<henninge> danilos: done
<danilos> henninge, thanke schÃ¶n
<henninge> ;)
<jcsackett> henninge: got one for the queue for you. https://code.edge.launchpad.net/~jcsackett/launchpad/redirects-681034/+merge/43392
* jcsackett changed the topic of #launchpad-reviews to: On call:  henninge || Reviewing: - || queue: [jcsackett] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* henninge changed the topic of #launchpad-reviews to: On call:  henninge || Reviewing: jcsackett || queue: [-] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<henninge> jcsackett: looking
<jcsackett> thanks.
* sinzui changed the topic of #launchpad-reviews to: On call:  henninge || Reviewing: jcsackett || queue: [sinzui, sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* henninge changed the topic of #launchpad-reviews to: On call:  henninge || Reviewing: - || queue: [sinzui, sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<henninge> jcsackett: review done. ;)
<jcsackett> henninge: cool, thanks.
* abentley changed the topic of #launchpad-reviews to: On call:  henninge, abentley || Reviewing: -,- || queue: [sinzui, sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* henninge changed the topic of #launchpad-reviews to: On call:  henninge, abentley || Reviewing: sinzui,- || queue: [sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<henninge> Hi abentley, Hi sinzui! I am looking at the delete-team-4 branch.
<henninge> sinzui: I am a bit confused why the test uses the 'admin' celebrity while it talks about registry experts.
<henninge> I saw that the target_person must be registry experts. What is the meaning of the target_person here?
<sinzui> henninge, in the setup of the test to put the teams into the correct hierarchy?
<sinzui> henninge, in the Test case has a member of registry admins logged in. The specific tests uses `with celebrity_logged_in('admin'):` to temporarily become and admin to place the teams in the relationship. Otherwise I would need to switch between both team owners to do the invitation dance
<henninge> ah, of course
<henninge> the join method is still part of the setup, I thought it was already what is being tested.
<henninge> s/method/call/
<sinzui> henninge, decided not to make the teams share an owner because that case is rare and create a permission case that implementation can easily accomplish, but still be broken
<sinzui> I should ad a comment about the reason in the test
<henninge> sinzui: why do you import removeSecurityProxy locally?
<sinzui> hmm
* henninge changed the topic of #launchpad-reviews to: On call:  abentley || Reviewing: - || queue: [sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<sinzui> henninge, I think this is misguided approach taken in the module. It happens several times. I dates from when we had a poor understanding of when we need to elevate permissions to accomplish a task
<sinzui> I will use a single import for the whole module
<henninge> sinzui: thank you. With that, r=me ;)
<sinzui> abentley, do  you have time to review my branch: https://code.launchpad.net/~sinzui/launchpad/mailing-list-simple-0/+merge/43522
<abentley> sinzui, sure.
* danilos changed the topic of #launchpad-reviews to: On call:  abentley || Reviewing: - || queue: [sinzui, danilo x 2] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<danilos> abentley, I am EOD but put two (related) branches up for review; if you prefer OCR, feel free to ignore them; thanks :)
<abentley> danilos, I do prefer OCR, but I'll see what I can dol.
<abentley> s/dol/do
<danilos> abentley, yeah, if someone else has a real OCR branch, just do it before mine
<danilos> abentley, cheers
<abentley> sinzui, I can't think of anything insightful to say.
<abentley> sinzui, r=me.
<sinzui> thanks abentley
* abentley changed the topic of #launchpad-reviews to: On call:  abentley || Reviewing: - || queue: [danilo x 2] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* 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
<allenap> abentley: Are you up for reviewing a short and fairly simple branch? https://code.launchpad.net/~allenap/launchpad/bug-mail-case-sensitive-domain-bug-48464-fix/+merge/43547
<abentley> allenap, sure.
<allenap> Thanks :)
* abentley changed the topic of #launchpad-reviews to: On call:  abentley || Reviewing: allenap || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<abentley> allenap, you should add to copyright years, not delete them.  i.e. 2009-2010 not 2010.  Unless you've removed all the code that dates from 2009...
<allenap> abentley: Ah, okay, I'll fix that.
<abentley> allenap, aside from that, looks good.
<allenap> abentley: I never really knew how to treat them before.
<allenap> abentley: Cheers!
<abentley> allenap, Oh, I guess the new tests need docs.
<allenap> abentley: Okay.
* abentley changed the topic of #launchpad-reviews to: On call:  abentley || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<gary_poster> abentley: please take a look at https://code.launchpad.net/~gary/launchpad/bug683115/+merge/43572 when you have a chance.  A quick one.
<abentley> gary_poster, r=me
<gary_poster> thanks abentley
* abentley changed the topic of #launchpad-reviews to: On call:  - || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
#launchpad-reviews 2010-12-14
<danilos> henninge, can I nudge you to take a look at https://code.launchpad.net/~danilo/launchpad/bug-682186/+merge/43549 (abentley only 'needsfixing'-ed it because of a [not so tiny] lint issue :)
<henninge> danilos: r=me ;)
<danilos> henninge, thanks again :)
* gmb changed the topic of #launchpad-reviews to:  On call:  gmb || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* gmb changed the topic of #launchpad-reviews to:  On call:  gmb || Reviewing: pcjc2 || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* gmb changed the topic of #launchpad-reviews to:  On call:  gmb || Reviewing: abentley || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* gmb changed the topic of #launchpad-reviews to:  On call:  gmb || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jcsackett> gmb: thanks for reviewing that MP even though i hadn't thrown it into the queue. :-)
<gmb> jcsackett: It was in the +activereviews queue and I was bored :)
<jcsackett> heheh. works for me. thanks again. :-)
<gmb> allenap: Fancy a bugjam review?
<allenap> gmb: Absolutely.
<gmb> allenap: https://code.edge.launchpad.net/~gmb/launchpad/fix-update-bug-watch/+merge/43643
<gmb> Code cleanup. Loverly jubbly.
<bac> hi gmb.  Have time for https://code.launchpad.net/~bac/launchpad/bug-662994/+merge/43647
<gmb> bac: Sure.
* gmb changed the topic of #launchpad-reviews to:  On call:  gmb || Reviewing: bac || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<bac> cool
<gmb> bac: r=me with some stylistic nitpicks
<bac> gmb: great, thanks
* gmb 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
<bac> thanks gmb.  fwiw, i wasn't following the PSG strictly but it wasn't as ugly as shown by the diff in the MP, which is disturbing
<mars> gmb, hi, I'll be out for a while yet - have a staff meeting in Montreal I need to attend
* mars changed the topic of #launchpad-reviews to:  On call:  mars || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<mars> gmb, unping, oops, I didn't see the topic first
* mars changed the topic of #launchpad-reviews to:  On call:  mars[meeting] || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* gmb changed the topic of #launchpad-reviews to:  On call:  gmb, mars[meeting] || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<gmb> mars: No worries; I'd just popped out to run an errand.
<gmb> allenap`, deryck: If either of you have a moment for another bugjam branch, there's one here: https://code.edge.launchpad.net/~gmb/launchpad/bug-557032/+merge/43671
<deryck> gmb, I can take it.
<gmb> deryck: Thanks
<gmb> It's a small one. Just a merge of one of Bryce's many patches.
<deryck> gmb: looks good.  r=me
<gmb> Ta
<deryck> np
* gmb changed the topic of #launchpad-reviews to:  On call:  mars[meeting] || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* mars changed the topic of #launchpad-reviews to:  On call:  mars[meeting] || Reviewing: lunch || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* mars changed the topic of #launchpad-reviews to:  On call:  mars|| Reviewing: lunch || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* mars changed the topic of #launchpad-reviews to:  On call:  mars || Reviewing: lunch || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jcsackett> mars: when you're back from lunch https://code.launchpad.net/~jcsackett/launchpad/linkifier-bugs/+merge/43677
* jcsackett changed the topic of #launchpad-reviews to:  On call:  mars || Reviewing: lunch || queue: [jcsackett] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* mars changed the topic of #launchpad-reviews to:  On call:  mars || Reviewing: jcsackett || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* mars changed the topic of #launchpad-reviews to:  On call:  mars || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* sinzui changed the topic of #launchpad-reviews to:  On call:  mars || Reviewing: - || queue: [sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<sinzui> mars: do you have time to review https://code.launchpad.net/~sinzui/launchpad/mailing-list-email-0/+merge/43656
<abentley> mars: could you please review https://code.edge.launchpad.net/~abentley/launchpad/use-existing/+merge/43701 ?
<mars> abentley, sinzui, sure
* mars changed the topic of #launchpad-reviews to:  On call:  mars || Reviewing: - || queue: [sinzui, abentley] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<abentley> mars, thanks.
* mars changed the topic of #launchpad-reviews to:  On call:  mars || Reviewing: abentley || queue: [sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<mars> shortest wins
<sinzui> mine is not short
<mars> sinzui, yes, sorry.  I'll review Aaron's, then some work with Chex, then yours
<abentley> mars, a .bzr directory is a control directory, and its format is a ControlFormat.
* mars changed the topic of #launchpad-reviews to:  On call:  mars || Reviewing: sinzui || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<mars> abentley, ok
<mars> abentley, so lines 55 and 65 are summarized as '# create the .bzr control directory' then
<abentley> mars, Well, except that we're actually manipulating the database.
<mars> ah
<mars> abentley, ok, no worries then
<mars> sinzui, you are a doctest eating machine
<sinzui> I am reading one right now and I cannot think why it keep repeating the same information
<sinzui> I think I need to delete most if it
<mars> sinzui, is there any way to use a factory for the mailman tests instead of sample data?
<mars> for the email addresses
* mars changed the topic of #launchpad-reviews to:  On call:  mars || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<sinzui> mars, I do not think sample data is being used for any mailman test. we do not have any mailman sample data
<mars> sinzui, ok.  I saw the test had some specific email addresses in it, and wasn't sure if they were sample data or not. I may have not read them carefully enough
<mars> sinzui, ah, I see
<mars> line 558 is the difference
<mars> I wasn't sure if gerbil's email was in the database or not
<mars> I guess the answer is 'not'
<mars> sinzui, interesting typo on line 538 as well
<sinzui> mars, the messages require an email address. I prefer not to use the factory to create random data for items under test. Many tests are using removeSecurityProxy() to get the email address to create a message. I think that implies bad design
<sinzui> mars, all the email addresses in the test are rodents starting with a different letter of the alphabet.
#launchpad-reviews 2010-12-15
* mars changed the topic of #launchpad-reviews to:  On call:  - || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* jelmer changed the topic of #launchpad-reviews to: On call: jelmer || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<stub>  https://code.launchpad.net/~stub/launchpad/pending-db-changes/+merge/43763, diff at http://paste.ubuntu.com/544036/
<stub> jelmer: ^
<jelmer> stub: Sorry, just got back from lunch
<jelmer> Is the diff generator down btw? That diff still hasn't appeared.
<jelmer> stub: Wouldn't this unnecessarily fail if the last line contained a comment?
<stub> Yes, but I'd rather keep it simple rather than attempt to implement an SQL parser.
<stub> I could steal the old regexp to strip comments, but that still has holes if you quote strings certain ways I think.
<stub> False positives with an easily understood error message, or potential strange issues like the bug this branch is addressing.
<jelmer> stub: Fair enough, though I don't think the error message is clear enough in that case.
<stub> Suggestion?
<jelmer> I think it should explicitly mention that the last non-whitespace character in the file should be a semicolon. if I would hit this error I'm pretty sure I would ignore comments when searching for missing semicolons.
<stub> k
<jelmer> stub: Other than that, r=me
<stub> ta
<benji> On call: jelmer, benji || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<benji> heh
* benji changed the topic of #launchpad-reviews to: On call: jelmer, benji || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jelmer> 'morning benji :-)
<benji> :)
<benji> leonardr: thanks for the info
<leonardr> benji: i'm writing a merge proposal now. at some point i need to take a nap--i woke up at 4 am
<benji> mars: I would have expected the exit code of the script to be that of the test run
<benji> leonardr: k :)
<benji> mars: shal I investigate?
<benji> s/l/ll/
<mars> benji, please do
<benji> k
<mars> benji, the test log I used was (I think) test.log.1
<mars> benji, like I said in the RT, check the shell history
<benji> k
<mars> thanks benji
* EdwinGrubbs changed the topic of #launchpad-reviews to: On call: jelmer, benji, Edwin || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<EdwinGrubbs> hi benji, it looks like there are couple of reviews in the queue that you can do. https://code.launchpad.net/launchpad/+activereviews
<benji> EdwinGrubbs: yay!  I forgot to look there.  I'll pick one up now.
<leonardr> benji: https://code.launchpad.net/~leonardr/launchpadlib/retry-on-invalid-token/+merge/43784
<EdwinGrubbs> jelmer: do you want to review one of my branches? The diff is actually much smaller than what the mp will say. https://code.launchpad.net/~edwin-grubbs/launchpad/bug-638924-testfix/+merge/43791
<jelmer> EdwinGrubbs: sure
<jelmer> I'll make that my last review of the day though, if that's ok.
<EdwinGrubbs> sure
<jelmer> EdwinGrubbs: I need to get some dinner, I'll finish the review of your branch when I get back.
<EdwinGrubbs> jelmer: ok, that's fine
<jelmer> EdwinGrubbs: btw, I didn't see a linked bug for the MP  but the URL suggests there is one?
<EdwinGrubbs> jelmer: oops, I'll fix that
<gary_poster> flacoste: do you happen to have any context on that revision that I'm reverting for adeuring?  In a perfect world, someone with context would, since I have relatively little.  The ones with context that I know of are Robert, Abel, and possibly Graham, since he reviewed Abel's branch.
<gary_poster> If you don't have much/any context, I'll just throw it to the on-call reviewers and hope that we are all on the right track.
<gary_poster> (The MP is https://code.edge.launchpad.net/~gary/launchpad/revert12041/+merge/43811 FWIW)
 * flacoste looks
<gary_poster> The basic context is trying to turn on Robert's librarian token stuff, I think
<gary_poster> I guess that didn't work out yet
<flacoste> gary_poster: yep, exactly, abel wrote some code to support private attachments in bugs this summer and did it using the existing patterns of streaming the file from the private librarian using the app server
<flacoste> gary_poster: this branch was supposed to remove that and serve files directly from the restricted librarian using the token-based protocol
<flacoste> it fails qa
<gary_poster> Got it
<flacoste> your reversal looks correct, i'll approve
<gary_poster> OK, thank you flacoste
<flacoste> gary_poster: don't forget the magic flags --rollback when submitting
<gary_poster> right, thanks :-)
<EdwinGrubbs> jcsackett: I've responded to your review from last week.
 * jcsackett looks.
<jcsackett> EdwinGrubbs: r=me on code*. i've requested a follow up from sinzui. thanks for those changes.
* sinzui changed the topic of #launchpad-reviews to: On call: jelmer, benji, Edwin || Reviewing: - || queue: [sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<sinzui> EdwinGrubbs,  ping
<EdwinGrubbs> sinzui: hi
<sinzui> EdwinGrubbs,  about your PPA MP. The message shown to the user is "Remove", but the only link I see is "Delete"
<EdwinGrubbs> sinzui: good point
<sinzui> EdwinGrubbs, Can I delete a PPA that has no published packages? What happens to it during the merge. It is gone or transferred
<sinzui> EdwinGrubbs, actually, I know the PPA is not transferred during a merge because there is a bug about merged users having PPAs.
<EdwinGrubbs> sinzui: how is that different than the bug I'm working on?
<sinzui> Edwin, we should prepare a query that sets all PPAs that belong to merge users to DELETED once your branch is released
<sinzui> EdwinGrubbs, because we did not recognise this bug years ago, the UI had 404s
<sinzui> EdwinGrubbs, the links to the PPAs will go away when the archives are set to DELETED
<EdwinGrubbs> sinzui: besides changing "Remove" to "Delete", is there anything else that needs to be changed in this branch?
<sinzui> EdwinGrubbs,  has_ppa_with_published_packages() will let me merge with an active ppa
<sinzui> EdwinGrubbs, that ppa will still be searchable
<sinzui> I think the rules must change in the method, or merge deactivates it for the user
<EdwinGrubbs> sinzui: ok, I was basing this on the same restrictions as for person renames, which is what Julian suggested, however, he might have not been thinking about that situation.
<sinzui> merge purges lists for users. It is okay for it to delete an empty ppa, but I think delete is slow...it goes into DELETING first
<EdwinGrubbs> sinzui: maybe, I should ask someone in soyuz if that is safe to do, then.
<sinzui> EdwinGrubbs, +1
<sinzui> I expect wgrant knows
<EdwinGrubbs> wgrant: ping
<wgrant> sinzui, EdwinGrubbs: Just running late for a sprint... I'll be around in 20 or so.
<wgrant> sinzui, EdwinGrubbs: You need the PPA to be DELETED before you do the merge.
<wgrant> sinzui, EdwinGrubbs: Since it needs to remove a directory named after the user from disk.
<sinzui> wgrant, thanks
<EdwinGrubbs> wgrant: if the user clicks on "Delete" for the PPA, does it take place immediately, or is it queued up?
<wgrant> EdwinGrubbs: Queued up.
<wgrant> Since it has to happen on germanium.
<wgrant> It could take half an hour or more, if things are running really slowly.
<wgrant> But normally <5min.
<EdwinGrubbs> wgrant: does the user get an email when it's done, so they know when they can finally merge their account?
<wgrant> EdwinGrubbs: No.
<thumper> anyone? https://code.edge.launchpad.net/~thumper/launchpad/bugjam-619555-request-builds-logging/+merge/43731
<EdwinGrubbs> benji: is it already your end of day? ^^^
<EdwinGrubbs> thumper: I can take it.
<benji> EdwinGrubbs: nope, 30 minutes left; if it's a small one I can take it, but I'm neck deep in Leonard's review at the moment
<benji> EdwinGrubbs: I can take it, it's a small one
<leonardr> benji: how is the review going? do you need anything?
<benji> thumper: I've got'cha
<thumper> thanks
<benji> leonardr: I can't think of anything, but I think it's going to have to be a two parter so I can at least get you something by EOD today; I first did a once-over to get the idea of what was going on and now I'm doing a more detailed review.  I'll give you what I have in about 20 minutes.
<leonardr> benji: ok, i'm almost eod as well
<benji> yep
<benji> you can sleep on it ;)
<benji> EdwinGrubbs: I've reviewed https://code.edge.launchpad.net/~thumper/launchpad/bugjam-619555-request-builds-logging/+merge/43731
<thumper> ta
<benji> thumper: since I'm being mentored, Edwin will have to approve too, of course
<benji> but you're still welcome ;)
<thumper> ok
<benji> EdwinGrubbs: I've partially reviewed https://code.launchpad.net/~leonardr/launchpadlib/retry-on-invalid-token/+merge/43784.  I don't know if you want to look at what I've done now or if you can look at the whole thing once I've finished it tomorrow.
<benji> leonardr: I've put your bedtime story in a comment on https://code.launchpad.net/~leonardr/launchpadlib/retry-on-invalid-token/+merge/43784. ;)
<leonardr> ok, thanks
<benji> leonardr: the short version is "it looks good"
<benji> leonardr: oh, one thing I forgot to mention, the tests fail for me :(
<leonardr> benji: RequestTokenAuthorizationEngine has changed but i know that no one uses it right now, so i'm ok with changing it
<leonardr> really. paste the errors?
<benji> cool
<benji> yep, pasting now
<benji> leonardr: false alarm; I forgot that I did something stupid with that branch; the up-to-date verion passes just fine
<leonardr> ah, ok
* benji changed the topic of #launchpad-reviews to: On call: jelmer, Edwin || Reviewing: - || queue: [sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<EdwinGrubbs> benji: I'll try to to look at what you've done so far.
<benji> thanks
<benji> ok, I'm off to buy a new van (why does the material universe have to hate me?)
<leonardr> benji: did your van break/crash?
<benji> leonardr: yeah; it's been broken for a while, but we found out yesterday just how broken it is (transmission is not so slowly turning itself into small metal shavings)
<leonardr> ah
<leonardr> good luck
<benji> thanks
<EdwinGrubbs> thumper: is it worthwhile to create a new logger class instead of just re-using c/l/scripts/logger.py/BufferLogger
<EdwinGrubbs> thumper: if there is a benfit to having it in a list instead of a StringIO, it seems like you would have a much more feature-full logger by subclassing from FakeLogger.
<thumper> I didn't see any point in having a fully featured logger
<thumper> I though we could add things as we need them
<thumper> although looking now
<thumper> perhaps bufferedlogger would have been enough
<thumper> I can change
#launchpad-reviews 2010-12-16
* jelmer changed the topic of #launchpad-reviews to: On call: -  || Reviewing: - || queue: [sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* jcsackett changed the topic of #launchpad-reviews to: On call: jcsackett  || Reviewing: - || queue: [sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jcsackett> sinzui: i'm on call today but i don't think it makes sense for me to grab the review you have in the queue.
<jcsackett> what with you being the one who does my follow up. :-P
<sinzui> I will ask bac or EdwinGrubbs
<bac> sinzui:  i can do it
<jcsackett> thanks, bac.
<jcsackett> sinzui, bac: mind if i take you out of the queue in the topic?
* sinzui changed the topic of #launchpad-reviews to: On call: jcsackett  || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<bac> sinzui: i haven't gotten to your review yet and now i have the YUI thing for an hour.  are you in a huge rush?
<sinzui> No rush at all
<henninge> danilos: did you want to review the branch or shall I ask jcsackett to look at it?
<danilos> henninge, just go for jcsackett, I can only glance to see that you didn't remove something important :P
<henninge> jcsackett: Hi! I have a somewhat oversized branch but it has a lot of mechanical changes. I wrote up a nice cover letter, too. ;)
* henninge changed the topic of #launchpad-reviews to: On call: jcsackett  || Reviewing: - || queue: [henninge] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jcsackett> henninge: shoot me the MP, i'm happy to look at it.
<henninge> https://code.launchpad.net/~henninge/launchpad/db-devel-688519-getusedtranslationmessage/+merge/43924
<henninge> jcsackett: thanks
* jcsackett changed the topic of #launchpad-reviews to: On call: jcsackett  || Reviewing: henninge || queue: [ - ] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<bigjools> mrevell: care to take a butcher's? https://code.launchpad.net/~julian-edwards/launchpad/front-page-link-to-ppas-bug-583707/+merge/43929
<mrevell> bigjools, I certainly would.
<bigjools> as long as you don't tell me to sling my butcher's
<mrevell> sorry for the delay bigjools, I'm having some trouble getting it to run.
<bigjools> mrevell: can I help?
<mrevell> bigjools, When I run make run I get the following:
<mrevell> http://pastebin.ubuntu.com/544503/
<bigjools> mrevell: run rocketfuel-get
<bigjools> your sourcecode is out of date
<mrevell> Oh, I had but after I'd pulled your branch. Do I need to merge in devel?
<mrevell> yes, I do, right, working
<bigjools> mrevell: no need to merge
<bigjools> just update sourcecode
<henninge> jcsackett: I have to run out now, sorry.
<jcsackett> henninge: no worries.
<bigjools> mrevell: any good?
<mrevell> bigjools, I've updated sourcecode and it then asked me to rerun link-external... so I did that and it still won't make run ... I'm trying rocketfuel-get again for good measure.
<bigjools> rerun?
<bigjools> mrevell: easy steps: 1. rf-get, 2. get my branch, 3. link-ext..., 4. make run
<bigjools> that should be enough
<mrevell> Yeah, did all that ... ah,  it's complaining about repo format when I do the rf-get
<mrevell> bigjools, running rf-setup to get a clean sheet
<bigjools> !
<bigjools> mrevell: can you paste output?
<mrevell> bigjools, link-external-sourcecode gives me this: http://pastebin.ubuntu.com/544510/
<bigjools> mrevell: try utilities/link-external-sourcecode ../../lp-sourcecode
<mrevell> hmm
<mrevell> bigjools, http://pastebin.ubuntu.com/544511/
<mrevell> oh, source-deps
<mrevell> ?
<bigjools> sourcedeps my bad
<bigjools> ok now?
<mrevell> okay, so that worked, thanks ... make run gives me: http://pastebin.ubuntu.com/544512/
<mrevell> which is what I was getting earlier
<mrevell> bigjools, When I try rocketfuel-get it's complaining about different rich root support. Does that sound familiar?
<bigjools> ignore it
<bigjools> mrevell: ok
<bigjools> try utilities/update-sourcecode
<mrevell> running
<mrevell> each one said "no change"
<mrevell> trying make run ... no luck
<bigjools> hmph
<bigjools> mrevell: have you got lp-sourcedeps/eggs/testtools-0.9.8_r151-py2.6.egg
<mrevell> bigjools, no
<bigjools> mrevell: are you running utilities/update-sourcecode in my branch's directory?
<mrevell> I am
 * bigjools is stumped
<mrevell> trying update-sourcecode again as I have no better ideas
<bigjools> mrevell: this has happened to me before - I think I blew away all my eggs and restarted
<bigjools> but it'd  be nice to work out wtf is going on
<leonardr> benji-lunch, going to give you an incremental diff for yesterday's branch
<bac> hi sinzui
<sinzui> hi bac
<sinzui> EdwinGrubbs, bac: I have another branch of 3 trivial bugs that needs review
<bac> hi sinzui.  i was testing your branch and created a new team with mailing list.  i was momentarily confused when i got the message "Mailing list requested and queued for approval"
<sinzui> yuck
<bac> is that still there just to indicate it isn't available immediately?  very misleading
<EdwinGrubbs> sinzui: I can take it.
<sinzui> lets get that message out of the code
<sinzui> bac, the list is not available immediately
<bac> sinzui: yes, as above
<bac> i think a new message that said "give us a few minutes already" would be swell
<bac> "hold your horses"
<sinzui> bac: maybe: "The mailing list will be available in a few minutes"
<bac> not as colorful but will work
<sinzui> There is a bug I decided to postpone. the team page implies the list and archive are working immediately
<sinzui> I want a message I can show in the portlet and on the configure page.
<leonardr> benji-lunch: https://code.launchpad.net/~leonardr/launchpadlib/retry-on-invalid-token/+merge/43784
<leonardr> now going to work on your feedback
<bac> sinzui: when creating a mailing list the button still says "Apply for a mailing list" which is misleading too
<sinzui> yeo
<sinzui> yep
<sinzui> that is also in the ug
<sinzui> bug
<bac> oh ok
<sinzui> there is a counter part too. The list is deactivated, but the button say create a list
<benji> leonardr: looking
<bac> sinzui: so i'm confused.  are you saying all of those issues are bundled in another bug?
<sinzui> yes
<sinzui> teo bug
<sinzui> two bugs
<bac> sinzui: ok, then i won't mention them in this review.  gotcha.
<bac> sinzui: were you getting an assertion error due to the lack of a page_title
<sinzui> bac: I thought I could fix them in this branch, but I think they should be a separate branch
<bac> sinzui: +1
<sinzui> bac, yes
<bac> sinzui: why is that assertion not seen in the web UI?
<sinzui> I think the upcall to the parent may be different. The class *should* provide page_title because the parent's label is different, thus the page title is different (and wrong) too
<bac> sinzui: r=bac
* jcsackett changed the topic of #launchpad-reviews to: On call: jcsackett  || Reviewing: - || queue: [ - ] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jcsackett> henninge: just in case you're no monitoring the channel i accidentally sent this message to first, there are comments/questions on your MP.
<jcsackett> please, no more 800+ branches, folks. :-P
* jcsackett changed the topic of #launchpad-reviews to: On call: jcsackett  || Reviewing: grabbing lunch || queue: [ - ] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* jcsackett changed the topic of #launchpad-reviews to: On call: jcsackett  || Reviewing: - || queue: [ - ] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<leonardr> benji: "programmatic enforcement of the
<leonardr>     given rules is warranted"
<leonardr> you mean by raising ValueError?
 * leonardr considering raising ValueError if the values given are _inconsistent_, but leaving it alone otherwise
<benji> leonardr: right; it seems that there are many combinations of arguments that leave some ignored, offering opportunities for user frustration; it would be nice to short circuit some of that frustration
<flacoste> gary_poster: can you make sure that bug 673252 is QAed?
<_mup_> Bug #673252: server.* scope not robust on misspelt scopes <feature-flags> <lp-foundations> <qa-needstesting> <Launchpad itself:Fix Committed by stub> < https://launchpad.net/bugs/673252 >
<gary_poster> flacoste: ok, on call, but will arrange
<flacoste> jcsackett: and you can QA bug 681034?
<_mup_> Bug #681034: UnknownURLScheme raised running distributionmirror-prober script <lp-registry> <oops> <qa-needstesting> <Launchpad itself:Fix Committed by jcsackett> < https://launchpad.net/bugs/681034 >
<gary_poster> benji, do you have availablility to QA the bug above
<gary_poster> ?
<jcsackett> ah, flacoste, sorry. i qa'ed it but forgot to change the tag. one moment.
<benji> gary_poster: sure
<gary_poster> thank you much benji
<benji> gary_poster: wait, jcsackett said that he QAed it
<gary_poster> benji that was another bug?
<jcsackett> benji: i qa'd 681034.
<jcsackett> distributionmirror-prober.
<benji> ah!  the reverse-cronology of the scroll-back confused me; I'll QA 673252
<benji> generally we need to be better about writing down solid reproduction steps
<benji> gary_poster, flacoste: 673252 is qa-ok
<gary_poster> thank you benji
<benji> np
<flacoste> thanks
<jcsackett> flacoste: 681034 is tagged qa-ok.
<EdwinGrubbs> sinzui: review sent
* jcsackett changed the topic of #launchpad-reviews to: On call: -  || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
#launchpad-reviews 2010-12-17
<StevenK> WTB reviewer, PST
<wallyworld> StevenK: ?
<StevenK> wallyworld: WoW-ism, it expands into "Want To Buy reviewer, Please Send Tell"
<StevenK> Where tell is a WoW-ism for privmsg
<wallyworld> StevenK: does that mean you want a review to do? :-)
<wallyworld> trivial one: https://code.edge.launchpad.net/~wallyworld/launchpad/oops-bug-filing-location/+merge/44005
 * StevenK stabs wallyworld 
<StevenK> No, I want someone to review one of MINE!
<wallyworld> it's really trivial - change launchpad-code to launchpad in 3 places
<wallyworld> tick and flick really
<wallyworld> sorry
<wallyworld> no, i am, really :-)
<StevenK> +1
<wallyworld> thanks :-)
<StevenK> Are you a mentat reviewer, or not yet>
<StevenK> s/\>/?/
<wallyworld> no, not yet
<StevenK> s/\(mentat\) \(reviewer\)/\2 (\1)/
<wallyworld> btw, i know the urls redirect but there was a bug and it is bugjam :-)
<wgrant> Excuses, excuses.
<wallyworld> StevenK: so shouldn;t there be an on call reviewer around? the channel topic always seems blank
<wallyworld> i have another mp too but the sound of crickets is quite loud
<wgrant> wallyworld: This is APac.
<wgrant> There is no good OCR coverage yet.
<wgrant> But this should change in 6 months, when there are three more...
<wallyworld> wgrant: sure. maybe i don't recall seeing the topic set in the evenings or early mornings AEST
<StevenK> I see it early mornings from the US people
<StevenK> Or jelmer, if he forgot to edit the topic
<wgrant> Well, a few weeks ago the UK would have been awake by this time.
<wgrant> But DST makes me cry.
 * wallyworld needs to find some glasses
<wallyworld> i love dst :-) wish we had it :-(
<StevenK> Queensland is broken, find a better state.
<wallyworld> can't beat the climate though
<wgrant> Too hot :(
<wallyworld> only if you're soft :-)
<wgrant> Only 9.2EB of data left :(
<StevenK> "Only"
<henninge> danilos: were you aware of assert statements being deprecated now?
<henninge> bac: are assert statements really deprecated now? Were do I read about that?
<henninge> bac: and Hi! ;)
<danilos> henninge, I didn't
<henninge> I am not sure that is true ...
<danilos> henninge, https://dev.launchpad.net/PythonStyleGuide
<danilos> henninge, which points to https://dev.launchpad.net/AssertionsInLaunchpad
<henninge> yes, reading that
<henninge> it's a year old and describing how they can be used
<henninge> my reviewer just sounded like there is a newer guideline.
<danilos> henninge, our style guide uses them for a few examples as well, and doesn't explicitely forbid them so use them :)
<danilos> henninge, also, with python's dynamic typing, it'd be very weird to do away with them
<henninge> danilos: the suggestion was to turn this particular one into a Value error.
<henninge> if side is None:
<henninge>     raise ValueError("Translation side must be specified").
<danilos> henninge, if that particular assertion would be hit as a cause of programmer error, it should stay an assertion imo
<henninge> danilos: yes, that is my impression, too.
<henninge> glad we agree ;-)
<danilos> henninge, well, valueerror in an OOPS would not make much sense, and would, arguably, be catchable in callsites
<danilos> henninge, assertionerrors should never be handled in call sites so we'd know a developer has made an error
<henninge> danilos: I never said I liked the suggestions, I was just wondering why he said that asserts are deprecated now.
<henninge> I'll be interested in his answer
<danilos> henninge, oh, I don't know that, but let me consult my crystal ball... I'll let you take a peak as well :)
<henninge> well, I could also call him to wake him and ask him ... ;-)
<henninge> some kind of crystal ball that would be ...
<StevenK> Is there any reviewer up for a short one?
<henninge> Looks like abel is not around any more.
<henninge> StevenK: what have you got?
<StevenK> I suspect he is off sick still
<StevenK> henninge: https://code.launchpad.net/~stevenk/launchpad/p3a-description-fix/+merge/44013 +8/-9
<henninge> oh, didn't know he was
<henninge> +8/-9? Too big! :-P
<StevenK> Haha
<henninge> StevenK: You also fixed two long lines, right? Or are those that part of the actual fix?
<StevenK> henninge: Before and after they fix they are one-char off
<henninge> ???
<henninge> I meant these:
<henninge> 32	-    "PPA named p3a for Celso Providelo" (ppa:cprov/p3a), which is hosted
<henninge> 33	-    by Launchpad and has the following description:
<henninge> 34	+    "PPA named p3a for Celso Providelo" (ppa:cprov/p3a), which is hosted by
<henninge> 35	+    Launchpad and has the following description:
<henninge> that is just cosmetic, not part of the fix, right?
<StevenK> henninge: Oh, sorry. Those had to be changed due to turning on -NORMALIZE_WHITESPACE
<StevenK> Well, turning off ... :-)
<henninge> oh, to match the lines breaks in the actual message? I see
<StevenK> Yes, it's icky :-(
<henninge> StevenK: thanks for the fix. r=me ;)
<StevenK> henninge: Thanks! :-)
<danilos> StevenK, hi, considering you are likely the only soyuz guy around, would you mind reviewing https://code.launchpad.net/~danilo/launchpad/bug-685624/+merge/44035 (it's very short, I am just worried that I don't break anything)
* bac changed the topic of #launchpad-reviews to: On call: bac  || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<bac> good morning launchpad
<bac> checking in for my last review shift of 2010!
<danilos> bac, I've got one for you then :)
<danilos> https://code.launchpad.net/~danilo/launchpad/bug-685624/+merge/44035
<bac> danilos: ok
* bac changed the topic of #launchpad-reviews to: On call: bac  || Reviewing: danilos || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<gmb> Hi bac, I've got a couple of branches that need reviewing. Can I stick them in the queue?
<bac> gmb: sure
 * bac misses abel
* gmb changed the topic of #launchpad-reviews to: n call: bac  || Reviewing: danilos || queue: [gmb*2] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* gmb changed the topic of #launchpad-reviews to: On call: bac  || Reviewing: danilos || queue: [gmb*2] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<salgado> folks, kiko has bribed me into doing a trivial fix to allow the drivers of a blueprint's target to edit that bp, but I'm off to catch a plane soon and after that I'll be completely deprived of 'net access, so I was wondering if anybody would be willing to land that branch for me?
<salgado> the merge-proposal is https://code.edge.launchpad.net/~salgado/launchpad/target-driver-edit-blueprint/+merge/44046
<danilos> bac, I am thinking of heading off for some food, I assume there are no big questions that you've got?
<bac> danilos: the branch looks good
<bac> danilos: the description was much more complicated than the code, which is quite simple.
<danilos> bac, right, it was quite an exercise for me to get to understand exactly what's going on and to see if we can try to get at TTB from a TTBJ without storing the data; complexity of the model didn't help there, just like the fact that TTBJ is serving two different purposes didn't
<danilos> bac, I am expecting to have fun time QAing this though :)
<bac> danilos: yeah, good luck with that.  what is the ETA for cleaning this up for good?
<danilos> bac, wgrant mentioned how he'd like to get it all switched to the new model maybe as early as next week
<bac> wow!
<danilos> bac, I am morally supporting him :)
<danilos> bac, for someone who understands well exactly what's going on where it's mostly just moving code around, afaiu
<danilos> (missing comma warning :)
<danilos> ...what's going on where *,* it's...
<bac> danilos: r=bac
<danilos> bac, thanks
* bac changed the topic of #launchpad-reviews to: On call: bac  || Reviewing: gmb1 || queue: [gmb2] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<bac> salgado: i'll look at it.  if no substantial changes are required i can land it
<salgado> bac, great, thanks!
* bac changed the topic of #launchpad-reviews to: On call: bac  || Reviewing: gmb1 || queue: [gmb2, salgado] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<bac> salgado: have a nice trip
<salgado> bac, I might even be able to do the changes if you review it now.  it's just getting it through ec2 that I won't be able to
<jcsackett> bac, mind if throw https://code.launchpad.net/~jcsackett/launchpad/linkifier-bugs-2/+merge/43980 on your queue?
<bac> jcsackett: sure
* jcsackett changed the topic of #launchpad-reviews to: On call: bac  || Reviewing: gmb1 || queue: [gmb2, salgado, bac] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* jcsackett changed the topic of #launchpad-reviews to: On call: bac  || Reviewing: gmb1 || queue: [gmb2, salgado, jcsackett] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jcsackett> thanks.
<jcsackett> henninge: your branch from yesterday is r=me for code*; i have requested review from sinzui for follow up. nice start on that clean up. :-)
<bac> salgado: r=bac with one tiny fix
<bac> salgado: are you leaving for the holidays or a work trip?  will you be around to QA it?
<henninge> jcsackett: thank you! ;)
<salgado> bac, good catch;  I'm leaving on holidays.  if I get instructions to QA it, would you be kind enough to do it for me?  if not I can see if somebody from Linaro can do it
<bac> salgado: i can QA it for you.  i'll subscribe to the bug.  please update the MP with a commit message.
<salgado> oh, I need to file a bug as well
<salgado> bac, have we moved all bugs to launchpad yet?
<salgado> looks like we did
<bac> salgado: yes we did
<salgado> bac, ok, bug filed, linked to branch and security adapter renamed.  should I add QA instructions on the bug or mp?
<bac> salgado: uh, MP i guess
<bac> gmb: hey it does not look like we have a dependency on lxml
<gmb> Rats.
<gmb> That's damnably irritating.
<bac> gmb: yep.
<gmb> bac: So we've three options:
<gmb> 1) Regexes (urgh)
<gmb> 2) BeautifulSoup (performance problems)
<gmb> 3) Bug someone to make lxml a dependency.
<gmb> 3 is annoying but I'd rather do that than take the performance hit.
<bac> 3a) And get it deployed
<gmb> Yes, that too :)
<bac> WWmthaddonD?
<gmb> Heh.
<gmb> bac: Do you know to whom I need to talk to get it made into a dependency?
<bac> [flacoste, gary, mars] maybe?
<gmb> Righto, I shall talk to one of the above.
<bac> getting _new will be great but i think landing the tabindex branch is more urgent
<gmb> bac: There is no tabindex branch at the moment.
<gmb> It's a truly horrible problem
<bac> oh, i was kindof hoping your other branch in queue was it
<gmb> No, sorry :/
<bac> i'm just worried about poor dustin having an aneurysm
<gmb> Well, naturally. As am I. I don't want to be at the platform rally and have his head explode all over me.
<gmb> Lessee how quickly I can get the dependency problem sorted.
<bac> i think an uncontained aneurysm is fairly rare
<gmb> But he was *really* cross...
<bac> of course they said that about jet engines before RR proved them wrong
<gmb> This is true.
<gmb> And as we know, all engineers are just basically machines for producing code.
<gmb> ... this is going down a rabbit hole.
<bac> yes
<bac> well i'll continue on with the review then
<gmb> Cool, thanks.
 * gary_poster looks around to try and figure out what's up
<deryck> gmb: I think the dependency issue is as easy as building a tarball of lxml, adding it to download-cache, and then adding lxml == FOO to versions.cfg.
<deryck> gary_poster can confirm ^^
<deryck> or deny ;)
<bac> deryck: really?  can't it just be added to launchpad-dependencies?
<gmb> Ooo.
<deryck> bac: I thought for python libs we preferred buildout.
<bac> deryck: oh there's that
<bac> that's why my name was not in the list
<deryck> maybe I'm nosing my way into a topic I don't really know enough about :-)
 * gmb summons gary_poster to add his $0.02...
<bac> gary_poster: the only other lxml in our code base is in migrater tools, not production code
<gary_poster> we prefer Python eggs, but lxml egg is really annoying to build
<gary_poster> gotcha bac
<deryck> ah, right, I had heard that about lxml
 * deryck has never actually built lxml
 * gary_poster has, and has the scars to prove it
<benji> run away! run away!
<gary_poster> :-)
<deryck> heh
 * deryck runs away now
<gary_poster> lemme remember how to look at our deb dependencies and I'll report back gmb.  1 sec
<gmb> gary_poster: Thanks.
<bac> good new gmb.  your tests pass now.  :)
<gmb> Heh
 * bac blames his poor spelling on frigid fingers which he blames on a horrible sanyo heat pump
<gary_poster> gmb, we did use to have lxml as a deb dependency.  It was dropped Thu, 10 Sep 2009 "(tests run fine without)"
<gmb> Hah
<gmb> I hereby repudiate that statment.
<gary_poster> :-)
<henninge> bac: Is it ok if I  append my mp to the queue?
<bac> yes, of course
* henninge changed the topic of #launchpad-reviews to: On call: bac  || Reviewing: gmb1 || queue: [gmb2, salgado, jcsackett,henninge] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<henninge> bac: thanks
<gary_poster> so, I can point you to how to update the deb dependecies, or you can ping me to do it...next week.  Or *maybe* this afternoon.  kinda booked right now.
<gary_poster> gmb: ^^
 * gary_poster tries to find doc link meanwhile
<gmb> gary_poster: Feel free to point me at it and I'll give it a shot.
<gary_poster> cool
<gary_poster> gmb https://dev.launchpad.net/Hacking#Changing%20the%20launchpad%20dependency%20debs
<gmb> Thanks
<gary_poster> np.  I'm no expert, but I've done it before gmb, so I can at least stare at a problem with you :-)
<gmb> gary_poster: Cool, thanks. I'll ping you if I run into difficulties.
<bac> gmb: review done with one suggestion, other than the obvious.
* bac changed the topic of #launchpad-reviews to: On call: bac  || Reviewing: jcsackett || queue: [gmb2,henninge] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<gmb> Cool, thanks.
<flacoste> gmb: did you solve your problem?
<gmb> flacoste: Yes, I think so. Well, we'll see shortly anyway.
<abentley> bac, could you please review https://code.launchpad.net/~abentley/launchpad/email-url-body/+merge/43838 ?
<bac> sure, abentley, chunk it on the queue
* abentley changed the topic of #launchpad-reviews to: On call: bac  || Reviewing: jcsackett || queue: [gmb2,henninge,abentley] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<abentley> bac, thanks.
<gmb> bac: I've replied to your review and uploaded a new version of launchpad-dependencies, which is building now.
<bac> gmb: thanks
<bac> jcsackett: is 'blacklist' the best word to use in that context?  it seems you're just trying to find the URLs that match a pattern but they aren't blacklisted, which has a specific, loaded connotation to me.
<jcsackett> bac: i'm open to other names; i was thinking blacklist in a proxy sense--these URIs don't count/aren't linked.
<bac> 'ignored'?
<jcsackett> bac: if that works better for you, i'm happy to make that change.
<danilos> henninge, fwiw, you still seem to have left some changes to credits handling which made the side context sensitive instead of explicitely using UPSTREAM as the side
<danilos> henninge, I am thinking of line 640 (so, tests)
<bac> jcsackett: can you explain what handle_parens_in_trailers is doing?
<bac> it really needs a docstring and explanation
<jcsackett> bac: happy to provide comments in it--i had some and must have deleted them.
<danilos> henninge, I am talking about https://code.launchpad.net/~henninge/launchpad/db-devel-688519-getusedtranslationmessage/+merge/43924, fwiw ;)
<jcsackett> bac: basically, it's capturing closing parens out of the trailers from the url and adding them to the url if there is an unpaired open paren in the url.
<jcsackett> this deals with http://wikipedia.org/something_(some_catagory) style urls.
<jcsackett> right now the trailing paren is dropped, and without restructuring the entire regex there's no real way to capture the last parens if and only if there are a like number of unpaired opens in the url.
<bac> jcsackett: so were the closing parens incorrectly characterized as trailers and are being moved back to the url proper?
<jcsackett> bac: yes.
<bac> oh, ok
<jcsackett> bac: if there are unpaired open parens in the url, the closing paren in the trailer is put back in the url.
<bac> jcsackett: i see now
<bac> jcsackett: but if all the missing parens are not at the beginning of the trailers then it doesn't work so well, right?
<bac> b/c you break out of the loop at the first non-)
<jcsackett> bac: the way the regex is structured, there should only be relevant closing ones at the end. if the trailer had text &c in it first it wouldn't close of the url there.
<jcsackett> in reality, this should only ever capture one paren, but i wanted to cover something like http:///thisurl.com/has_(too_many(parens)) as long as i was handling it.
<jcsackett> bac: i'll modify the test to double check some more irritating combinations are also handled right.
<bac> jcsackett: what would this do?
<bac> (http://example.com/path_(with_parens)and_stuff)
<bac> would the extra be part of the url?  trailer is only punctuation, right?
<jcsackett> bac: good question. i'll add that example to the text case.
<bac> yes, please make some corner case tests for that method
<jcsackett> bac: my intuition is that http://example.com/path_(with_parens)and_stuff would be linkified, and the surrounding parens would not.
<jcsackett> obviously, if the user typos and "and stuff" shouldn't be part of the URI, there's no way to handle that.
<bac> jcsackett: i approved the branch but have several suggestions
<bac> included in the MP
<jcsackett> bac: when you say plain english for the regex, you mean the enormous _re_linkify?
<jcsackett> b/c speaking plainly, i still have no idea what some of it does. :-P
<bac> jcsackett: no, the trailers regex that i asked you to relocate
<jcsackett> bac:ah, i see. i got mixed up.
<bac> were it closer to its use it would be less confusing
<bac> jcsackett: looking at that regex with all punctuation i cannot recall which are things to be matched and which are special regex chars
* bac changed the topic of #launchpad-reviews to: On call: bac  || Reviewing: gmb || queue: [henninge,abentley] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<henninge> danilos: well, since the factory produces upstream templates, the side will be UPSTREAM. So the test ist not really side sensitive. It also checks the flags directly instead of using getFlag.
<henninge> danilos: so, are you saying this should be fixed before I can land this?
<danilos> henninge, right, but the way the test is written it will fail if we change the factory method
<henninge> are we going to change it?
<danilos> henninge, we are not planning to, no, but the test is sometimes used as a documentation as well, and now it implies that setTranslationCreditsAsTranslated might be side-aware when it's not
<danilos> (well, it is, in the sense that it will always use the UPSTREAM side)
<bac> gmb: remind me how to get your subscription widget tests to run
<bac> gmb: i hate to do it, but i'm going to pass on your branch.  i don't think i can add anything of value.  please find a reviewer with more JS experience.
* bac changed the topic of #launchpad-reviews to: On call: bac  || Reviewing: henninge || queue: [abentley] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<gmb> bac: Okay, no worries. I do the same thing quite often :)
<gmb> allenap, deryck: Would either of you be able to review my JS branch for the subscriptions widget?
<deryck> gmb: I can
<gmb> deryck: Cool, thanks. https://code.launchpad.net/~gmb/launchpad/use-subscribe-form-for-fun-and-profit-bug-687747/+merge/43629
 * deryck looks
<salgado> bac, thanks a lot for taking care of that branch for me.   kiko said he's going to ping you once it lands to help with QA, if needed
<bac> salgado: ok.  is it ready to land now?
<salgado> bac, yep, I've done the change you suggested
<bac> ok
<bac> salgado: have a good holiday.
<salgado> bac, thanks!  you too
<bac> salgado: sending to ec2 now
<bac> henninge: but *why* are leading '/' in filenames bad?
<henninge> bac: it's about consistency
<henninge> bac: we normalize on no leading '/'
<henninge> so that they stay comparable.
<henninge> also, tar usually strips leading slashes off file names unless you tell it not to.
<bac> henninge: ok, i see
<jcsackett> bac: when you get a chance, there's a comment in reply to part of your comments on my MP; i've got to run out for a bit.
<bac> jcsackett: ok
* bac changed the topic of #launchpad-reviews to: On call: bac  || Reviewing: abentley || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<henninge> bac: it's also  a bit dangerous to have leading slashes in tarballs. If not careful you might unpack your files into system directories.
<henninge> of course, only if you'd be working as root ...
<bac> henninge: yeah, tbh i forgot we were talking about tarballs...
<henninge> bac: thanks for the review ;)
<deryck> gmb: looks good to me.  Nice tests.  I'll add a comment to the MP for getting prettier CSS temporarily, but otherwise, this looks fine. r=me
<gmb> deryck: Thanks,.
<deryck> gmb: np
<gmb> deryck: Thank you for explaining that godawful logger
<gmb> I spent ages trying to work that out
<gmb> (and then I just maximized firebug and logged directly to the JS console :))
<deryck> gmb: heh, np.  It's not documented anywhere as far as I know.
<deryck> it is awful, and I hope to fix something up for it long term.
* bac changed the topic of #launchpad-reviews to: On call: bac  || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<henninge> where did salgado go?
<henninge> EdwinGrubbs: Are you up for a UI review?
<EdwinGrubbs> henninge: I can do it in a little while. Isn't there a UI mentee that needs some more practice?
<henninge> EdwinGrubbs: oh, you have graduated? Missed that. ;)
 * henninge updates wiki page
<henninge> Congrats!
<EdwinGrubbs> henninge: actually, I don't think I have graduated, but I also don't have a mentor.
 * henninge reverts
<henninge> EdwinGrubbs: I could do that for you, if you want.
<henninge> Anyway, that does not help in this case.
<EdwinGrubbs> henninge:  I can start on it in about 30 minutes.
<henninge> EdwinGrubbs: ok, let me prepare the mp.
<danilos> bac, one more up: https://code.launchpad.net/~danilo/launchpad/bug-676011/+merge/44066 :)
* danilos changed the topic of #launchpad-reviews to: On call: bac  || Reviewing: - || queue: [danilos] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<bac> danilos: looks good
<danilos> bac, thanks
* bac changed the topic of #launchpad-reviews to: On call: bac  || Reviewing: - || queue: [-] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
 * bac -> lunch
* sinzui changed the topic of #launchpad-reviews to: On call: bac  || Reviewing: - || queue: [sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<henninge> EdwinGrubbs: https://code.edge.launchpad.net/~henninge/launchpad/devel-bugjamming-1/+merge/44070
 * EdwinGrubbs looks
<henninge> EdwinGrubbs: please look for the implementation details to see the screen shots.
<henninge> sinzui: are you able to mentor Edwin's ui review?
* henninge changed the topic of #launchpad-reviews to: On call: bac  || Reviewing: - || queue: [sinzui, henninge] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<sinzui> henninge, I can, but I am off to a hair cut for lunch. Can you wait 90 minutes?
<henninge> sinzui: I will be off, too. I will look at the replies later tonight.
<henninge> EdwinGrubbs: please forward to sinzui for mentoring.
<EdwinGrubbs> ok
* danilos changed the topic of #launchpad-reviews to: On call: bac  || Reviewing: - || queue: [sinzui, henninge, danilo] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<danilos> (I've put up another very simple branch up for review which solves an OOPS that we've seen 857 times yesterday because of the 'rosetta' product being removed)
<danilos> 6 lines changed total, anybody up for a review?
<bac> danilos:  yep
<danilos> bac, it's up at https://code.launchpad.net/~danilo/launchpad/bug-691634/+merge/44077
<bac> 180 OOPS/line -- pretty goo
<danilos> bac, yeah :)
<danilos> bac, I've also filed a separate bug 691646 to get rid of the 'rosetta' product from the sampledata (including test sampledata) to see if something else breaks as well when we mirror our production state today
<_mup_> Bug #691646: Remove 'rosetta' product from the sampledata <lp-translations> <tech-debt> <Launchpad itself:Triaged> < https://launchpad.net/bugs/691646 >
<bac> thanks danilos
* bac changed the topic of #launchpad-reviews to: On call: bac  || Reviewing: sinzui || queue: [ henninge, danilo] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<bac> sinzui:  your css file has a conflcit
<bac> conflict
* danilos changed the topic of #launchpad-reviews to: On call: bac  || Reviewing: sinzui || queue: [ henninge] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<danilos> bac, ta, cheers
* bac changed the topic of #launchpad-reviews to: On call: bac  || Reviewing: henninge || queue: [ ] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* bac changed the topic of #launchpad-reviews to: On call: bac  || Reviewing: - || queue: [ ] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<flacoste> gary_poster: https://code.edge.launchpad.net/~flacoste/launchpad/bug-688503/+merge/44102
<gary_poster> got it flacoste.
<flacoste> sorry about the wrapping
<flacoste> seems that my editor wraps a little after the code page :-/
<flacoste> i should probably turn off auto-wrapping with lp-propose
<flacoste> and i'm missing the special body plugin
<gary_poster> flacoste, do you already know what signal we use for log rotation?  I was going to try and investigate, but if you know, there's no need
<gary_poster> eh, maybe sigusr1 :-P
<gary_poster> sigusr2
<gary_poster> anyway, no conflicts, which was all I was worried about
<flacoste> gary_poster: right, i checked for existing signals
<flacoste> that's also a reason i split this into two modules
<flacoste> sighup and haproxy
<flacoste> since we already have a sigusr1 and sigusr2 modules
<gary_poster> gotcha
<gary_poster> flacoste: approved.  You could update the PPR heading, but I don't think it matters much.  You need me to QA this next week?
<flacoste> gary_poster: yes, please
<gary_poster> coool, will do
<flacoste> thanks and enjoy your week-end!
<gary_poster> thanks :-) have a great vacation
<flacoste> thank you
* bac changed the topic of #launchpad-reviews to: On call: -  || Reviewing: - || queue: [ ] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<bac> and that concludes my OCR duties for 2010.  have a great weekend.
#launchpad-reviews 2010-12-19
<wallyworld_> thumper: can you approve thus one
<wallyworld_> https://code.edge.launchpad.net/~wallyworld/launchpad/recipe-find-related-branches/+merge/42097
<wgrant> https://code.launchpad.net/~wgrant/launchpad/no-lucilleconfig-distroseries/+merge/44178 (+86/-477) would like a review, if someone is around.
<thumper> both done
<wgrant> thumper: Thanks.
