[02:50] <wgrant> lifeless: Interested in a very challenging +1/-194 review?
[02:51] <lifeless> sure
[02:51] <wgrant> https://code.launchpad.net/~wgrant/launchpad/goodbye-binarypackagereleaseset/+merge/45094
[02:51] <wgrant> Speaking of reviews, I probably want to find a mentor at some point.
[02:59] <wgrant> lifeless: Thanks.
[05:16] <StevenK> wgrant: And a 4 digit bug closure! Win!
[05:22] <wgrant> StevenK: That was the main purpose of the branch.
[09:26] <adeuring> gmb: already around?
[09:27] <gmb> adeuring: Yes. I'll be starting reviews at the top of the hour.
[09:27] <gmb> Happy new year, by the way :)
[09:27] <adeuring> gmb: happy new year to you too :)
[10:34] <gmb> adeuring: I think it's worth refactoring the part of test_user_access_to_private_bug_attachment() that deals with disabling the redirect mechanism in the test HTTP client out into its own method or even a standalone function; it's complex enough that it doesn't really fit in the middle of the test without confusing the reader (well, me, anyway) and we may want to use it elsewhere at some point.
[10:34] <adeuring> gmb: yes, I'll do that. thanks for the suggestion!
[10:35] <gmb> adeuring: Cool. I've got a couple of other comments but nothing major, so r=me with that refactoring.
[10:35] <adeuring> gmb: thanks!
[11:42] <jelmer> happy new year gmb, and thanks for the review!
[11:43] <gmb> jelmer: Happy new year; you're welcome :)
[11:57] <mrevell> Hello there gmb. Happy new year and belated congrats on your first anniversary :) I wonder if you'd fancy looking at a branch with two simple text changes? https://code.launchpad.net/~matthew.revell/launchpad/VAT-update/+merge/45113
[11:59] <gmb> mrevell: Hello, good morning, thankee, thankee and yes of course.
[11:59] <mrevell> ta :)
[12:03] <gmb> mrevell: What editor are you using? It appears to be adding DOS line endings (or is that deliberate?)
[12:04] <gmb> Actually, they're all over the tour HTML files, by the look of it.
[12:05] <gmb> ... okay, no, I'm wrong. Just tour/api so far...
[12:08] <mrevell> gmb, Gedit, although the original HTML for the tour was done by an external contractor on some sort of Mac, I believe. AFAIK I'm preserving whatever the file uses at the time. Perhaps I should strip the DOS line endings.
[12:09] <gmb> mrevell: Yes, I think that would be best. Otherwise, you end up with a situation where we get noise in the diffs just because an editor does something with the line endings.
[12:10] <gmb> mrevell: Other than that, r=me.
[12:11] <mrevell> gmb, Thanks. I'll fix those line endings.
[12:12] <gmb> Cool
[14:11] <jcsackett> allenap: i updated the mp you were looking at before the break.
[14:11] <jcsackett> i had bac take a look at it since i wasn't sure how long you were out for, but i'd still love for you to take a look and make sure i addressed your concerns.
[14:13] <allenap> jcsackett: Okay, cool, I'll go and take a look.
[14:13] <jcsackett> allenap: thanks!
[14:16] <bigjools> mrevell: hi, fancy reviewing a UI mockup?
[14:16] <mrevell> bigjools, I'd love to.
[14:16] <bigjools> mrevell: https://dev.launchpad.net/LEP/DerivativeDistributions
[14:17] <bigjools> https://dev.launchpad.net/LEP/DerivativeDistributions#Mockups
[14:17] <bigjools> in fact
[14:17] <bigjools> see the first 2
[14:17] <mrevell> Thanks bigjools, I'll get onto that now.
[14:17] <bigjools> mrevell: we can have a Grumble if that helps
[15:50] <bigjools> mrevell: https://dev.launchpad.net/LEP/DerivativeDistributions updated a bit
[15:50] <mrevell> Thanks bigjools
[16:41] <Edwin__> gmb: Can you review this branch? https://code.edge.launchpad.net/~edwin-grubbs/launchpad/bug-682727-bug-count-timeout/+merge/45141
[16:42] <gmb> EdwinGrubbs: Sure.
[16:52] <gmb> EdwinGrubbs: r=me with a couple of minor nitpicks
[16:52] <EdwinGrubbs> gmb: thanks
[20:39] <benji> leonardr and mars: you two can thumb wrestle over https://code.edge.launchpad.net/~benji/launchpad/bug-669701/+merge/45171
[20:40] <leonardr> i'll get it
[20:42] <leonardr> mars, then maybe you can review a couple small branches of mine
[20:45] <leonardr> benji, can you make diff_rules a method of the view instead of a free-floating function?
[20:46] <benji> leonardr: I can.
[20:47] <leonardr> can you do anything about that lines_of_context = 99999? is there some 'unlimited' constant?
[20:49] <benji> leonardr: unfortunately not; it's required to be a number (arithmatic is performed on it)
[20:49] <leonardr> ok
[20:50] <benji> re. diff_rules, since it doesn't use self, I assume you want it to be a class method, correct?
[20:50] <benji> er, not a classmethod a staticmethod
[20:52] <leonardr> benji: i don't think it matters much, but i don't know much about the best practices of views
[20:52] <leonardr> maybe mars can weigh in
[20:53] <leonardr> go ahead and make it a static method. the main thing is that it be close to the place that uses it
[20:53] <leonardr> i do think your code in line 150-151 violates our style guide
[20:54] <leonardr> it should be more like
[20:54] <leonardr> browser.getControl(
[20:54] <leonardr>  name="field.feature_rules").value = (
[20:54] <leonardr>   'beta_user some_key 10 another value with spaces')
[20:54] <leonardr> iow the parenthesis should be the last thing on the line, and you should add parentheses to avoid exceeding the line limit
[20:56] <benji> ok, I'll change that (that'll be a hard one for me to remember)
[20:59] <leonardr> same futher along in the diff
[21:00] <benji> leonardr: hmm, I can't find the second one, what is the line number in the diff?
[21:00] <leonardr> benji: around 153?
[21:01] <benji> oh, yeah I already changed that one
[21:01] <leonardr> ok
[21:01] <leonardr> i think that's it
[21:02] <benji> cool, thanks
[21:10] <benji> leonardr: given that this has a (admin-only) UI component, should it get UI review too?
[21:11] <leonardr> benji: yes, they may want to change the way you display the diff or something
[21:12] <benji> leonardr: ok, thanks; do I have to wait until a ui reviewer is on call or is there some other mechanism for getting ui reviews?
[21:12] <leonardr> benji: you should ping a ui reviewer
[21:13] <benji> thanks
[21:13] <benji> sinzui: do you have a moment for a (hopefully quick) UI review?
[21:13] <sinzui> yes
[21:14] <benji> sinzui: great! the MP describes how to see the UI change in question: https://code.edge.launchpad.net/~benji/launchpad/bug-669701/+merge/45171
[21:22] <leonardr> mars, my small branches #1:
[21:22] <leonardr> https://code.launchpad.net/~leonardr/launchpad/launchpadlib-integration/+merge/45174
[21:22] <mars> leonardr, ok
[21:22] <leonardr> oops, that's the wrong mp
[21:22] <leonardr> that's the mp for my launchpadlib branch
[21:22] <leonardr> 1 sec
[21:23] <sinzui> benji. When I use other forms, Lp uses an info notification to tell my my change was applied. I can be hacked in the template as you have done, but it is more commonly added using self.request.response.addNotification
[21:23] <sinzui> Why is this form different
[21:24]  * sinzui expects to see a div with the class="message info" to make it look like other changes
[21:24] <benji> sinzui: I used self.request.response.addNotification originally when I just had the "Your changes have been applied" message, but since I had the diff to include addNotification didn't seem appropriate
[21:25] <benji> and mixing them seemed odd (but I'm glad to do it if that's the status quo)
[21:25] <benji> the class="message info" seems like a good idea, I'll do that
[21:26] <leonardr> mars: ok, the branches are
[21:26] <mars> leonardr, did you push the latest version?  I don't see the "Removed a doctest" bit
[21:26] <leonardr> https://code.launchpad.net/~leonardr/launchpad/launchpadlib-integration/+merge/45174
[21:26] <leonardr> and then https://code.launchpad.net/~leonardr/launchpadlib/launchpad-integration/+merge/45176
[21:27] <sinzui> benji: class="informational message" is correct, but I see you point about oddness.
[21:27]  * sinzui tries to hack it
[21:27] <leonardr> mars: i think you're reading the wrong merge proposal? the one i posted to https://code.launchpad.net/~leonardr/launchpad/launchpadlib-integration/+merge/45174 by mistake?
[21:27] <mars> leonardr, I don't see the "removed a test" bit in the first branch
[21:27] <leonardr> the doctest removal is in launchpadlib, not launchpad
[21:27] <mars> leonardr, same message for both commits?
[21:27] <mars> err, MPs
[21:28] <leonardr> mars: if you reload https://code.launchpad.net/~leonardr/launchpad/launchpadlib-integration/+merge/45174 you'll see a comment with an updated commit
[21:28] <leonardr> s/commit/MP/
[21:28] <mars> ah :)
[21:32] <sinzui> benji: r=me. I do not want to introduce a blue, yellow, red, and green splotch to a page and risk giving a beloved losa  a seizure
[21:32] <benji> sinzui: to be clear: you're approving it without any changes?
[21:33] <sinzui> yes
[21:33] <benji> cool, thanks
[22:10] <mars> leonardr, still reading the second one, found a note or two so far
[22:16] <leonardr> mars, great
[22:17] <mars> just finished
[22:38] <leonardr> mars: i have an unrelated question about ec2 test. i'm getting "aws credentials could not be loaded" on my new computer when i try to do ec2 test. do you know what i need to copy over from my old computer?
[22:38] <mars> leonardr, copy the $HOME/.ec2 directory
[22:39] <leonardr> thanks