#launchpad-reviews 2009-11-10
* gmb changed the topic of #launchpad-reviews to: on call: gmb || reviewing: sinzui || queue [sinzui] || This channel is logged: http://irclogs.ubuntu.com
* bac changed the topic of #launchpad-reviews to: on call: gmb, bac || reviewing: sinzui || queue [sinzui] || This channel is logged: http://irclogs.ubuntu.com
* gmb changed the topic of #launchpad-reviews to: on call: gmb || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com
* gmb changed the topic of #launchpad-reviews to: on call: gmb || reviewing: lunch || queue [] || This channel is logged: http://irclogs.ubuntu.com
* bac changed the topic of #launchpad-reviews to: on call: gmb,bac || reviewing: lunch || queue [] || This channel is logged: http://irclogs.ubuntu.com
<al-maisan_> stub: I'd appreciate it if you could look at https://code.edge.launchpad.net/~al-maisan/launchpad/builddsc-478919/+merge/14689 at your earliest convenience.
<al-maisan_> stub: this patch should be much more readable as I promised the last time ;)
<stub> :-)
<stub> al-maisan_: Will there be BuildQueue records that reference a Job that does not have a corresponding BuildPackageJob record?
<al-maisan_> stub: no, they always come in pairs
<al-maisan_> Job and BuildPackageJob always come in pairs that is
<stub> It is probably better to link BuildQueue to BuildPackageJob then rather than directly to Job. That will enforce that better.
<al-maisan_> stub: the issue here is that there will be other types of jobs (in addition to BuildPackageJob) later
<stub> Ok. So in the future a BuildQueue record may be linked to a Job without a BuildPackageJob.
<al-maisan_> that's why BuildQueue refers to Job as opposed to some of these specific rows
<al-maisan_> stub: there will be e.g. a combination of Job and BuildFromBranchJob
<al-maisan_> .. or of Job and RosettaJob
<al-maisan_> hance BuildQueue refers to Job and not to any of: BuildPackageJob, BuildFromBranchJob or RosettaJob
<al-maisan_> directly
<stub> RosettaJob sounds like a dodgy table name, but we can worry about that when it is suggested. Patch looks good.
<al-maisan_> stub: 'RosettaJob' was just an example. Thank you for reviewing it!
* gmb changed the topic of #launchpad-reviews to: on call: gmb || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com
* bac changed the topic of #launchpad-reviews to: on call: gmb,bac || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com
<leonardr> gmb or bac, i have a trivial branch for you: https://code.edge.launchpad.net/~leonardr/lazr.restful/474522-unmarshall-uri/+merge/14691
<bac> leonardr: i can take it
* bac changed the topic of #launchpad-reviews to: on call: gmb,bac || reviewing: -,leonardr || queue [] || This channel is logged: http://irclogs.ubuntu.com
<bac> leonardr: done.  thanks.
* bac changed the topic of #launchpad-reviews to: on call: gmb,bac || reviewing: -,- || queue [] || This channel is logged: http://irclogs.ubuntu.com
<allenap> gmb: Fancy a review? :) https://code.edge.launchpad.net/~allenap/launchpad/ec2-parry/+merge/14693
<allenap> gmb: There are 4 diffs in that review. If you could do one of them I'd be mucho grateful.
<gmb> allenap: Sure, I'll take one.
<gmb> allenap: Wouldn't it be easier to use bzr pipeline to split it up into different branches, or is the change pretty much atomic?
<gmb> Anyway, I'll take the first diff to start with.
* gmb changed the topic of #launchpad-reviews to: on call: gmb, bac || reviewing: allenap-1, - || queue [allenap-2, allenap-3, allenap-4] || This channel is logged: http://irclogs.ubuntu.com
<allenap> gmb: I probably should. I'm happy to do it if you want?
<gmb> allenap: Might be worth giving it a shot, otherwise the m-p is going to get confusing.
<allenap> gmb: The first diff is the biggest, the most complex, but probably the most interesting.
<gmb> I'll keep looking over it, then :)
<allenap> gmb: I'll split it up too. Worth learning if nothing else (I used the predecessor to pipeline once and got very very confused).
<gmb> Didn't we all?
<gmb> allenap: Your XXX about permissiveness should have a bug filed for it.
<allenap> gmb: Ah yes... there are a lot of XXXs in there. I shall file bugs for all of them.
<gmb> Cool.
* sinzui changed the topic of #launchpad-reviews to: on call: gmb, bac || reviewing: allenap-1, - || queue [allenap-2, allenap-3, allenap-4, sinzui] || This channel is logged: http://irclogs.ubuntu.com
<gmb> allenap: Your first diff looks good. Once you've split it up, request a review from me and I'll approve it.
<allenap> gmb: Cool, thanks :)
<gmb> bac: I've got to go afk for a while to pick my missus up from work. I'll come back online later and finish my review shift then, if you haven't powered through them all ;)
* gmb changed the topic of #launchpad-reviews to: on call: bac || reviewing: -, - || queue [allenap-2, allenap-3, allenap-4, sinzui] || This channel is logged: http://irclogs.ubuntu.com
 * gmb -> exeunt
<bac> gmb:  great
<leonardr> bac,can you take a look at this trivial backport of my branch which you reviewed earlier?
<leonardr> https://code.edge.launchpad.net/~leonardr/lazr.restful/474522-oops-backport/+merge/14696
<bac> sinzui: when you get a chance: https://code.edge.launchpad.net/~bac/launchpad/bug-476833/+merge/14697
<bac> leonardr: sure
<sinzui> bac: I start it in a few minutes
<bac> sinzui: no rush.  i'm going to lunch soon
<bac> leonardr: done
<leonardr> great
* bac changed the topic of #launchpad-reviews to: on call: bac-lunch || reviewing: -, - || queue [allenap-2, allenap-3, allenap-4, sinzui] || This channel is logged: http://irclogs.ubuntu.com
* bigjools changed the topic of #launchpad-reviews to: on call: bac-lunch || reviewing: -, - || queue [allenap-2, allenap-3, allenap-4, sinzui, bigjools (CP candidate)] || This channel is logged: http://irclogs.ubuntu.com
* bac changed the topic of #launchpad-reviews to: on call: bac || reviewing: -, - || queue [allenap-2, allenap-3, allenap-4, sinzui, bigjools (CP candidate)] || This channel is logged: http://irclogs.ubuntu.com
* bac changed the topic of #launchpad-reviews to: on call: bac || reviewing: - || queue [allenap-2, allenap-3, allenap-4, sinzui, bigjools (CP candidate)] || This channel is logged: http://irclogs.ubuntu.com
* bac changed the topic of #launchpad-reviews to: on call: bac || reviewing: bigjools || queue [allenap-2, allenap-3, allenap-4, sinzui] || This channel is logged: http://irclogs.ubuntu.com
<bac> hi bigjools
<bigjools> hi bac
<bac> bigjools: just looking at your branch
<bigjools> great
<bac> bigjools: if you change line 85 to be
<bac> for build in builds_list{:num_builds]
<bac> you can get rid of the break logic
<bigjools> true
<bac> also, what about exporting the magic number of 80 to a symbol, since you say it will likely change in the future
<bigjools> bac: it will almost certainly change - to a completely different algorithm
<bac> other than those small suggestions it looks good
<bigjools> this is a stopgap measure
<bigjools> how'd ya like the monster query? :)
<bac> bigjools: ah, ok.  i just find those hard-code values to be cringey
<bac> eek
<bigjools> yeah I know, I didn't really like it either, but I figured since it was going to get removed ...
<bigjools> but I don't mind changing it if you prefer
<bac> bigjools: just do it if you have to touch this again.  then you'll appreciate it.
<bigjools> okay
<bigjools> not a problem
<bac> looks good.  let me do it up official
<bigjools> ok, thanks bac, I'll land it later tonight my time, I gotta go now
<bac> ok
* bac changed the topic of #launchpad-reviews to: on call: bac || reviewing: sinzui || queue [allenap-2, allenap-3, allenap-4] || This channel is logged: http://irclogs.ubuntu.com
<bac> sinzui: this is the branch you queued up:  https://code.edge.launchpad.net/~sinzui/launchpad/distroseries-breadcrumb-bug-400643/+merge/14657  ?
<sinzui> bac: yes. Sorry for 500 lines of mechanical changes
<bac> sinzui: np
<sinzui> We have got to get rid of those unused and untested application search options
 * sinzui considers the Christmas holidays to remove the badness
<sinzui> bac: r=me as it is. I have no remarks other than thanks for the comprehensive tests
<bac> sinzui: cool, thanks
<sinzui> rockstar: I have some questions for you at https://code.edge.launchpad.net/~rockstar/launchpad/inline-mp-status/+merge/14700? I think there are some improvements you can make to the code and a have a few questions. I think you can answers my questions quickly
 * rockstar looks
<rockstar> sinzui, okay, so there was some lint and that commented out error handler that were removed after you saw the patch.
<rockstar> The patch should be updated to show that.
<sinzui> fab
<rockstar> sinzui, also, the lazr-js widgets make baby jesus cry because they use ids.  The branch index page currently has this bug, and we'll fix that this week.
 * sinzui nods
<rockstar> sinzui, technically, that "handcrafted" link is tested, since it adds a class that the widget needs to be able to find.  The windmill test then tests it.
<sinzui> So we are certain that enabled works under all conditions and that is the correct text?
<rockstar> sinzui, and I see no reason why this javascript shouldn't be compressed.  It's just that I didn't know about that base-layout-macros.
<rockstar> sinzui, oh, I see what you're saying.  That is tested well in pagetests.
<rockstar> The only thing I changed was adding a class.
<sinzui> rockstar: The list of scripts used to be in main-template. The moved to macros last June
<rockstar> sinzui, okay, so where is base-layout-macros?
<sinzui> Why are we testing something defined in a view in a page test? Are you testing that the link is there and then not there for all scales of permission? Are we uncertain the @require('launchpad.Edit') works?
<sinzui> lib/lp/app/templates/base-layout-macros.pt
<rockstar> sinzui, I'm pretty sure that link is tested properly.  If it's not, it's still out of the scope of this branch.
<sinzui> rockstar: we have a formatter so that we never have to ask these questions.
<sinzui> if the formatter does not work, we should fix it
<rockstar> sinzui, yes, but the formatter does not provide for being able to set ids of the links.  This is a known problem.
<sinzui> for example, what if we change the icon of the link, so we want to change every template that duplicated the instruction
<sinzui> rockstar: that link does not have an id. I am confused
<rockstar> I am confused too.  WTF...
<sinzui> rockstar: the difference between that template and the macro is that macro also guarantees not extra whitespace is generated
<rockstar> sinzui, is that really a problem?
<sinzui> We had s lot of mismatched icons during the 3.0 conversion because they were hard coded.
<sinzui> rockstar: please update this link to the standard. when you use the formatter, you know you do not need to test the icon, the text, or if it is enabled.
<rockstar> sinzui, the formatter doesn't let me set the class of the edit icon, does it?
 * rockstar doesn't see.
