[02:42] <rockstar> thumper, care to do a review for me?
[02:43] <rockstar> thumper, https://code.edge.launchpad.net/~rockstar/lazr-js/overlay-width/+merge/33478
[02:43] <thumper> rockstar: my head is down right now, how big is it?
[02:43] <rockstar> thumper, not big, but it adds value...
[02:44] <rockstar> They're merely CSS changes that fix two bugs.
[02:44] <rockstar> Also, it'll make it easier to animate the opening and closing of the overlay.
[02:45]  * thumper looks
[02:46] <thumper> done
[03:22] <thumper> rockstar: you around?
[03:22] <rockstar> thumper, indeed.
[03:22] <thumper> rockstar: I have a very large but boring review
[03:22] <rockstar> thumper, :(
[03:22] <rockstar> thumper, send it on over.  I'll take a look at it in a sec.
[03:22] <thumper> rockstar: it adds layer to our browser zcml
[03:22] <thumper> all of the changes are either adding the layer in the zcml
[03:23] <thumper> or changing tests to show code.lp.dev instead of lp.dev
[03:23] <rockstar> (I'm currently providing marriage counseling for bzr and savannah)
[03:23] <thumper> or adding rootsite= to canonical url sites
[03:23] <thumper> heh
[03:28] <thumper> rockstar: https://code.edge.launchpad.net/~thumper/launchpad/fix-browser-zcml/+merge/33482
[03:30] <mwhudson> 7	 >>> canonical_url(getUtility(IBazaarApplication))
[03:30] <mwhudson> 8	- u'http://launchpad.dev/+code'
[03:30] <mwhudson> 9	+ u'http://code.launchpad.dev/+code'
[03:30] <mwhudson> the +code there is freaking odd really
[03:30] <mwhudson> i wonder if it would actually be better to have the root objects be different for each publication
[03:35] <thumper> mwhudson: probably
[03:35] <thumper> mwhudson: but somewhat out of scope for this change
[03:35] <rockstar> thumper, holy balls that's a big diff...
[03:35] <mwhudson> thumper: yes
[03:36] <thumper> rockstar: but boring
[03:36] <rockstar> thumper, might I make a suggestion for the next time?
[03:36] <thumper> yes
[03:36] <thumper> rockstar: don't do them all at once?
[03:37] <rockstar> thumper, yes, for the love of Odin, yes.
[03:37] <thumper> :)
[03:39] <rockstar> thumper, here's an example: line 1640-1641 of the diff doesn't look to be along the same lines as the rest of the changes in the branch.
[03:40] <rockstar> Actually, 1635-1665...
[03:40] <thumper> rockstar: :)
[03:41] <thumper> yes, I removed a chunk of code that was showing bad data
[03:41] <thumper> the person that wrote it thought it was showing the number of new revisions this month for that branch
[03:41] <thumper> but what it was showing was the number of revisions on all branches this month
[03:42] <rockstar> thumper, the change makes sense.  I just think it would be better to not get lumped in with something that's supposed to be just technical debt.
[03:42] <rockstar> Is there a bug filed for that change?
[03:45] <thumper> no
[03:46] <rockstar> thumper, I think there should be.
[03:46]  * thumper nods
[03:47] <rockstar> thumper, other than that, r=me.
[03:48] <thumper> rockstar: thanks
[03:48] <rockstar> thumper, no prob.
[03:48] <rockstar> thumper, do you think it's a sensible suggestion to break up branches like that into smaller branches?
[03:48] <thumper> rockstar: yeah, probably
[03:49] <thumper> in hindsight I shouldn't have tried to do all at once
[03:51] <rockstar> thumper, do you think a 400 line diff limit would have been reasonable in breaking them up?
[03:52] <rockstar> (I ask because I'm planning on proposing a smaller diff limit in the next reviewers meeting)
[04:15] <thumper> perhaps 500...
[04:45] <lifeless> I don't think smaller diffs are really a great need
[04:46] <lifeless> I think more focused work is great
[04:46] <lifeless> the -big- thing we had in the path was huge entire projects landing
[04:46] <lifeless> s/path/past/
[04:46] <lifeless> ymmv
[07:32] <jtv> stub: care to review a branch for me?  It's a blocking risk, but not large.  https://code.edge.launchpad.net/~jtv/launchpad/recife-pre-resetCurrentTranslation/+merge/33490
[07:35] <stub> jtv: Not a blocking risk yet - nothing is using it apart from tests
[07:35] <jtv> stub: nothing I can _land_ now, no.  Hence: blocking.
[07:36] <stub> You have an XXX that doesn't cite a bug.
[07:36] <stub> There are two ways of fixing that.
[07:36] <jtv> stub: our policy is currently not to file bugs too far in advance.  It's listed in various places though.
[07:37] <stub> Our policy is also no XXX's without cited bugs IIRC. So remove three letters or create a bug I think are the options.
[07:37] <jtv> Okay, okay, I'm removing the XXX.
[07:38] <jtv> Pushing.
[07:40] <stub> jtv: IPOFile doesn't describe the parameters, which is minor but doesn't hurt to mention translator remains unchanged and timestamp is now by default.
[07:40] <jtv> Pushed.
[07:40] <stub> Otherwise all fine.
[07:41] <jtv> stub: thanks.  I'll update the docstring as you say.
[07:42] <jtv> Done.
[07:51] <henninge> jtv: sure, I can ;-)
[07:51] <jtv> ok, mp'ing now...
[07:52] <jtv> henninge: https://code.edge.launchpad.net/~jtv/launchpad/recife-retire-traits-helpers/+merge/33491
[07:57] <henninge> jtv: r=me but please hold back landing until we have that ec2 failure figured out.
[07:58] <jtv> henninge: ok, thanks
[07:59] <jtv> henninge: I did just land another small branch on recife
[08:00] <henninge> oh well ...
[08:28] <jtv> adeuring, maybe you would like to review this very small and hopefully fun branch?  https://code.edge.launchpad.net/~jtv/launchpad/fakelibrarian-commit/+merge/33322
[08:28] <adeuring> jtv: sure, I'll look
[08:28] <lifeless> (I suggested the approach, I think, if that helps)
[08:28] <jtv> adeuring: it allows (some) tests that need the librarian to get by without database commits
[08:29] <jtv> lifeless: I ended up going with the "just slap a method on the fakelibrarian class" approach.  Not exactly future-proof but effective for the time being.
[08:30] <lifeless> jtv: I hear we can change code in the future ;)
[08:30] <jtv> The future's just crazy.
[08:36] <adeuring> jtv: r=me.
[08:37] <jtv> adeuring: great, thanks
[09:02] <StevenK> adeuring: Would you like a review a small branch of mine too?
[09:03] <adeuring> StevenK: If it's a _small_ one, sure ;)
[09:03] <StevenK> adeuring: Is 144 lines too bug?
[09:03] <StevenK> s/bug/big/
[09:03] <adeuring> OK, I'll look at it
[09:22] <StevenK> adeuring: No news is good news? :-)
[09:30] <adeuring> StevenK: yes, r=me
[09:30] <StevenK> adeuring: Thanks!
[09:46] <jtv1> gmb: congratulations Recipient!  Fate has drawn your name.  Today could be the day, dear Recipient, that you review jtv's branch.  Collect your prize here: https://code.edge.launchpad.net/~jtv/launchpad/recife-check-conflicts/+merge/33494
[09:47] <StevenK> Bwahaha
[09:47] <jtv> Don't laugh—if it works for the direct marketers, why shouldn't it work for me?
[09:48] <lifeless> because they try 10K for every bite
[09:48] <lifeless> and if you do that in this channel... it will end badly
[10:18] <gmb> jtv, Sorry, wasn't ignoring your ping; have had to deal with home things. I'll be starting my shift in ~45 minutes, so I'll look then. Don't worry if your day ends before then, though.
[10:18] <jtv> gmb: no worries—actually I wasn't even aware that today was your shift.  :-)
[10:18] <jtv> Just trying someone at random.
[10:18] <jtv> Fate picked another winner, Recipient!
[10:19] <gmb> :)
[10:20]  * gmb goes to finish pre-shift stuff
[11:30] <jtv> gmb: if you had any questions pent up, I'm back
[11:30] <gmb> jtv, No questions; it's r=me :)
[11:31] <jtv> gmb: Thank you, Recipient, for your interest in our product!
[11:31] <gmb> :)
[11:31] <jtv> Hmm… this tack seems to work.  I should do it more often.
[11:32] <gmb> jtv, You need to be careful on whom you try it. See, me, I'm polite and friendly to all comers. But if you ask the wrong person they'll try to /kick you before you get to finish your pitch.
[11:33] <jtv> gmb: I'm not worried… people know me here
[11:34] <jtv> …hey, what happened?
[11:34] <gmb> Heh.
[11:35] <jtv> Okay, maybe I won't try that again then.  :-)
[12:01] <jtv> gmb: if you're not busy, I've got another small one in the backlog here.  Converts a test to use the FakeLibrarian (speeding it up by about 20%).  https://code.edge.launchpad.net/~jtv/launchpad/use-fakelibrarian/+merge/33324
[12:02] <gmb> jtv, Sure
[12:02] <jtv> Thanks Rec^H^H^Hgmb!
[12:10] <gmb> jtv, r=me
[12:10] <jtv> gmb: thanks once more
[12:11] <gmb> np
[13:33] <jtv> gmb, can I chuck a modest one on the queue?
[13:34] <gmb> jtv, Sure. I'll take a look in about 20 minutes
[13:35] <jtv> gmb: splendid
[13:56] <gmb> jtv, Is there any reason that the new pagetest, which isn't really a user story, just a set of tests in doctest form, can't be written as a unittest?
[13:56] <gmb> That would fit better with our policy of not adding stories and doctests where they aren't needed.
[13:56] <jtv> gmb: mostly just laziness on my part tbh… I think you're right.
[13:57]  * jtv twiddles
[13:57] <jtv> gmb: this will take a bit of course
[13:58] <gmb> jtv, No problem. I'll happily re-review when it's done.
[13:58] <jtv> thanks!
[13:58] <gmb> jtv, Other than that it's absolutely fine.
[13:58] <jtv> \o/
[13:59] <jtv> Of course "other than that" is the remaining 2 lines or so of change.  But still, nice to hear.  :)
[14:16] <jtv> gmb: I have the test converted and, in the process, found intimately related "stories" tests that I could move over into the same unit tests.  Pushing.
[14:16] <gmb> jtv, Cool.
[14:17]  * gmb grabs a drink before looking
[14:29] <gmb> jtv, I wouldn't object if you removed the VWS from those tests, but that's a preference thing. r=me either way.
[14:29] <gmb> Nice work :)
[14:29] <jtv> thanks!  <beam>
[14:30] <jtv> I like blank lines to separate setup, execute, verify stages.  But yes, in this case there's not much point.  I'll try it on for size.
[14:31] <jtv> Yup, shorter works better in this case.
[15:04] <leonardr> gmb, i need a follow-up review of https://code.edge.launchpad.net/~leonardr/launchpad/optimized-length-2/+merge/33417 (the latest revision only)
[15:04] <leonardr> it's a little complicated so i'm happy to ask questions
[15:05] <gmb> leonardr, Okay, just OTP at the moment; will look shortly.
[15:05] <leonardr> np
[15:25] <gmb> leonardr, Looking nwo.
[15:26] <gmb> leonardr, So, is it just r11392 that I need to look at?
[15:26] <leonardr> gmb, right
[15:26] <gmb> Righto
[15:29] <gmb> leonardr, Nice XXX explaining the problem.
[15:29] <gmb> leonardr, No further explanations needed; r=me.
[15:30] <leonardr> cool
[15:30] <leonardr> we'll see if the tests pass
[15:38] <EdwinGrubbs> abentley: when you have time, can you review the changes I made to my branch?  https://code.edge.launchpad.net/~edwin-grubbs/launchpad/bug-615655-unicode-oops/+merge/33428
[15:49] <abentley> EdwinGrubbs, I am tempted to think that your binary header handling should still be performed in message_from_string, so that it is easier for others to reuse.
[15:51] <abentley> EdwinGrubbs, btw, 5.5% of the messages in my inbox use "8bit" or "binary" content-transfer-encoding.
[15:53] <abentley> EdwinGrubbs, also, the standard mandates CRLF, not LF or CR as the header separators.  Do you know whether the existing parser treats CR and LF as equivalent to CRLF?
[15:53] <EdwinGrubbs> abentley: is there a different message_from_string() function? I only see the one on the email module.
[15:54] <abentley> EdwinGrubbs, I may be confused.  There's one for creating a SignedMessage from a string, I believe.
[15:56] <EdwinGrubbs> abentley: yes, the email module treats all three different line endings as possibly splitting the header and body.
[15:59] <abentley> EdwinGrubbs, okay, I think it would be nice to provide this as a utility function in lp.services.mail, but I won't require it.  r=me.
[15:59] <EdwinGrubbs> abentley: thanks
[17:04] <jml> simple little patch: https://code.edge.launchpad.net/~jml/launchpad/subject-and-attachment/+merge/33544
[17:09] <jtv> gmb: one more tiiiiny branch?  https://code.edge.launchpad.net/~jtv/launchpad/bug-615673/+merge/33547
[17:11] <gmb> jml, Looking now
[17:11] <gmb> jtv, You're up next.
[17:11] <gmb> First and last customer for the day :)
[17:11] <jtv> cool!
[17:12] <jml> gmb: thanks.
[17:17] <gmb> jml, Approved.
[17:19] <gmb> jtv, r=me.
[17:19]  * gmb is done
[17:19] <jtv> gmb: thanks again.  You have earned your rest.
[18:09] <jml> Another small one: https://code.edge.launchpad.net/~jml/launchpad/testtools-0.9.6/+merge/33556
[18:55] <jml> Yet another small patch: https://code.edge.launchpad.net/~jml/launchpad/launchpad-tester/+merge/33564