=== Ursinha is now known as Ursinha-afk === Ursinha-afk is now known as Ursinha [02:42] thumper, care to do a review for me? [02:43] thumper, https://code.edge.launchpad.net/~rockstar/lazr-js/overlay-width/+merge/33478 [02:43] rockstar: my head is down right now, how big is it? [02:43] thumper, not big, but it adds value... [02:44] They're merely CSS changes that fix two bugs. === Ursinha is now known as Ursinha-afk [02:44] Also, it'll make it easier to animate the opening and closing of the overlay. [02:45] * thumper looks === Ursinha-afk is now known as Ursinha [02:46] done === Ursinha is now known as Ursinha-afk [03:22] rockstar: you around? [03:22] thumper, indeed. [03:22] rockstar: I have a very large but boring review [03:22] thumper, :( [03:22] thumper, send it on over. I'll take a look at it in a sec. [03:22] rockstar: it adds layer to our browser zcml [03:22] all of the changes are either adding the layer in the zcml [03:23] or changing tests to show code.lp.dev instead of lp.dev [03:23] (I'm currently providing marriage counseling for bzr and savannah) [03:23] or adding rootsite= to canonical url sites [03:23] heh [03:28] rockstar: https://code.edge.launchpad.net/~thumper/launchpad/fix-browser-zcml/+merge/33482 [03:30] 7 >>> canonical_url(getUtility(IBazaarApplication)) [03:30] 8 - u'http://launchpad.dev/+code' [03:30] 9 + u'http://code.launchpad.dev/+code' [03:30] the +code there is freaking odd really [03:30] i wonder if it would actually be better to have the root objects be different for each publication [03:35] mwhudson: probably [03:35] mwhudson: but somewhat out of scope for this change [03:35] thumper, holy balls that's a big diff... [03:35] thumper: yes [03:36] rockstar: but boring [03:36] thumper, might I make a suggestion for the next time? [03:36] yes [03:36] rockstar: don't do them all at once? [03:37] thumper, yes, for the love of Odin, yes. [03:37] :) [03:39] 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] Actually, 1635-1665... [03:40] rockstar: :) [03:41] yes, I removed a chunk of code that was showing bad data [03:41] the person that wrote it thought it was showing the number of new revisions this month for that branch [03:41] but what it was showing was the number of revisions on all branches this month [03:42] 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] Is there a bug filed for that change? [03:45] no [03:46] thumper, I think there should be. [03:46] * thumper nods [03:47] thumper, other than that, r=me. [03:48] rockstar: thanks [03:48] thumper, no prob. [03:48] thumper, do you think it's a sensible suggestion to break up branches like that into smaller branches? [03:48] rockstar: yeah, probably [03:49] in hindsight I shouldn't have tried to do all at once [03:51] thumper, do you think a 400 line diff limit would have been reasonable in breaking them up? [03:52] (I ask because I'm planning on proposing a smaller diff limit in the next reviewers meeting) === Ursinha-afk is now known as Ursinha [04:15] perhaps 500... [04:45] I don't think smaller diffs are really a great need [04:46] I think more focused work is great [04:46] the -big- thing we had in the path was huge entire projects landing [04:46] s/path/past/ [04:46] ymmv === Ursinha is now known as Jorjao === Jorjao is now known as Ursinha === Ursinha is now known as Ursinha-afk [07:32] 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] jtv: Not a blocking risk yet - nothing is using it apart from tests [07:35] stub: nothing I can _land_ now, no. Hence: blocking. [07:36] You have an XXX that doesn't cite a bug. [07:36] There are two ways of fixing that. [07:36] stub: our policy is currently not to file bugs too far in advance. It's listed in various places though. [07:37] 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] Okay, okay, I'm removing the XXX. [07:38] Pushing. [07:40] 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] Pushed. [07:40] Otherwise all fine. [07:41] stub: thanks. I'll update the docstring as you say. [07:42] Done. [07:51] jtv: sure, I can ;-) [07:51] ok, mp'ing now... [07:52] henninge: https://code.edge.launchpad.net/~jtv/launchpad/recife-retire-traits-helpers/+merge/33491 [07:57] jtv: r=me but please hold back landing until we have that ec2 failure figured out. [07:58] henninge: ok, thanks [07:59] henninge: I did just land another small branch on recife [08:00] oh well ... [08:28] 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] jtv: sure, I'll look [08:28] (I suggested the approach, I think, if that helps) [08:28] adeuring: it allows (some) tests that need the librarian to get by without database commits [08:29] 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] jtv: I hear we can change code in the future ;) [08:30] The future's just crazy. [08:36] jtv: r=me. [08:37] adeuring: great, thanks [09:02] adeuring: Would you like a review a small branch of mine too? [09:03] StevenK: If it's a _small_ one, sure ;) [09:03] adeuring: Is 144 lines too bug? [09:03] s/bug/big/ [09:03] OK, I'll look at it [09:22] adeuring: No news is good news? :-) [09:30] StevenK: yes, r=me [09:30] adeuring: Thanks! [09:46] 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 === jtv1 is now known as jtv [09:47] Bwahaha [09:47] Don't laugh—if it works for the direct marketers, why shouldn't it work for me? [09:48] because they try 10K for every bite [09:48] and if you do that in this channel... it will end badly === adeuring1 is now known as adeuring [10:18] 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] gmb: no worries—actually I wasn't even aware that today was your shift. :-) [10:18] Just trying someone at random. [10:18] Fate picked another winner, Recipient! [10:19] :) [10:20] * gmb goes to finish pre-shift stuff === 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 [11:30] gmb: if you had any questions pent up, I'm back [11:30] jtv, No questions; it's r=me :) === 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 [11:31] gmb: Thank you, Recipient, for your interest in our product! [11:31] :) [11:31] Hmm… this tack seems to work. I should do it more often. [11:32] 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] gmb: I'm not worried… people know me here [11:34] …hey, what happened? [11:34] Heh. [11:35] Okay, maybe I won't try that again then. :-) [12:01] 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] jtv, Sure === 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 [12:02] Thanks Rec^H^H^Hgmb! [12:10] jtv, r=me === 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 [12:10] gmb: thanks once more [12:11] np === gmb changed the topic of #launchpad-reviews to: On call: gmb || reviewing: vittles || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews === matsubara-afk is now known as matsubara [13:33] gmb, can I chuck a modest one on the queue? [13:34] jtv, Sure. I'll take a look in about 20 minutes [13:35] gmb: splendid === jtv changed the topic of #launchpad-reviews to: On call: gmb || reviewing: vittles || queue: [jtv] || 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: jtv || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [13:56] 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] That would fit better with our policy of not adding stories and doctests where they aren't needed. [13:56] gmb: mostly just laziness on my part tbh… I think you're right. [13:57] * jtv twiddles [13:57] gmb: this will take a bit of course [13:58] jtv, No problem. I'll happily re-review when it's done. [13:58] 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 [13:58] jtv, Other than that it's absolutely fine. [13:58] \o/ [13:59] Of course "other than that" is the remaining 2 lines or so of change. But still, nice to hear. :) [14:16] 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] jtv, Cool. [14:17] * gmb grabs a drink before looking === Ursinha-afk is now known as Ursinha [14:29] 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] Nice work :) [14:29] thanks! [14:30] 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] Yup, shorter works better in this case. [15:04] 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] it's a little complicated so i'm happy to ask questions [15:05] leonardr, Okay, just OTP at the moment; will look shortly. [15:05] np [15:25] leonardr, Looking nwo. [15:26] leonardr, So, is it just r11392 that I need to look at? [15:26] gmb, right [15:26] Righto [15:29] leonardr, Nice XXX explaining the problem. [15:29] leonardr, No further explanations needed; r=me. [15:30] cool [15:30] we'll see if the tests pass [15:38] 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] 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] EdwinGrubbs, btw, 5.5% of the messages in my inbox use "8bit" or "binary" content-transfer-encoding. [15:53] 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] abentley: is there a different message_from_string() function? I only see the one on the email module. [15:54] EdwinGrubbs, I may be confused. There's one for creating a SignedMessage from a string, I believe. [15:56] abentley: yes, the email module treats all three different line endings as possibly splitting the header and body. [15:59] 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] abentley: thanks === Ursinha is now known as Ursinha-lunch === matsubara is now known as matsubara-lunch === benji is now known as benji-lunch [17:04] simple little patch: https://code.edge.launchpad.net/~jml/launchpad/subject-and-attachment/+merge/33544 [17:09] gmb: one more tiiiiny branch? https://code.edge.launchpad.net/~jtv/launchpad/bug-615673/+merge/33547 [17:11] jml, Looking now [17:11] jtv, You're up next. [17:11] First and last customer for the day :) [17:11] cool! === gmb changed the topic of #launchpad-reviews to: On call: gmb || reviewing: jml || queue: [jtv] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [17:12] gmb: thanks. [17:17] jml, Approved. === 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 [17:19] jtv, r=me. [17:19] * gmb is done === 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 [17:19] gmb: thanks again. You have earned your rest. === matsubara-lunch is now known as matsubara === Ursinha-lunch is now known as Ursinha === benji-lunch is now known as benji [18:09] Another small one: https://code.edge.launchpad.net/~jml/launchpad/testtools-0.9.6/+merge/33556 [18:55] Yet another small patch: https://code.edge.launchpad.net/~jml/launchpad/launchpad-tester/+merge/33564 === Edwin is now known as Guest13428 === Guest13428 is now known as EdwinGrubbs === matsubara is now known as matsubara-afk