<sinzui> rockstar: it sets the <a> link class, then created a <span class="invisible-link">link text</span>
<sinzui> There is no icon, we use sprites
<rockstar> sinzui, yes, but the class on the icon is important to the widget.
<sinzui> okay now we are making progress
<rockstar> So we didn't need the handcrafted link before, but we do now.
<sinzui> Does beuno know that his maoist death march to sprites is being undermined by the Chang Kai Shecks' nationalist lazr.js force
<sinzui> rockstar: I leave the link as it is since as you point out, it is required.
<sinzui> rockstar: I think we are done
<rockstar> sinzui, thanks.
<sinzui> rockstar: r=me
<rockstar> sinzui, I do think we need to re-consider that kind of markup that lazr-js widgets need.
<sinzui> rockstar: I have a branch that fixed some UI issues in blueprints. can you take a look at the QA section in the proposal and the images I added in a comment: https://code.edge.launchpad.net/~sinzui/launchpad/blueprints-ui/+merge/14639
<leonardr> sinzui: can i jump the allenap-ful queue with a tiny branch that just upgrades some launchpad packages?
<leonardr> https://code.edge.launchpad.net/~leonardr/launchpad/launchpadlib-devel/+merge/14705
<sinzui> leonardr: certainly with me. I am not an OCR
 * sinzui looks
<leonardr> argh
<leonardr> the topic scrolled off my screen and so "sinzui" was the first name i saw
<sinzui> np
<leonardr> but you might want to review that anyway since it fixes the problem you were complaining about
<sinzui> indeed I know the issue
<sinzui> I am just waiting for the diff to appear
* bac changed the topic of #launchpad-reviews to: on call: - || reviewing: - || queue [allenap-2, allenap-3, allenap-4] || This channel is logged: http://irclogs.ubuntu.com
<bac> sinzui: are you reviewing leonardr or do you want me to
<sinzui> I will review it
<bac> sinzui: we've spent more time discussing who gets to review it than it'll take.  i'm glad to see it land!
<sinzui> indeed!
<sinzui> leonardr: land your branch
<leonardr> ok
<sinzui> bac: are you still about for a review
<bac> sinzui: we're on our way out the door.
<sinzui> okay
<bac> is it short?
<sinzui> The fix is, I got consumed in by sorting the imports https://code.edge.launchpad.net/~sinzui/launchpad/active-series-sort-bug-464014/+merge/14720
<sinzui> bac I created series like lucid 10.04 and verify we sort it on the DSP page
<bac> sinzui: i'll have a look now
<thumper> sinzui: up to relook at the project page change?
<thumper> sinzui: it has a test now :)
<sinzui> sure
<bac> sinzui: done.  nice.
 * bac EOD & off to dinner
<sinzui> fab: I can make some people happy
<sinzui> thumper: lines 87-91 are like many tests that failed because there is no confirmation or the confirmation is delayed until it is not clear what went wrong. You can fix this and justify the use of a story by completing the story, The owner should not be opening his browser on line 113, if he is not already on the correct page, lines 87-91 failed.
<sinzui> I think you can fix this my switching the order of 102 and 113 and removing the call for owner_browser.open('http://launchpad.dev/fooix')
<thumper> sinzui: I think the reason the browser is opened, is that once the branch is linked, it ends up on the product series page, not the project page
<thumper> sinzui: we could add yet another view that knows to go back to the main page
<sinzui> thumper: not think, we must know. That view should return the user, that is part of the story
<thumper> sinzui: I'm open to suggestions
<sinzui> if you want to test state write a doctest or unittest
<thumper> sinzui: so, what do you suggest for the story?
<thumper> sinzui: write it as it happens
<thumper> sinzui: or change what happens
<sinzui> changing the order will make a simple narrative instead of something from a David Lynch movie
<sinzui> owner seeing nothing, he uses the edit link, he submits the form, he sees it worked
<sinzui> anon visits and sees the information too
<sinzui> thumper: my only remark about the template change is line 149. I expected to see a "sprite" with 'info', then I asked why there isn't a menu link for this? Is there a menu link to browse the source code?
<thumper> probably not
<thumper> there isn't always a link
<sinzui> that is disappointing
<thumper> how do we handle sometimes menu items?
<thumper> are you meaning on the menu for the branch?
<sinzui> thumper the only uses of class="info" are on lists, there are only three of them, and I believe I have a branch that removed two of them
<thumper> hmm
<sinzui> thumper: I was hoping for a menu item so that you did not have to make the sprite
<sinzui> thumper: I see lots of class="sprite info"
<thumper> I can change it
<sinzui> thumper: I think we are done here. fix the link and reorder the test.
<thumper> sinzui: ok
#launchpad-reviews 2009-11-11
* allenap changed the topic of #launchpad-reviews to: on call: allenap || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com
<wgrant> allenap: I have a slightly (~16 lines once you exclude the inherited sample data changes) oversized branch. Can you have a look at it?
<allenap> wgrant: Sure.
<wgrant> allenap: https://code.edge.launchpad.net/~wgrant/launchpad/distroseries-source-format-selection-part1/+merge/14729
<wgrant> I was assuming someone Soyuzy would review it, so if you need more explanation just ask.
<allenap> wgrant: Actually, do you mind asking someone Soyuzy to look at this? I think this needs some domain knowledge to do it justice. bigjools, can you suggest someone to review wgrant's branch?
<bigjools> just me really :/
<bigjools> but I am rather busy
<wgrant> noodles is at lazr-js?
<bigjools> yeah
<bigjools> I'll try and get to it later wgrant
<allenap> bigjools: Is Muharem away?
<wgrant> bigjools: Thanks.
<bigjools> he's not but he's busy too
<bigjools> we've got stuff that needs to be done before Monday or we'll block others
<allenap> bigjools: Okay, thanks. If you get an early impression that the approach is basically sound then I'm happy to review the boring bits, like style, docstrings, etc, if that'll help.
<bigjools> allenap: that would be very useful, thank you
<bigjools> E_notenoughpeopledoingsoyuz :(
<wgrant> Found your fourth musketeer yet?
<bigjools> yes
<bigjools> all will be revealed by the end of the week I hope
<wgrant> Aha.
<bigjools> hmmm my 4 core could do with a faster disk
<danilos> allenap, hi, I've got a CP candidate for review, it's very short :) if you have time, it's up at https://code.edge.launchpad.net/~danilo/launchpad/bug-479385/+merge/14742
<allenap> danilos: Sure :)
<danilos> allenap, thanks :)
<allenap> danilos: r=me.
<danilos> allenap, thanks!
<bigjools> wgrant: I'm going to start reviewing your branch now
<danilos> bigjools, happy to hear you are ignoring anything else in life to unblock "others" :) thanks!
<bigjools> danilos: I don't want the Serbian mafia on my case
<danilos> bigjools, hehe, nobody does :)
<bigjools> allenap: I reviewed wgrant's branch but it would be good if you also did as we agreed :)
<allenap> bigjools: Sure, of course.
<bigjools> allenap: thanks dude
* allenap changed the topic of #launchpad-reviews to: on call: - || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com
<bigjools> allenap: argh, that regex is making my eyes bleed!
<allenap> bigjools: Jolly good :)
<bigjools> jwz got it right
<citrus> regex is a black art.
 * citrus likes regex
<bigjools> citrus me old fruit
<citrus> I am just lurking today
<bigjools> a lurking lemon
<citrus> hoping for an itinerant UI reviewer to approve my Blueprints branch while I have on Answers 
<citrus> I am feeling more like a Lime to day
<bigjools> green fingernail paint?
<citrus> I guess I am very bruised fruit -- Very dark red (Moscow at midnight), purple (Lourve me Lourve me not), Blue (Yoga-ta Get this Blue)
<citrus> Why did type that? no one on this channel knows nail lacquer colours
<jtv> deryck: https://code.launchpad.net/~jtv/lazr-js/bug-412576/+merge/14754
<deryck> jtv, looking now.
<jtv> ...if the branch scanner ever wakes up :)
<jtv> thanks
<deryck> jtv, yeah, just waiting on the diff here, and then will review.
<jtv> deryck: you may want to extend an admonishing punch to your left and front.
<thumper> rockstar: you have two emails from me, any chance you are going to get a chance to reply?
<rockstar> thumper, I need a beuno before I can respond, and he's been a rare sight today.
<thumper> rockstar: hmm.. ok
<rockstar> thumper, I'm not convinced that scrolling the page is going to be something that will make beuno happy.
<rockstar> thumper, worse comes to worse, you can just use a link and a page anchor to have it jump.
<thumper> rockstar: perhaps jumping is the way to go
<thumper> rockstar: I don't care particularly
<thumper> rockstar: I just would like to get it to the diff, not load another file
<rockstar> thumper, I've been wanting to make it some when people click [review] on an mp, it goes down to the form, instead of going to another form.
<thumper> rockstar: so really just after UI guidance there
<thumper> rockstar: beuno wants the form to appear where they click
<rockstar> thumper, have it jump, land it that way, and I'll consult the beuno tonight sometime.
<rockstar> thumper, does he want to overlay?  I'm so sick of FormOverlays...
<thumper> not sure
<thumper> ask him
<rockstar> Okay.
<rockstar> thumper, what's the other email I have from you?  I only see that one.
<thumper> rockstar: it is a review https://code.edge.launchpad.net/~thumper/launchpad/bug-branch-proposal-view/+merge/14736
<rockstar> Ah, okay.
<thumper> I have three more branches in that pipeline
<rockstar> thumper, do you want me to ui AND code review?
<thumper> rockstar: you could do
<thumper> rockstar: it may speed some things up
<rockstar> thumper, I can has screenshot then?
<thumper> hmm...
<thumper> I can't attach a screen shot to the review yet
<thumper> rockstar: yes...
<rockstar> thumper, have you tested this in IE, Safari, Chrome, and Firefox?
 * rockstar kids
<thumper> rockstar: there is no js, just page details
 * thumper stabs rockstar
<rockstar> thumper, I've been working in Windows all day.  You'll have to find a place to stab that I haven't already stabbed.
<rockstar> thumper, I've been working in Windows all day.  You'll have to find a place to stab that I haven't already stabbed.
<thumper> rockstar: awww....
<thumper> how sad
<thumper> :(
<rockstar> I couldn't even check out the lazr-js trunk because it used symlinks.  I had to fix that.  I still can't build just yet.
<thumper> rockstar: the next pipe does the popup diff
<thumper> rockstar: and the one after that changes the format of the diff link
<thumper> rockstar: and the one after that will show the mp details on the bug page (with popup diff)
<rockstar> thumper, I probably can't review all of them.  I still owe intellectronica and urbanape a review.
<thumper> rockstar: sure
<rockstar> thumper, however, deryck and I are about to start hacking on a wizard widget.
<thumper> awesome
<thumper> rockstar: http://penhey.net/~tim/branch.png
<rockstar> thumper, r=me for ui and code, but you'll still need another ui reviewer.
<thumper> rockstar: anyone there?
<rockstar> thumper, well, we're all pretty heads down.  Let me bug someone.
<thumper> ta
<thumper> rockstar: just wave the picture at them
<thumper> rockstar: deryck already has given +1 on jfdi
<rockstar> thumper, deryck's not a ui reviewer.
<thumper> :(
<thumper> who is?
<thumper> is barry?
<thumper> is he there?
<deryck> yeah, sorry, not a UI reviewer.
<thumper> barry: are you around?
<rockstar> thumper, I'll find barry when he gets back.  He's not there right now.
<thumper> rockstar: but barry is at the sprint?
<rockstar> thumper, yes.
<rockstar> thumper, also, it's a national holiday in the US.
<thumper> :(
<barry> plus i have irc open on a vm :)  hi thumper 
<thumper> barry: hi
<thumper> barry: just after a UI approval:  http://penhey.net/~tim/branch.png
<thumper> barry: showing more proposal details on the branch page (and soon the bug page)
<thumper> barry:  https://code.edge.launchpad.net/~thumper/launchpad/bug-branch-proposal-view/+merge/14736
<barry> thumper: looking
<thumper> barry: this is only part of the final product
<thumper> barry: FYI - next pipe adds popup diff viewing on the branch page, then next changes the format of the diff link rendering to be something like:  327 lines (+43/-120) 8 files modified
<barry> thumper: ui*=me dunno how much more i can review atm
<thumper> barry: that's fine, one at a time :)
<thumper> barry: can you add a ui review to the mp?
<beuno> rockstar, I'll go by in a bit and we can go over it
<beuno> I'm in landscape land atm
<rockstar> beuno, yes, I know. You are needed everywhere.
<barry> thumper: done
<thumper> barry: thansk
#launchpad-reviews 2009-11-12
* thumper changed the topic of #launchpad-reviews to: on call: - || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<thumper> jml: CannotUploadToPocket doesn't inherit from exception
<jml> thumper, am I raising it?
<jml> thumper, ahh, I'll fix the docstring
<thumper> ta
<jml> thumper, this code doesn't use exceptions -- I lost an argument.
 * thumper frowns
<thumper> with who?
<jml> thumper, the reviewer of my previous patch in this area :)
<thumper> # XXX: Tests
<jml> yes
<thumper> either fix the XXX style, or remove it
<thumper> also julians XXX date is off by 2 years
<jml> I could add "icle" to the line :)
<jml> thumper, it's not off, it's moved from old code.
<thumper> :(
<jml> I've deleted the XXX: Tests.
<thumper> is there a nicer way to do this? getUtility(IComponentSet)['partner']
<jml> thumper, how do you mean "nicer"?
<thumper> __get_item__ on a utility class seems icky
<thumper> unclean almost
<jml> I'm fairly sure it's the only way right now.
<jml> I think they want to make it an enum.
<jml> yeah, it's the only method with those semantics.
<thumper> omg - makeSourcePackagePublishingHistory
<jml> yeah, neat huh?
<thumper> jml: so where does can_upload_to_archive get used for branches?
<jml> thumper, canonical.launchpad.security.EditBranch calls it, via can_upload_linked_package(person, branch)
<thumper> InvalidPocketForPartnerArchive docstring needs fixing
<stub> If anyone wants staging database updates back, they can help by reviewing https://code.launchpad.net/~stub/launchpad/replication/+merge/14738
<stub> Most of it is a SQL dump of bits of production and can be ignored.
<stub> The rest is trivial
<thumper> stub: done
<jml> thumper, thanks.
<thumper> jml: done the InvalidPocketForPartnerArchive change?
<thumper> jml: if so r=me
<thumper> jml: and I'll update the mp
<jml> thumper, fixed.
<stub> Huh. https://code.edge.launchpad.net/~stub/launchpad/replication/+merge/14738 has approved code reviews but ec2 land doesn't agree
<mwhudson> stub: it checks the merge proposal status
<mwhudson> stub: is it still "needs review" ?
<stub> nup
<thumper> I've had that before too
<thumper> local object cache?
<mwhudson> replication lag?
<mwhudson> (no)
<stub> Only a few seconds lag - normal.
<stub> Where does my cache live?
 * stub finds and nukes his cache
<stub> Now it just fails :-P
<stub> AttributeError: 'Launchpad' object has no attribute 'credentials'
<mwhudson> i think that's code for "the wrong version of launchpadlib"
<stub> Still telling me its unapproved ;-P
<thumper> stub: land manually like the old days :)
* thumper changed the topic of #launchpad-reviews to: on call: - || reviewing: - || queue [https://code.edge.launchpad.net/~thumper/launchpad/diff-tal-change/+merge/14771] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
* thumper changed the topic of #launchpad-reviews to: on call: - || reviewing: - || queue [https://code.edge.launchpad.net/~thumper/launchpad/popup-diff/+merge/14770, https://code.edge.launchpad.net/~thumper/launchpad/diff-tal-change/+merge/14771] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
 * thumper queues up a couple waiting hopefully
* thumper changed the topic of #launchpad-reviews to: on call: - || reviewing: - || queue [https://code.edge.launchpad.net/~thumper/launchpad/popup-diff/+merge/14770, https://code.edge.launchpad.net/~thumper/launchpad/diff-tal-change/+merge/14771, https://code.edge.launchpad.net/~thumper/launchpad/lp-distroseries/+merge/14777] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<thumper> jml: as you were an original author, would you care to look at https://code.edge.launchpad.net/~thumper/launchpad/lp-distroseries/+merge/14777 ?
<jml> thumper, sure.
<thumper> jml: thanks
<thumper> jml: I'm away making dinner though
<thumper> jml: I'll be back later
<jml> thumper, ok. I'll do the review on the web.
<jml> actually, blocked now because of structured time
* thumper changed the topic of #launchpad-reviews to: on call: - || reviewing: - || queue [https://code.edge.launchpad.net/~thumper/launchpad/popup-diff/+merge/14770, https://code.edge.launchpad.net/~thumper/launchpad/diff-tal-change/+merge/14771] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<adeuring> Moin al-maisan_, could you please review this mp: https://code.edge.launchpad.net/~adeuring/launchpad/bug-479331/+merge/14783 ?
<al-maisan_> adeuring: I am very sorry, but I won't be able to review today as announced in the email I sent yesterday.
<adeuring> al-maisan_: np, missed that mail
<al-maisan_> adeuring: thanks for your understanding!
<leonardr> anyone want to review a lazr.restful branch?
<leonardr> https://code.edge.launchpad.net/~leonardr/lazr.restful/latest-version/+merge/14791
<leonardr> flacoste, maybe you want to review my branch, see if the versioning project is getting off to a good start?
<leonardr> https://code.edge.launchpad.net/~leonardr/lazr.restful/latest-version/+merge/14791
<flacoste> leonardr: i'm not really available to review that until next Monday
<leonardr> ok, np, just looking for someone to look at it
<flacoste> leonardr: i can do a post-merge review though if you find another reviewer
<jtv> barry: https://code.edge.launchpad.net/~jtv/lazr-js/bug-480986/+merge/14801
 * thumper looks for reviewers
<jtv> barry: https://code.edge.launchpad.net/~jtv/lazr-js/bug-480986
<jtv> oops, that's the bug.
<jtv> hang on, digging up mp
<jtv> https://code.edge.launchpad.net/~jtv/lazr-js/bug-480986/+merge/14801
<thumper> jtv: hi
<jtv> hey thumper
<thumper> jtv: are you at the lazr-js thing too
 * jtv looks around
 * thumper is alone with no reviewers
<jtv> yup, that's what it looks like
 * jtv looks sympatethetic
<jtv> okay, I mis-typed that.  What do you need?
<thumper> jtv: three reviews :)
<jtv> thumper: I may meet you somewhere in the middle :)
<thumper> jtv: how about just the oops bug fix then
<thumper> jtv: it is easy
<jtv> gimme
<thumper> https://code.edge.launchpad.net/~thumper/launchpad/branch-email-subject-oops/+merge/14780
<thumper> jtv: and short
<jtv> ok
<jtv> it also happens to be yours
<jtv> I thought you were asking help reviewing stuff :)
<thumper> heh
<thumper> no, I need reviews
<jtv> thumper: on the _getSubject in branch.py, what you put in the docstring really belongs in a comment.
<thumper> ok
<thumper> jtv: how about the comment part of the docstring?
<jtv> (And that comment would just rephrase the code, so I'd replace it with a reference to how things go differently in the other implementation)
<jtv> thumper: yes, that'd probably do it...  but where are the invocations?  And is this method in a common base class or something?
<thumper> yes
<thumper> and I found the BranchMailer is a base class for BMPMailer
<thumper> which I had to reinstate base class behaviour in
<jtv> Then the appropriate first line for the docstring might be a "See `BaseClass`."
<thumper> sure
<jtv> o-kayyy.... I don't see any invocations of _getSubject in trunk either.  Is it outside the Code tree or something?
<jtv> ah, things are falling into place now.
<jtv> thumper: I don't think this is the right solution.
<thumper> jtv: why
<thumper> jtv: test proves it :)
<thumper> jtv: lp.services.mail.basemailer
<jtv> Painful as it is, the "subject" argument for BaseMailer is specified as a string for interpolation, not a raw subject line.
<jtv> So if you want a "non-%-safe" string in there, seems to me the proper way of doing it is to insert that string using interpolation into the subject template.
<thumper> jtv: actually I think you may be right :)
 * thumper tweaks
<jtv> well, you asked for a review, not a rubber stamp.  :-P
<jtv> The moral of this story is "pre-imp calls are hard to come by when everyone else but your cat is on sprint."
<rockstar> thumper, what jtv is pointing out as an issue plagues us in other places of the mailer as well.
<rockstar> At least, I have my suspicions.
<jtv1> rockstar: sprints?
<rockstar> jtv, no, the string interpolation issue.
<jtv> ohhhh, _that_
<thumper> jtv: diff updaetd
 * jtv looks anew
<jtv> thumper: looks good, thanks for sticking with me
<thumper> jtv: thanks for taking the time to review
<jtv> I already fixed a JS bug for today, and was working on one I didn't particularly like.  :)
<jtv> thumper: I gave it a review type of "code," so you can "ec2 land" this one
<thumper> w00t
<thumper> have sip working
<thumper> jtv: thanks
<jtv> thumper: just having a quick look at your diffoverlay branch...  your windmill test has an old-style copyright notice at the top, without the "affero" part.
<thumper> jtv: ah
<jtv> Sorry, not the windmill test (some distractions here), the js source
<thumper> jtv: was copied from somewhere else
<jtv> evidently :)
<thumper> :)
<thumper> will fix
<jtv> thumper: also in the JS file, the spinner diff probably doesn't need any text besides the spinner.  The "loading..." might make more sense as an alt text for the spinner image.
<thumper> jtv: ok
<jtv> There's a comment "barr" in there... probably to say "ouch, I'll just have to pray for inspiration for handling this case."
<jtv> (There's some work going on here for unifying lazr-js error handling a bit more, I believe)
<jtv> You'll want to display the error text somewhere, and probably splash a bit of red across the page.
 * jtv goes for coffee
<jtv> (not far)
<jtv> thumper: in line 64 of that diff, I think "make lint" will complain about a missing semicolon.
<jtv> Or maybe line 63, which is where a human (as opposed to "make lint") would expect it.
<jtv> Near the top of that file some blank lines would be nice too
<thumper> jtv: trailing commas are not good in js object defs are they?
<jtv> thumper: no, they're not, and IE will seize each and any instance of them as an excuse to stop working for you
<jtv> "make lint" should catch them though
 * thumper makes lint
<jtv> thumper: a few notes about get_mp_class...
<jtv> I think this is perfectly valid use of classes, but of course it's not particularly well-supported by YUI.
 * thumper pushed lint cleanup
 * thumper reminds himself about that method
<jtv> It'd be useful to have a small comment above get_mp_class explaining what it does & why
<thumper> ah
<thumper> yes
<jtv> Also, constructing regexes is apparently costly, so please hoist that out of the loop.
<jtv> With a comment for the function, the comment for its final line can go afaic
<jtv> thumper: just checked: 2 blank lines between top-level functions, same as we do in python.
<thumper> ok
<rockstar> thumper, so I'm looking at popupdiff, and just FYI, I'm about to put together a branch that organizes our YUI/lazr-js namespaces, because they are a super crazy.
<thumper> rockstar: what is the personal impact of that with me?
 * jtv bows out so rockstar and thumper can work on this one
<thumper> jtv: thanks for the input
<jtv> mp np
<rockstar> thumper, it's just FYI right now, unless you take forever to land this branch, and then you're going to screw me and, I will go crazy.
<thumper> heh
<thumper> rockstar: btw, i have sip working with twinkle and did a conf call with kfogel and mrjazzcat and it was clear with no delay
<rockstar> Er, screw me UP.  I need to remember to add that.
<rockstar> thumper, great.  I'll see if I can get it configured tonight.
<thumper> rockstar: the instructions are on the wiki
<rockstar> thumper, you don't need a DiffOverlay.bindUI function.
<rockstar> thumper, and you never need to make sure to call the superclass bindUI, because it's already called.
<thumper> rockstar: I followed the instructions on the PrettyOverlay page
<rockstar> So lines 20-25 can be deleted.
<thumper> rockstar: it said I had to
<rockstar> thumper, yes, they are out of date.
 * thumper sighs
<rockstar> thumper, the "out of date" is a known issue that we are currently fixing.
 * thumper removes them
 * thumper confirms it still works
<rockstar> thumper, we should also probably remove the calls to Y.log.
<rockstar> What's the comment on line 58?  A placeholder, or an incomplete comment.
 * thumper frowns
<thumper> rockstar: it does need the bindUI
<thumper> rockstar: if I take it out it fails to work
<thumper> rockstar: probably a placeholder originally
 * thumper removed all the Y.log links
<rockstar> thumper, I almost think you should wait on landing this until after yui 3.0 lands.
<thumper> rockstar: diff has updated
<thumper> rockstar: why?
<thumper> rockstar: and when will that be?
<rockstar> thumper, today if flacoste doesn't get distracted...
<thumper> rockstar: well, then it'll have to be after yui 3
<thumper> flacoste: don't get distracted
<thumper> flacoste: I'd love our code to match the online docs :)
<rockstar> thumper, in his defense, it's a 4,000 line diff.
<thumper> rockstar: you should have been around in the early days
<thumper> rockstar: that wasn't too unusual
<rockstar> thumper, did you walk to school in the snow uphill both ways?
<rockstar> :)
<thumper> rockstar: barefoot over broken glass
<rockstar> thumper, I don't think the diff overlay should be created at all if the diff can't be loaded.
<thumper> rockstar: we don't know we can't load it until after we have given them some feedback that the click is doing something
<rockstar> A popup that says "The diff can't be loaded" is probably not an ideal story.
<thumper> we need to give some immediate feedback to the user
<thumper> rockstar: you are right
<thumper> it isn't ideal
<rockstar> Well, yes, we always need to provide feedback.  A spinner is probably better.
<thumper> I'm waiting on good error handling from you guys
<rockstar> I just talked to Bjorn, and we were thinking you should add "spec=lazr-error-handling" in a comment around it, but then I saw how the error handling works, and it'd probably be better to use a spinner.
<rockstar> thumper, ^^
<thumper> rockstar: what do you mean?
<thumper> rockstar: it has a spinner
<thumper> rockstar: I'll add the comment though
<deryck> rockstar, https://code.edge.launchpad.net/~deryck/lazr-js/multiline-fixed-width-412574/+merge/14808
<deryck> rockstar, and thanks :)
<rockstar> thumper, so, the story should be "click the diff link, get a spinner next to where you clicked (or replacing the icon), get the diff, make an overlay and populate it with the diff, show the diff"
<thumper> rockstar: ok, there is no icon to replace
<thumper> rockstar: and how do you show failure?
<thumper> rockstar: perhaps when we have a good error handling story I'll agree with you
<rockstar> thumper, yes, in this case, there is no icon to replace, but you can put a spinner in there, and then, if there's an error, you can redirect to where the diff is without javascript.
<rockstar> Then you have a graceful degradation story as well.
<thumper> hmm..
 * thumper thinks for a while
<rockstar> Our error handling story is atrocious, this we can agree on.  However, we shouldn't be making it any worse.
<thumper> rockstar: I'll look at changing this after lunch
<rockstar> thumper, okay.
<thumper> rockstar: changed 
<thumper> rockstar: pushed, and waiting for the diff to change
<jtv> thumper: we're talking to Virtual Maris
<rockstar> thumper, okay.  I'm not sure if I'll be able to get to it today.
#launchpad-reviews 2009-11-13
<thumper> rockstar: why is maris virtual?
<al-maisan> Hello jml, how are things? Can we have a chat re. the schema patch today?
<jml> al-maisan, hi. later today should be good.
<al-maisan> jml: great! "later today" == ?
<jml> al-maisan, thinking...
<jml> al-maisan, probably 0700.
<al-maisan> jml: your local time i.e. in approx. 2 hours?
<jml> al-maisan, I meant UTC, sorry
<jml> al-maisan, I'm in Australia at a sprint right now.
<jml> brb
<al-maisan> jml: OK, at the top of the hour then.. I see.
<jml> al-maisan, I'll just start reading the thread that Julian started
<jml> al-maisan, so I don't waste your time on the phone :)
<al-maisan> jml: I like having conversations :) but do read the email thread first.
<jml> al-maisan, ok thanks.
<jml> al-maisan, just off to my room to grab my headset. back soon.
 * al-maisan fires up skype
<mwhudson_> al-maisan: can i see your patch?
<al-maisan> mwhudson: just a minute..
<al-maisan> mwhudson: http://pastebin.ubuntu.com/317567/
<jml> al-maisan, everything is ready
<jml> (at last!)
<jml> al-maisan, shall I skype you?
<al-maisan> jml: please.
<jml> al-maisan, I am told that you are unavailable.
<al-maisan> jml: let me skype you then
<jml> sure. fire away.
<al-maisan> jml: I am being told that you went off-line
<al-maisan> only getting your voice mail
<jml> the internet sucks
<al-maisan> always
<al-maisan> +61 7 5665 4450
<al-maisan> all lines busy
<al-maisan> in holding queue
* thumper changed the topic of #launchpad-reviews to: on call: - || reviewing: - || queue [https://code.edge.launchpad.net/~thumper/launchpad/diff-tal-change/+merge/14771] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<jml> al-maisan, hmm
<jml> al-maisan, let me consult others in the room.
<al-maisan> a direct number would also be good
<jml> al-maisan, +61 7 5444 7311
<jml> al-maisan, you'll still need to ask for me @ room 63
<al-maisan> ok
<al-maisan> jml: I've been put through (".. one moment..") but don't hear anything
<al-maisan> phone rings again
<mwhudson> \o/
<al-maisan> :)
<al-maisan> jml: http://pastebin.ubuntu.com/317588/
<al-maisan> jml: could you please approve https://code.edge.launchpad.net/~al-maisan/launchpad/builddsc-478919/+merge/14689 when you get to it?
<jml> al-maisan, will do.
<al-maisan> jml: thank you very much indeed.
* adeuring changed the topic of #launchpad-reviews to: on call: adeuring || reviewing: - || queue [https://code.edge.launchpad.net/~thumper/launchpad/diff-tal-change/+merge/14771] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<danilos> adeuring, hi, do you have time for another review? :)
<adeuring> danilos: sure
<adeuring> danilos: do you mean your mp from yesterday?
<danilos> adeuring, yeah, https://code.edge.launchpad.net/~danilo/launchpad/bug-430702/+merge/14794
<adeuring> danilos: ok, I'll look into it
<danilos> adeuring, thanks
<danilos> adeuring, I am around for any questions
<adeuring> danilos: there is a call client.waitsForElementProperty(..., timeout=800000) in test_documentation_links.py . This value is a bit scary ;)
<danilos> adeuring, ah, that was while testing, thanks for catching it :)
<danilos> adeuring, with new windmill stuff, it's not so easy to stop an investigate what's failing
<adeuring> danilos: OK, other that that, r=me
<danilos> adeuring, thanks!
* sinzui changed the topic of #launchpad-reviews to: on call: adeuring || reviewing: - || queue [sinzui, https://code.edge.launchpad.net/~thumper/launchpad/diff-tal-change/+merge/14771] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<sinzui> adeuring: Do you have time to review https://code.edge.launchpad.net/~sinzui/launchpad/merge-mailing-list-bug-471770/+merge/14806
<adeuring> sinzui: sure
<adeuring> sinzui: r=me; only two spelling questions
<sinzui> adeuring: thanks.
<sinzui> adeuring: Do you have time to do the code review of https://code.edge.launchpad.net/~sinzui/launchpad/ask-a-question-bug-438467/+merge/14766
<adeuring> sinzui: sure. Let me just grab somethine to eat first ;)
<adeuring> sinzui: r=me
<sinzui> thanks
<sinzui> adeuring: I have a another branch that has a lot of UI changes. There are code changes that need review none-the-less. https://code.edge.launchpad.net/~sinzui/launchpad/answers-ui/+merge/14761
<adeuring> sinzui: OK, i'll look at it
* adeuring changed the topic of #launchpad-reviews to: on call: adeuring || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<jtv> adeuring: if you're bored, I've got an oversized one on the queue.  :-)
<adeuring> jtv: hmm, I've seen it... can't promise that I get it finisehed today, but I'll try ;)
* adeuring changed the topic of #launchpad-reviews to:  on call: adeuring || reviewing: jtv || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<jtv> adeuring: that's great, thanks!
* bigjools changed the topic of #launchpad-reviews to:  on call: adeuring || reviewing: jtv || queue [bigjools (CP, again] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<adeuring> jtv: you raise UnexpetecFormData in HasCustomLanguageCodesTraversalMixin. You are checking a plain URL component here, so wouldn't it be better to raise NotFound?
<jtv> adeuring: looking up...
<jtv> adeuring: you're right.
<adeuring> great, that's better for our "zero oops policy" ;)
<jtv> :)
<jtv> Changed.
<adeuring> jtv: and I think you can remove IDistribution.getCustomLanguageCode()
<adeuring> at least the implementation is gone
<jtv> Oh, did I miss that one?  Fixing...
<adeuring> jtv: well, that's really easy to forget ;)
<jtv> running tests
 * adeuring would probably have left more cruft...
<adeuring> jtv: r=me
<jtv> adeuring: yay, thanks!
<adeuring> bigjools: r=me
<bigjools> thanks!
* adeuring changed the topic of #launchpad-reviews to: on call: adeuring || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<bigjools> adeuring: fwiw, there is a bug
<adeuring> bigjools: OK, so scrap my comment ;)
<bigjools> it was a timely reminder
<bigjools> need to fix that bug before the next LTS
* adeuring changed the topic of #launchpad-reviews to:  on call: - || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
#launchpad-reviews 2009-11-14
<deryck> rockstar, https://code.edge.launchpad.net/~deryck/launchpad/dupe-portlet-click-fail/+merge/14875
#launchpad-reviews 2009-11-15
<jml> mwhudson, hello
<thumper> jml: mwhudson is not really feeling well :(
<thumper> jml: where are you?
<thumper> jml: Dallas?
<jml> thumper, I am.
<thumper> rockstar: I need you
 * thumper fires off an ec2 test run
<rockstar> thumper, hi
<rockstar> jml, where are you in the hotel?  At the bar?
<jml> rockstar, very close to it, yet.
<rockstar> jml, "in the vicinity of the bar?"
<jml> rockstar, I'm not leaning on it, downing my sixth for the evening and asking them to play "Pianoman"
<jml> rockstar, but if you look at the TV to the left of the bar and then look down and somehow manage to tear your gaze away from sidnei, you'll see me.
<rockstar> jml, I'm in my hotel room, being anti-social.
<jml> rockstar, most of us in my area are being asocial.
<rockstar> jml, I wasn't sure you were going to be here.  If you have any super mega hacking sessions in the evening this week, please let me know.
<jml> rockstar, will do
<jml> rockstar, I hope I get to have one
<jml> rockstar, I've reviewed some branches already
<jml> but no real hacking yet
<jml> lots of emails
<rockstar> Yea, deryck and I had a pretty productive day yesterday, but we're just passively hacking now, watching Transformers 2.
<jml> an excellent movie to ignore
#launchpad-reviews 2010-11-15
<henninge> Hm, I am not sure if I should be OCR today given that I am to do r-c reviews, too.
<adeuring> goodmoring
* henninge changed the topic of #launchpad-reviews to: On call: henning || Reviewing: - || queue: [] ||  This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* henninge changed the topic of #launchpad-reviews to: On call: henninge || Reviewing: - || queue: [] ||  This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* henninge changed the topic of #launchpad-reviews to: On call: - || Reviewing: - || queue: [] ||  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
<thumper> abentley: https://code.edge.launchpad.net/~thumper/launchpad/recipe-binary-builds/+merge/40686
<thumper> http://people.canonical.com/~tim/recipe-latest-builds.png
* abentley changed the topic of #launchpad-reviews to: On call: abentley || Reviewing: thumper || queue: [] ||  This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<abentley> thumper: It looks like you're showing the start time and estimated start time for binary builds, even though the times for sourcepackagerecipe builds are completion times.  Was this intentional?
<thumper> no... I don't think so
<abentley> Sorry, what do you mean?  "I don't think it was intentional"?  "I don't think I'm showing the start time"?  "I don't think the times for sourcepackagerecipe builds are completion times"?
<thumper> abentley: :)
<thumper> abentley: I wasn't intentionally showing different things
<thumper> I copied the binary build time stuff from their build-index page
<thumper> I hadn't realised that our times were completed times
<abentley> thumper: It wouldn't hurt to change the title of that column from "Time" to "completion time" or something.
<thumper> perhaps we should change ours to be start time too?
<abentley> thumper: I figure that you don't really care when a build will start, you care when it will be done.
<thumper> abentley: well... we are showing the status...
<abentley> Sure, but I don't get the relevance.
<thumper> I'd rather see "started 3 minutes ago" than "in 5 minutes"
<thumper> if you read the status then the time we see things like:
 * thumper skips examples
<thumper> perhaps we should have a build ETA as well
<thumper> do we know that?
<abentley> thumper: Sure.  That's what we show for source package recipe builds.
<thumper> if we can say "started 5 minutes ago (ETA 6 minutes)
<thumper> or something
<abentley> thumper: And then when it completes we go back to showing "started 11 minutes ago"?
<thumper> no...
<thumper> when it's complete we show the finished time
<abentley> thumper: If we're going to show a start and end time all the time, why not just have two columns?
<thumper> we only show "started 11 minutes ago" when it is building
<thumper> we only care about both times when it is actually building
<thumper> before it is building we mostly care about when it will start
<abentley> thumper: But after 6 minutes, it should be finished building.
<thumper> when it's done we care about when it finished
<thumper> abentley: sure
<abentley> Before it is building I mostly care about when it will finish.
<abentley> While it is building I mostly care about when it will finish.
<thumper> and here is part of the problem
<thumper> the expectation that is shown is when the recipe will be finished
<thumper> not the binary build
<thumper> so showing an expected completed time (according to some) should include the binary times
<thumper> ah...
<thumper> the build estimates come from past history?
<abentley> thumper: Sure.
<abentley> thumper: On the first build, we guess 10 minutes.
<thumper> so... what you are saying is we should show ETA to build completion for the binary buids
<abentley> thumper: Yes, I think we should.
<thumper> ok
<abentley> thumper: I'm okay with the idea of adding another column for the estimated (or actual) start time.
<abentley> thumper: I would rather not put both times into the same column, like "started 5 minutes ago (ETA 6 minutes)"
<thumper> hmm...
<thumper> I'll poke it and see how it looks
<abentley> thumper: okay.
<abentley> thumper: Whatever we settle on, I want the times in that column to consistently refer to either the start or the finish.
 * thumper nods
* 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
<abentley> thumper: It looks like you're also deliberately changing sourcepackagerecipe build date display.  I didn't notice this before.
<abentley> thumper: I wanted to reduce the amount of status-based switching that goes on in these TAL templates, but your changes introduce that.
<thumper> yeah...
<thumper> I changed it after messing with the binary buids
<abentley> thumper: I'm still pleased  with the way I did it, and I'd rather we did binary builds that way.
<abentley> thumper: The less logic in the TAL, the better.
#launchpad-reviews 2010-11-16
* wgrant changed the topic of #launchpad-reviews to: On call: - || Reviewing: - || queue: [wgrant] ||  This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jtv> wgrant: I'll trade you if you likeâhttps://code.launchpad.net/~jtv/launchpad/recife-test_translatablemessage-cleanup/+merge/40930
<wgrant> jtv: If only.
<jtv> If only what?
<jtv> Ah
 * wgrant is no reviewer.
<jtv> Right.
<jtv> Forgot that for a moment.
<wgrant> Heh.
<wgrant> Maybe soon.
<jtv> Indeed.
<jtv> Anyone else for a tiny, tiny review?  bac?
* jtv changed the topic of #launchpad-reviews to:  On call: - || Reviewing: - || queue: [wgrant, jtv] ||  This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* jtv changed the topic of #launchpad-reviews to: The topic for #launchpad-reviews is:  On call: - || Reviewing: - || queue: [wgrant] ||  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: [wgrant] ||  This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jtv> danilos: had a flurry of phone calls; getting to your branch now
<danilos> jtv, if you are busy, we can agree on a general direction and I can find another reviewer
<danilos> jtv, basically, I've asked you since you already looked at it :)
<jtv> danilos: why does that sound like "honey if you're too busy to do the dishes, don't worry I can find another wife"?  :-)
<danilos> jtv, heh, because I wouldn't mind you making better progress on updateTranslation killing spree
<jtv> but you already reviewed mine
<danilos> jtv, instead of spending time on this review :)
<danilos> jtv, that one was slightly easier :)
<danilos> jtv, and I also don't mind helping get you unstuck so you can make better pro... :)
<jtv> â¦zac?
<danilos> smthng lk that
<jtv> oh wellâsure, go run off to some other reviewer then.  I just want you to be happy.  I'll have to re-read the last comments anyway though to see where we stand on mixing "intelligent side-sensitivity" with "passing a specific side."
<danilos> jtv, yeah, in short this is what I said: I don't mind removing it, it's mostly useful for tests (where we'll probably have more of these calls, especially in the future)
<danilos> jtv, we don't really need the functionality there fwiw, and with or without it we can get all the relevant messages (iow, there's no need to introduce ignore_diverged parameter)
<jtv> To be clear, I'm not opposed at all to either of the ways this method can be usedâonly saying I think we'd be happier if they were separate calls.
<jtv> Ah, good to know that that's not an issue.
<danilos> jtv, my point is that we'll only use this in one place inside production code, and we won't use auto-detection there
<jtv> Only one place?
<danilos> jtv, so, such a "separate call with auto-detection" should probably be a test helper if it exists
<danilos> jtv, uhm, sorry, a small finite number of places in all of which we won't use auto-detection anyway
<jtv> "O(1)"?  :-)
<danilos> jtv, yes :)
<jtv> But the idea was always to replace get{Current,Imported}TranslationMessage with something that auto-detected, wasn't it?
<jtv> Or are you saying you want to layer that on top of this one?
<jtv> Maybe the auto-detection that's there just made me think that this method was meant to do more than it actually is.
<danilos> jtv, that will be layered on top of this one, especially since you'll always have a pofile and potemplate when you are doing these things, and then potemplate.translation_side is a short type away (and even then, we'll want to figure out the side only once)
<jtv> Ah now I see.
<danilos> jtv, yeah, perhaps :)
<jtv> So then I'd just leave the auto-detection out of this method, yes, and just have a thin wrapper with the auto-detection as a separate thing wherever it's useful (test helper or potmsgset)
<danilos> jtv, right, I don't mind that at all, and I won't even worry about the helper method until it's actually needed
<jtv> Perfect.
<danilos> jtv, and I won't worry you about the review then, cheers :)
<jtv> Thanks.  :-)  I'll be done for the day soon.  I'll land the one you reviewed, and I'm waiting for test results.  I may just have time for that small potmsgset.potemplate fix in the meantime.
* gmb changed the topic of #launchpad-reviews to: On call: gmb || Reviewing: wgrant || 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
<bigjools> gmb: hey, up for an eye-opening soyuz review?  It's onlky 1k lines of diff. :)
<bigjools> actually when I say soyuz - it's really the buildmaster.
<gmb> bigjools: Why not? I've not had my brain dribble out of my ears for a while.
<bigjools> https://code.launchpad.net/~julian-edwards/launchpad/async-file-uploads-bug-662631/+merge/40883
<bigjools> grassy ass
* gmb changed the topic of #launchpad-reviews to: On call: gmb || Reviewing: bigjools || queue: [] ||  This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<bigjools> gmb: it's not that bad actually, most of it's mechanical.  If you don't know Twisted you might need me to explain some stuff.
<gmb> bigjools: Okay. I'll do this in two passes anyway - one for Spelling, punctuation and grammar and one for actual technical issues, so if I've got questions it'll be later on.
<bigjools> alreet
<danilos> gmb, heya, I've got one up for review myself: https://code.launchpad.net/~danilo/launchpad/get-current-translation/+merge/40885
<gmb> danilos: I'll add it to the queue. Might be a while before I get to it though.
* gmb changed the topic of #launchpad-reviews to: On call: gmb || Reviewing: bigjools || queue: [danilos] ||  This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<danilos> gmb, sure, thanks
<gmb> bigjools: Where does the function "method" come from? Is it a twisted thing or something else (I'm looking at the diff rather than the code here).
<gmb> diff line 258, for reference.
<bigjools> gmb: it uses getattr on the same class
<bigjools> and appends the status that the builder returns to 'method'
<gmb> Ah, right.
<gmb> Ta.
<bigjools> gmb: so in that diff, one of the methods is right below
<bigjools> "_handleStatus_OK"
<gmb> bigjools: Yep, with you. Ta.
<jtv> hi gmb!  I've got one for you that ties in with the one you're reviewing now.
<jtv> sorry, with the one you're reviewing _next_.
* jtv changed the topic of #launchpad-reviews to:  On call: gmb || Reviewing: bigjools || queue: [danilos, jtv] ||  This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<gmb> bigjools: So, I've been over the code and I don't really have any questions beyond what I've asked alread. So, since that generally means that I might have missed something, what did you expect questions about?
<gmb> Also, kill me for using "so" at the start of a sentence twice.
<bigjools> gmb: my main concerns are that I didn't cock up the translations stuff
<bigjools> I've run it on dogfood and it worked. but ...
<danilos> jtv, I've glanced over your branch, nice way to limit damage you inflict :)
<bigjools> gmb: I had to change the logic around, the code was a little weird.  It now reads into files on disk instead of memory.
<gmb> bigjools: I'm not convinced that I can answer that question without domain knowledge. Maybe one of the translations team would be able to review that bit for you.
<bigjools> gmb: so if you see anything that looks like a red flag in their code...
<jtv> danilos: this is my chance to return the favor: "it's your fault"  :-P  You should have implemented the new methods earlier.
<bigjools> gmb: I actually think it's better looking at it w/o domain knowledge as too much familiarity can be dangerous, IYKWIM
<gmb> bigjools: Nothing jumps out at me. I'll give it one more pass to be sure.
<danilos> jtv, it is, I admit... but still, changing one updateTranslation call site at a time is probably the way to go
<danilos> jtv, so, good job there :)
<bigjools> gmb: cheers.  It should be just a case of following the flow and making sure it's not going to do anything stupid in different conditions.
<jtv> danilos: thanks!  I'm actually aiming for a conflict with your branch, so as to make it impossible not to remove the XXXâbut was slightly too lazy to check if I put mine in the same place as yours.
<bigjools> gmb: although I'd like to think that you not having many comments means my code's freaking awesome.  Sadly, I've been disproved on that many times.
<jtv> Nice basis for a review process: "if you have nothing nice to say, keep quiet."  :-)
<danilos> jtv, yeah, tests will start failing if we don't merge them the right way (otoh, we can merge them in such a way that your method is before mine when tests won't fail, but we'll notice that soon :)
<danilos> bigjools, your code is freaking, if that's any consolation
<jtv> danilos: I'm not _too_ worried. :)
<bigjools> ho ho!
<gmb> bigjools: Heh. The only bugbear I've got is the repeated use of "d" and "dl" as variable names. But I concede that that's a pretty common pattern with Twisted, so I'm not going to demand that you go and fix them all.
<bigjools> gmb: yeah, I had the same reaction on my first review of a twisted branch, but it's a *very* strong twisted idiom.
<gmb> bigjools: I think the translations stuff looks okay. It's going to be one of them see-if-it-blows-up jobs, I think.
<bigjools> gmb: such is life with the buildd-manager :/
<bigjools> the last cockup happened even though I borrowed production builders on dogfood and ran through a couple of hundred builds.  C'est la vie.
<gmb> bigjools: Yeah. r=me anyway, with one typo to fix.
<danilos> gmb, we are not really looking for any blow-up jobs in translations, thankyouverymuch :)
<gmb> danilos: I'll leave the Translations and Soyuz teams to duke it out on that score.
 * danilos resisted the temptation to put "-up" in parenthesis
<bigjools> gmb: thanks matey.  And I certainly don't want any blow-up jobs from Translations.
<gmb> Arf arf
<gmb> danilos: I'm going to make a cup of coffee and then I'll take a look at your branch.
* gmb changed the topic of #launchpad-reviews to: On call: gmb || Reviewing: - || queue: [danilos, jtv] ||  This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<danilos> gmb, make it strong :)
<gmb> Oh, gods.
<danilos> gmb, j/k
 * bigjools chuckles
<gmb> Thank goodness.
<danilos> gmb, the background is complicated, the branch is pretty straightforward :)
 * gmb sometimes thinks he should have a bottle of "Review whisky"
<gmb> ok
<bigjools> I grabbed some Ardbeg at Gatwick on the way back from Orlando.  It's bloody fantastic.
<jtv> bigjools: then you should be in the mood for my "kill 'em all" branchâI don't think the OCR should do that right now.  :-)
<jtv> 1090 lines removed, no lines added, no lines modified.
<jtv> bigjools: are you game for reviewing that?
<bigjools> jtv: I am heading to lunch and then I have some urgent OOPSes to fix, sorry :(
<jtv> :(
<jtv> Okay, who else..?
<bigjools> jtv: does it *need* reviewing?
<bigjools> land it rs=you
<jtv> henninge, do you want to see 1090 lines of dead code go into our trash bin?
<henninge> jtv: please, please! ;)
<jtv> henninge: thanks!  https://code.launchpad.net/~jtv/launchpad/pre-675426/+merge/40953
<lifeless> bigjools: r=you, not ts
<lifeless> blah, not *rs*
<jtv> bigjools: the problem with that is that you never know _when_ you're making the silly, impossible mistake that anyone (else) would have spotted.
<jtv> At least I don't.
<bigjools> furry muff
<jtv> you're mumblingâ¦ did you grow a beard or is that someone else's curly hair around your lips?
<henninge> jtv: don't we need "remove-obsolete-translations" any more?
<jtv> henninge: if we don't test it, it's broken, so even if we need it we don't have it.
 * jtv had a brief gust of dogmatism there
<danilos> henninge, we need remove-translations-by, but the obsolete one, not really
<danilos> henninge, apart from the fact that it's most likely broken :)
<henninge> Are we sharing amont all Ubuntu series by now?
<henninge> among
<danilos> henninge, yes
<danilos> henninge, for most of things, OOo is still being migrated I think
<henninge> oh right, that long-running script ;)
<jtv> We haven't seen any noticeable drops in TMs lately, so not sure it's still running.
<jtv> (There were some big drops over the past month though)
* gmb changed the topic of #launchpad-reviews to: On call: gmb || Reviewing: danilos || queue: [jtv] ||  This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<henninge> I just remembered that we initially shared  between just two Ubuntu series and only added new ones after that.
<henninge> jtv: other than that, you can remove those lines. r=me
<jtv> henninge: great, thanks!
<danilos> gmb, I'll go fetch something to eat, I'll be back soon if you've got any questions
<gmb> danilos: Okay. going smoothly so far.
<danilos> gmb, cool
<gmb> danilos: r=me. Thanks to jtv for taking a look first; I wouldn't have picked up on those items :)
* gmb changed the topic of #launchpad-reviews to: On call: gmb || Reviewing: jtv || queue: [] ||  This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jtv> gmb: so you don't eat, drink & dream our specialty?  How odd!  :)
<gmb> jtv: I have nightmares about Soyuz, though.
<danilos> gmb, then you are excused :)
<jtv> gmb: Heh.  I actually have 3 waiting for a vote now.  henninge approved one of them verbally but didn't vote formally.
<danilos> gmb, thanks, btw
<gmb> np
<henninge> jtv: sorry, got distracted by lunch
<gmb> jtv: Okay, so give me a link to the one you want reviewed first.
<jtv> henninge: yes, that can distract.  :)
<jtv> gmb: coming up
<jtv> gmb: this one â https://code.launchpad.net/~jtv/launchpad/recife-kill-updatetranslation-1/+merge/40948
 * gmb looks
<jtv> thanks henninge
<gmb> jtv: Please add some comments or docstrings to the start of your unit test functions. Other than that, r=me.
<jtv> gmb: cool, thanks.  I was trying to be self-explanatory but it's hard to judge that when you're already in the know.
<gmb> Yeah.
<gmb> I usually write the comments first on the basis that I want to read my own code as little as possible.
<gmb> jtv: So, is there a second branch for me to look at?
<jtv> absolutely!  https://code.launchpad.net/~jtv/launchpad/bug-675426/+merge/40958
<gmb> jtv: r=me.
<jtv> splendid, thanks!
* 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
<jtv> That nicely rounds out my evening.
<gmb> np.
* gmb changed the topic of #launchpad-reviews to: On call: gmb || Reviewing: late lunch || queue: [] ||  This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jtv> danilos: I think I accidentally moved a kanban cardâ¦ is your "re-enable global suggestions" still in the right place?
<danilos> jtv, don't know, I'll check
<jtv> sorry for the inconv.
* 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
<flacoste> danilos: shouldn't we make global suggestions a feature flag?
<danilos> flacoste, we should!
<danilos> flacoste, I already had it in the back of my mind (it basically is a feature flag, just implemented through a config option)
<danilos> flacoste, unfortunately, there's no room in our backlog
* 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: :
* 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: - || Reviewing: - || queue: [] ||  This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
 * gmb fails at IRC
* You're now known as ubuntulog
* You're now known as ubuntulog_
* You're now known as ubuntulog
* jtv changed the topic of #launchpad-reviews to: On call: - || Reviewing: - || queue: [jtv] ||  This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* bryceh changed the topic of #launchpad-reviews to: On call: - || Reviewing: - || queue: [jtv, bryce] ||  This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
#launchpad-reviews 2010-11-17
* jtv changed the topic of #launchpad-reviews to: On call: jtv || Reviewing: bryce || queue: [jtv] ||  This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* bac changed the topic of #launchpad-reviews to: On call: jtv || Reviewing: bryce, jtv(bac) || queue: [jtv] ||  This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* bac changed the topic of #launchpad-reviews to: On call: jtv || Reviewing: bryce || queue: [jtv] ||  This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<bac> jtv: r=bac
<jtv> thanks bac!
* jtv changed the topic of #launchpad-reviews to: On call: jtv || Reviewing: - || queue: [] ||  This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<stub> jtv: https://code.launchpad.net/~stub/launchpad/bug-675562-readonly-violation/+merge/41034
<jtv> stub: call first, then I'll review.
<jtv> stub: actually, looking alreadyâ¦  would it make sense in the doctest to verify that the store you get in read-only mode does not implement IMasterStore (in addition to the current test that it does implement ISlaveStore)?
<stub> It would be redundant since a store isn't that schizophrenic . I can change the test if you think checking for not IMasterStore is clearer.
<jtv> stub: I'll leave it to youâdepends on how well-established the rule is that master stores don't implement ISlaveStore.
* jtv changed the topic of #launchpad-reviews to: On call: jtv || Reviewing: stub || queue: [] ||  This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jtv> stub: nice branch btw.
<jtv> One other thing: in lib/canonical/launchpad/webapp/adapter.py, the "if/elif" should have an "else" as per our coding standard.
<stub> Hmm... I guess technically ISlaveStore(Foo) could return a master store, and if so, would be violating contract if the result didn't implement the interface.
<stub> Although it doesn't work that way at the moment.
<jtv> stub: ironically, my review got a warning that we're going into read-only mode.  But made it just in time.
* jtv changed the topic of #launchpad-reviews to: On call: jtv || Reviewing: - || queue: [] ||  This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<henninge> bigjools: without know too much about the builder but shouldn't the cowboy be in assessFailuresCount where it disables the builder instead of not assessing the failure countes at all?
<henninge> just as sanity check question
<bigjools> henninge: no, I want that whole method eliminated
<henninge> ok
<bigjools> henninge: because it fails the job itself too
<henninge> I see
<jtv> hi danilos
<danilos> hi jtv
* jtv changed the topic of #launchpad-reviews to: On call: - || Reviewing: - || queue: [] ||  This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* jtv changed the topic of #launchpad-reviews to: On call: - || Reviewing: - || queue: [jtv] ||  This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jtv> EdwinGrubbs: are you not reviewing today?
<jtv> Or jelmer?
<jelmer> jtv: Yes, I am. I figured you were still around and it was quiet.
<jelmer> jtv: I can have a look at your MP.
<jtv> jelmer: I am, but I can't review my own.  :-)  Thanks.
* jelmer changed the topic of #launchpad-reviews to: On call: jelmer || Reviewing: jtv || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<EdwinGrubbs> jtv: my work day hasn't quite started yet.
<jcsackett> jelmer, (or EdwinGrubbs when your day starts): i have an MP to throw on the queue. https://code.edge.launchpad.net/~jcsackett/launchpad/convert-sql-627631-storm/+merge/41011
* jcsackett changed the topic of #launchpad-reviews to: On call: jelmer || Reviewing: jtv || queue: [jcsackett] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jelmer> jcsackett: thanks
<jcsackett> jelmer: shouldn't i be thanking you? :-P
<jelmer> jcsackett: I guess it's a bit premature, if I haven't looked at your MP yet, but I was thanking you for improving lp :-)
<jcsackett> jelmer: aaaah. :-)
<benji> EdwinGrubbs: when do you want me to start doing mentored reviews?  I assume it'll be on days that you are OCR.
<EdwinGrubbs> benji: oh, that's a good idea. I'll put you in the oncall list with me.
* EdwinGrubbs changed the topic of #launchpad-reviews to: On call: jelmer, benji, Edwin || Reviewing: jtv || queue: [jcsackett] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<benji> EdwinGrubbs: I've read over the various reviewer docs and kept an eye on reviews as they've gone through my inbox.  Is there anything else you'd like me to read/do to prepare?
<EdwinGrubbs> benji: well, the reviewers meeting is coming up in 13 minutes in #launchpad-meeting.
<benji> ah, good call
<EdwinGrubbs> benji: one of school of though on doing oncall reviews is to just ask a bunch of questions to verify that the coder's logic is sound as opposed to analyzing the patch in depth for the logic, i.e. there is a precedent for reviewers pestering the coder with questions that aren't though through. This should make it less tiresome to do reviews on parts of the code you are less familiar with.
<jtv> wow, 3 ocrs
<jelmer> well, one is *
<EdwinGrubbs> benji: can you review https://code.edge.launchpad.net/~jcsackett/launchpad/convert-sql-627631-storm/+merge/41011
<benji> EdwinGrubbs: sure!
<EdwinGrubbs> benji: make sure to move jcsacket from the queue to reviewing in the channel topic, and click "Claim review" on the mp
<benji> k
* benji changed the topic of #launchpad-reviews to: On call: jelmer, benji, Edwin || Reviewing: jtv, jcsackett || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<bigjools> hello, can I have a review of this please: https://code.launchpad.net/~julian-edwards/launchpad/disable-failures-bm-bug-676479/+merge/41068
<jelmer> bigjools: I'll do a review before I EOD
<benji> EdwinGrubbs: I've done my review: https://code.edge.launchpad.net/~jcsackett/launchpad/convert-sql-627631-storm/+merge/41011.
<bigjools> thanks jelmer
<EdwinGrubbs> benji-lunch: when you review a branch as a mentee, you should set the review type to "code*" instead of "code".
<benji> EdwinGrubbs: will do
<EdwinGrubbs> benji: I'm going to lunch now, but I added a comment in the review about a problem that I really wish storm would warn us about. We can discuss it more when I get back.
<benji> cool
* jelmer changed the topic of #launchpad-reviews to: On call: benji, Edwin || Reviewing: jcsackett || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jcsackett> EdwinGrubbs: I fixed the bad query you pointed out.
<jcsackett> changes pushed.
* sinzui changed the topic of #launchpad-reviews to: On call: benji, Edwin || Reviewing: jcsackett || queue: [sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<sinzui> benji, Edwin-lunch I placed my enable-tests-0 branch into the queue
<benji> sinzui: cool, I'll take a look at it momentarily.
 * benji considers writing an irssi plugin for review topic handling.
* benji changed the topic of #launchpad-reviews to: On call: benji, Edwin || Reviewing: jcsackett, sinzui || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jcsackett> benji: if you write that, pass it around. :-)
<benji> :)  will do
<benji> sinzui: I only had one question on https://code.launchpad.net/~sinzui/launchpad/enable-tests-0/+merge/41088.
<benji> EdwinGrubbs: I'v reviewed sinzui's branch at https://code.launchpad.net/~sinzui/launchpad/enable-tests-0/+merge/41088
 * EdwinGrubbs looks
<gary_poster> benji, may I add https://code.launchpad.net/~gary/launchpad/bug676489/+merge/41069 to your queue?  Hopefully quick one, only 75 line diff
<benji> gary_poster: sure
<gary_poster> cool
* gary_poster changed the topic of #launchpad-reviews to: On call: benji, Edwin || Reviewing: jcsackett, sinzui || queue: [gary] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<benji> jcsackett: I commented on https://code.edge.launchpad.net/~jcsackett/launchpad/convert-sql-627631-storm/+merge/41011
* benji changed the topic of #launchpad-reviews to: On call: benji, Edwin || Reviewing: jcsackett, sinzui, gary || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
 * benji requires another Dr. Pepper
<jcsackett> benji: i saw. actually it suggests i got my ec2 results mixed up. i have another branch that's *very* similarly named that was being developed more or less at the same time as this one.
<jcsackett> :-P
<benji> gary_poster: your branch looks good; I did have one small thought which I included in a comment.
<jtv> benji: I couldn't resist one more tryâ¦ this one saves me over a minute of build time.  Wanna review?  https://code.launchpad.net/~jtv/launchpad/parallel-make/+merge/41108
<benji> jtv: sure (by the way, I've figured out why so much option parsing is taking place and I think I can avoid it all)
<jtv> \o/
<jtv> benji: then between the two of us we may just be halving build speed today.
<jtv> Sorry, build _time_ :)
<benji> :)
* benji changed the topic of #launchpad-reviews to: On call: benji, Edwin || Reviewing: gary, jtv || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
 * benji has flashbacks from his days of being knee deep in Makefiles.
<jtv> Fond memories, eh?
<benji> you could say that... I was an absolute expert in make for about six weeks; most of those brain cells have comitted suicide since then.
<jtv> chuckle
<benji> jtv: I commented (approvingly) on your branch: https://code.launchpad.net/~jtv/launchpad/parallel-make/+merge/41108
<jtv> thanks benji!
<benji> EdwinGrubbs: it should be ready for you to review now.
* EdwinGrubbs changed the topic of #launchpad-reviews to: On call: benji, Edwin || Reviewing:  jtv || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<EdwinGrubbs> I'll look at it
<benji> thanks
<gary_poster> many thanks, benji and Edwin-afk
<gary_poster> EdwinGrubbs:
<gary_poster> I meant thank you :-P
<EdwinGrubbs> I really wish I could get freenode to kill Edwin-afk. It has been there for days.
* EdwinGrubbs changed the topic of #launchpad-reviews to: On call: - || Reviewing:  - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
#launchpad-reviews 2010-11-18
<henninge> jtv: How about reviewing the gettext-check-messages branch in the sun?
<henninge> https://code.launchpad.net/~henninge/launchpad/recife-gettext-check-messages/+merge/41155
<jtv> henninge: sun?  It's night here.
<henninge> just barely ... ;)
<jtv> Completely dark.
<jtv> Has been for a while now.
<jtv> And that also means: eod!
<henninge> jtv: you live by the sun? ;)
<jtv> Sort of.  It's past EOD time.
<henninge> oh well, I have to find me another reviewer, then.
<henninge> allenap: are you reviewing today?
<allenap> henninge: Strictly speaking, yes.
* allenap changed the topic of #launchpad-reviews to: On call: allenap || Reviewing:  - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<henninge> allenap: I think this is an easy one ... ;)
<henninge> https://code.launchpad.net/~henninge/launchpad/recife-gettext-check-messages/+merge/41155
* henninge changed the topic of #launchpad-reviews to: On call: allenap || Reviewing:  - || queue: [henninge] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<henninge> allenap: if you would be so nice, please?
<allenap> henninge: Sure :)
<henninge> allenap: I added a comment to give you a little more background.
<henninge> I will be off to a longer lunch soon.
* allenap changed the topic of #launchpad-reviews to: On call: allenap || Reviewing:  - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
 * henninge is back
<danilos> allenap, hi, I've got a nice branch for you :)
* danilos changed the topic of #launchpad-reviews to: On call: allenap || Reviewing:  - || queue: [danilos] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<danilos> MP with the diff up at https://code.launchpad.net/~danilo/launchpad/recife-translation-credits/+merge/41172
* allenap changed the topic of #launchpad-reviews to: On call: allenap || Reviewing: danilos || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* jaycee changed the topic of #launchpad-reviews to: On call: allenap, jcsackett || Reviewing: danilos || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<gmb> jcsackett, allenap: Could I get a review for https://code.launchpad.net/~gmb/launchpad/bug-197775/+merge/41171 please?
<jcsackett> gmb: i'll take it.
<gmb> jcsackett: Manythanks.
* jcsackett changed the topic of #launchpad-reviews to: On call: allenap, jcsackett || Reviewing: danilos, gmb || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<allenap> Hello jcsackett, fellow review dude :)
* EdwinGrubbs changed the topic of #launchpad-reviews to: On call: allenap, jcsackett || Reviewing: danilos, gmb || queue: [Edwin] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<EdwinGrubbs> allenap, jcsackett: I put my branch in the queue: https://code.edge.launchpad.net/~edwin-grubbs/launchpad/bug-667900-dsp-page-upstream-link-form/+merge/41025
<jcsackett> gmb: comments are on your MP.
<jcsackett> gmb: i mentioned it in the comments, but i will also need to ping sinzui to look at this, since i'm currently a trainee.
* jcsackett changed the topic of #launchpad-reviews to: On call: allenap, jcsackett || Reviewing: danilos, Edwin || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<gmb> jcsackett: Thanks.
<jcsackett> gmb: your welcome. :-)
<gmb> jcsackett: I've updated the branch. I'll request the review from sinzui
<gmb> Thanks again.
<jcsackett> gmb: you're welcome. thanks for those changes and grabbing sinzui.
<danilos> allenap, heya, I hope you are not having too much trouble with the review :) if you need any clarifications, I am around for on-call review as well
<allenap> danilos: Sorry, I'm just being very slow and easily distracted :-/
<abentley> allenap, jcsackett: Could you please review https://code.edge.launchpad.net/~abentley/launchpad/find-review/+merge/41195 ?
* jcsackett changed the topic of #launchpad-reviews to: On call: allenap, jcsackett || Reviewing: danilos, Edwin || queue: [abentley] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jcsackett> abentley: you're in the queue. :-)
* allenap changed the topic of #launchpad-reviews to: On call: jcsackett || Reviewing: Edwin || queue: [abentley] || 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: abentley || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* danilos changed the topic of #launchpad-reviews to: On call: jcsackett || Reviewing: abentley || queue: [danilo] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<danilos> jcsackett, hi, when you can get to it, one very simple branch up at https://code.launchpad.net/~danilo/launchpad/invalid-messages/+merge/41209 :)
<abentley> jcsackett: Could you please review https://code.edge.launchpad.net/~abentley/launchpad/builder-limits/+merge/41211 ?
<danilos> abentley, isn't addressable memory used for other things than just actual memory? for instance, mmap files can take up a lot of AS
<danilos> abentley, (I don't know much about RLIMIT_AS, so I am just wondering)
<abentley> danilos: that is interesting, but we don't generally mmap things in bzr, and 1 GB is still huge.  Our example https://code.dogfood.launchpad.net/~abentley/+recipe/test/+build/4803 took 1 hour 28 minutes to fail, so I think there is lots of breathing room.
<danilos> abentley, sure, I can see this is only a limit for recipe builders, but I wonder what'd happen if somebody tried to do things like language pack builds where source package itself is a few hundred megs (probably just like qtwebkit)
<abentley> danilos: Also, our python doesn't provide RLIMIT_VMEM, so we don't have a lot of choice.
<danilos> abentley, (though, this is unrelated to my first comment: I guess we want to fail early, so perhaps it's good anyway)
<danilos> abentley, ok, it sounds good, but I wonder if we have a way to find out if we have been too aggressive
<danilos> abentley, would we just track 'too many builds are getting killed because of this' or should we have something more specific in place?
<abentley> danilos: We have lotsa logs.
<danilos> abentley, I know, but I am sure we don't have a way to track these easily, and that's one thing I suggest: i.e. figure out a way to track these, especially right after it's rolled out
<danilos> abentley, unless you count something like "grep SIGKILL buildd-manager.log" as "easy" :)
<abentley> danilos: I view this as a necessary evil.  The current behaviour is catastrophic: https://wiki.canonical.com/IncidentReports/2010-11-17-buildd-manager-disabling-builders
<danilos> abentley, yes, I agree, I am just thinking a bit more forward into "what if we killed too many builds that would have succeeded"
<danilos> abentley, basically, I'm giving you my r=danilo, as long as we have some strategy in place to figure out that we were not too aggressive
<danilos> abentley, i.e. something that will tell us later that 1GB was the right cut-off point (I trust your judgement in choosing it, it's just that it'd be nice to have a way to confirm it as a good choice later, when we can't do it now)
<abentley> danilos: what would you consider an adequate strategy?
<danilos> abentley, I don't know, a graph tracking number of builds failed because of this particular reason for instance, and a promise to look at it in say week's or two-weeks' time
<abentley> danilos: I don't know how to generate a graph of that.
<abentley> danilos: You'd have to scrape the builder logs.
<danilos> abentley, right, so is there a way to have this fail in a more specific way?
<abentley> danilos: It's conceivable that there might be.
<danilos> abentley, or, alternatively, at least a promise to do a one-time scraping of the logs so we know we haven't cocked up in significant way (if it's too serious we'll know it anyway, but what if we kill something like 15% of the builds that have worked in the past - how will we know?)
<danilos> abentley, it doesn't have to be too formal, except that imo, it needs to happen
<abentley> danilos: Well, a jump in "failed to build" here is a good hint: https://lpstats.canonical.com/graphs/CodeRecipeBuildsDailyStatusCounts/
<danilos> abentley, ok, that's probably good enough if it's regularly tracked, even though it has quite some variation
<abentley> danilos: We can certainly keep our eyes on it after the deployment.
<danilos> abentley, that'd be great then, thanks, r=danilo
<danilos> abentley, btw, can I ask you for a contra-review as well? it's removal of a method and related tests, all "red" :)
<abentley> danilos: okay.
<danilos> abentley, https://code.launchpad.net/~danilo/launchpad/invalid-messages/+merge/41209
<danilos> abentley, the only tricky bit is at the very end, but that was a work-around introduced for the getPOTMsgSetWithErrors tests to pass (we didn't want to worry about it at the time to keep branch size in check, so this is a separate branch for this)
<abentley> danilos: r=me
<danilos> abentley, thanks
<jcsackett> abentley: i apologize for a long delay on this review. "stepping out for lunch" turned into "oh god i have locked myself out of my workspace." :-(
<abentley> jcsackett: Ah.  No worries.
<jcsackett> i have few questions (for my own edification--i don't believe they constitute issues with the branch).
 * benji is amused by jcsackett's misfortune. ;)
<jcsackett> abentley: the aim of this branch was to move the api exposure of getMergeProposal to IBranch, correct?
<jcsackett> (in order to be able to hunt down MPs over the api)
<abentley> jcsackett: the main aim was to expose merged_revno.
<abentley> jcsackett: Other IHasMergeproposal objects will still have getMergeProposal, so it's not "moved", it's extended.
<jcsackett> abentley: ah. okay.
<jcsackett> abentley: some other questions (likely dumb ones--i've never seen much of this code before).
<jcsackett> abentley: the use of patch_collection_return_type is necessary just so the webservice is aware of precisely what collection it's getting?
<abentley> jcsackett: Yes.  Normally, we'd have IBranchMergePoposal in the definition, and we wouldn't need it.  But because interfaces.branchmergeproposal imports interfaces.branch, we can't import interfaces.branchmergeproposal into interfaces.branch.
<jcsackett> ah, which is why the return type decorator on the method says Interface.
<jcsackett> okay, thanks. that makes sense.
<jcsackett> abentley: i see a change to getMergeProposals on GenericBranchCollection to include the merged_revnos argument, but no corresponding change to the IBranchCollection interface it implements. isn't that required?
<abentley> jcsackett: no, the tests pass without it.  It's only required if we have a security-proxied object, and we don't.
<jcsackett> abentley: given the docstring says to refer back to the interface though, isn't awkward to have the interface definition differ?
<jcsackett> is there a reason to not add it?
<abentley> jcsackett: The reason not to add it is that it only makes sense to use when you use it the way Branch uses it.
<jcsackett> abentley: fair.
<EdwinGrubbs> sinzui, jcsackett: here are screenshots of the upstream portlet with the summary. I don't think including the logo is beneficial, and it is more useful to see the PROJECTGROUP=>PROJECT=>SERIES info, which has the icons. https://devpad.canonical.com/~egrubbs/upstream/
<jcsackett> EdwinGrubbs: i like the addition of the summary, though i wonder what it might look like with a really long summary. not sure there's a good solution for that. :-/
<sinzui> EdwinGrubbs, I like the summary. The icon is fine
<jcsackett> i agree with icons over the enormous logo.
<sinzui> jcsackett, the project owner (Us in 1100 cases should set a single sentence for the summary. It is for search listings)
<jcsackett> sinzui: dig.
<sinzui> EdwinGrubbs, you have my r=me
 * sinzui find mp
<EdwinGrubbs> sinzui: thanks
<jcsackett> abentley: r=me. i had some comments on the MP, but they're not blockers in my opinion. since i'm training on this, sinzui will have to follow up. i've already requested a review from him.
<jcsackett> sinzui, to save you time: https://code.edge.launchpad.net/~abentley/launchpad/find-review/+merge/41195
<jcsackett> danilos: abentley reviewed the MP you put up for the queue already, didn't he?
<danilos> jcsackett: that's right, though I'll shortly have another one :P
<abentley> jcsackett: yes.
* 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> danilos: i'll happily review that when it's ready. :-)
<sinzui> abentley, r=me
<abentley> sinzui: thanks.
 * jcsackett is fast learning reviewing really is as much about educating the reviewer as anything.
<allenap> jcsackett: Fancy another learning opportunity? ;)
<allenap> https://code.launchpad.net/~allenap/launchpad/subscribe-to-tag-bug-151129-2/+merge/41186
<abentley> jcsackett: Yes, reviewing is primarily an education exercise.  Frequently in both directions.
* jcsackett changed the topic of #launchpad-reviews to: On call: jcsackett || Reviewing: allenap || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jcsackett> allenap: looking at it now.
* danilos changed the topic of #launchpad-reviews to: On call: jcsackett || Reviewing: allenap || queue: [danilo x 2] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<allenap> jcsackett: Thank you :)
<danilos> jcsackett: damn, allenap beat me to it :) anyway, two very simple branches in the queue
<danilos> first one is https://code.launchpad.net/~danilo/launchpad/bzr-export-pofile-gathering/+merge/41227 and next one is https://code.launchpad.net/~danilo/launchpad/bug-669831/+merge/41228
<allenap> danilos: Sorry dude :)
<jcsackett> danilos: happy to look at 'em. may take a bit, allenap's appears vast. :-P
<danilos> jcsackett: I hope you don't mind me dropping off then? they should be really simple and short (one is ~140 lines with 10 lines of moved code and tests added for that, another is 2 line diff with another test added for it :)
<danilos> or perhaps allenap can help a poor fellow like me who wants to head home :)
<jcsackett> if allenap wants to take a peak, that's cool. if not, it's fine, i'll leave comments on the MP if i have any.
<jcsackett> danilos^
<danilos> jcsackett: cool, that should be fine
<jcsackett> allenap: any reason that you inline import structuralsubscriptionfilter? is there a circular problem you can't work around?
 * danilos wanders off...
<allenap> jcsackett: Yes, it's a circular import problem.
<allenap> I'll have a look at danilio's branches. Missed the pings.
<jcsackett> allenap: ah, thanks.
<jcsackett> for both the answer and looking at danilos's stuff.
<allenap> jcsackett: How's it going? Got any questions? Unfortunately I have to toddle off soon, but if you have any questions later you can ask them via the merge proposal.
<jcsackett> allenap: i'm doing alright. there was a lot of code to read through (those filters). :-)
<jcsackett> i'm about to post to the MP.
<jcsackett> allenap: comments are on MP. :-) sorry for the wait.
* jcsackett changed the topic of #launchpad-reviews to: On call: jcsackett || Reviewing: allenap || 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
<allenap> jcsackett: Cheers, that's awesome :)
* 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-11-19
* 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 changed the topic of #launchpad-reviews to: On call: bac || Reviewing: allenap || 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
<jtv> hi henninge!
<henninge> Hey jtv!
* adeuring changed the topic of #launchpad-reviews to: On call: bac, adeuring || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jtv> adeuring, got one for ya :)  https://code.launchpad.net/~jtv/launchpad/buildfail-recipebuilder/+merge/41280
<adeuring> jtv: I'll look
<jtv> thanks
* adeuring changed the topic of #launchpad-reviews to: On call: bac, adeuring || Reviewing: -, jtv || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<adeuring> jtv: the diff is huge...
<jtv> adeuring: then I MP'ed to db-devel by mistakeâhang on
<adeuring> jtv: it is even another branch ;)
<jtv> oh
<jtv> adeuring: https://code.launchpad.net/~jtv/launchpad/buildfail-recipebuilder/+merge/41282
* bac changed the topic of #launchpad-reviews to: On call: adeuring || Reviewing: -, jtv || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* bac changed the topic of #launchpad-reviews to: On call: adeuring || Reviewing: jtv || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<adeuring> jtv: you mention the method becomeDbUser() in the MP, but II don't see it in the diff...
<jtv> arr
<jtv> hang on
<jtv> adeuring: I pushed the update some time ago, but the diff hasn't updated yet.  :-(
<adeuring> jtv: Ah, I see. I'll use a local "bzr diff --old=<devel>"
<jtv> thanks
<adeuring> jtv: that diff is odd in other ways: contains changes in lib/lp/bugs/browser/bugsubscription.py, for example. But I can't find becomeDbUser()... It also doesn't appear in "bzr diff -r 11943" for the branch. Could it be that a commit is missing?
<adeuring> jtv: gahhh. "bzr pull"
<adeuring> that seems to help...
<jtv> :)
<adeuring> jtv: r=me
<jtv> adeuring: dankeschÃ¶n
<adeuring> welcome :)
* adeuring changed the topic of #launchpad-reviews to: On call: adeuring || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<allenap> adeuring: Fancy doing a mentor review? https://code.launchpad.net/~allenap/launchpad/subscribe-to-tag-bug-151129-2/+merge/41186. I'd value your opinion on the SQL bits especially.
<adeuring> allenap: sure, I'll look at the branch.
<allenap> Cheers.
<adeuring> allenap: r=me
<allenap> adeuring: Thank you, I'll sort out that docstring now.
<adeuring> allenap: thanks!
* benji changed the topic of #launchpad-reviews to: On call: adeuring || Reviewing: - || queue: [benji] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<benji> hi adeuring, I have a small branch that speeds up the WADL generation script ready for review: https://code.edge.launchpad.net/~benji/launchpad/faster-wadl/+merge/41315
<adeuring> benji: I'll look
<benji> thanks
* adeuring changed the topic of #launchpad-reviews to: On call: adeuring || Reviewing: benji || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jcsackett> sinzui: adeuring followed up on the review i did for allenap (https://code.edge.launchpad.net/~allenap/launchpad/subscribe-to-tag-bug-151129-2/+merge/41186). do you still want to follow up for mentor process? allenap has dealt with my quibbles. :-)
<sinzui> jcsackett, no. all it well
<jcsackett> cool beans.
<adeuring> benji: yay for faster "make build", r=me :)
<benji> :)
* adeuring 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
<abentley> flacoste: could you give me an RS on downgrading bzr to 2.2.0? https://code.launchpad.net/~abentley/launchpad/downgrade-bzr/+merge/41356
<flacoste> abentley: done already
<flacoste> abentley: in case, you didn't saw it
<abentley> flacoste: thanks.
