| wgrant | lifeless: Interested in a very challenging +1/-194 review? | 02:50 |
|---|---|---|
| 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:51 |
| wgrant | lifeless: Thanks. | 02:59 |
| StevenK | wgrant: And a 4 digit bug closure! Win! | 05:16 |
| wgrant | StevenK: That was the main purpose of the branch. | 05:22 |
| adeuring | gmb: already around? | 09:26 |
| 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 :) | 09:27 |
| === gmb changed the topic of #launchpad-reviews to: On call: gmb || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| === gmb changed the topic of #launchpad-reviews to: On call: gmb || reviewing: adeuring || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| 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:34 |
| 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! | 10:35 |
| === 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 | ||
| jelmer | happy new year gmb, and thanks for the review! | 11:42 |
| gmb | jelmer: Happy new year; you're welcome :) | 11:43 |
| 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:57 |
| gmb | mrevell: Hello, good morning, thankee, thankee and yes of course. | 11:59 |
| mrevell | ta :) | 11:59 |
| gmb | mrevell: What editor are you using? It appears to be adding DOS line endings (or is that deliberate?) | 12:03 |
| gmb | Actually, they're all over the tour HTML files, by the look of it. | 12:04 |
| gmb | ... okay, no, I'm wrong. Just tour/api so far... | 12:05 |
| 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:08 |
| 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:09 |
| gmb | mrevell: Other than that, r=me. | 12:10 |
| mrevell | gmb, Thanks. I'll fix those line endings. | 12:11 |
| gmb | Cool | 12:12 |
| === matsubara-afk is now known as matsubara | ||
| === gmb changed the topic of #launchpad-reviews to: On call: gmb || reviewing: lunch || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| === mrevell_ is now known as mrevell-lunch | ||
| === gmb changed the topic of #launchpad-reviews to: On call: gmb || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| jcsackett | 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:11 |
| allenap | jcsackett: Okay, cool, I'll go and take a look. | 14:13 |
| jcsackett | allenap: thanks! | 14:13 |
| === mrevell-lunch is now known as mrevell | ||
| 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:16 |
| 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 | 14:17 |
| === salgado is now known as salgado-lunch | ||
| === deryck is now known as deryck[lunch] | ||
| bigjools | mrevell: https://dev.launchpad.net/LEP/DerivativeDistributions updated a bit | 15:50 |
| mrevell | Thanks bigjools | 15:50 |
| Edwin__ | gmb: Can you review this branch? https://code.edge.launchpad.net/~edwin-grubbs/launchpad/bug-682727-bug-count-timeout/+merge/45141 | 16:41 |
| === Edwin__ is now known as EdwinGrubbs | ||
| gmb | EdwinGrubbs: Sure. | 16:42 |
| gmb | EdwinGrubbs: r=me with a couple of minor nitpicks | 16:52 |
| === 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 | ||
| EdwinGrubbs | gmb: thanks | 16:52 |
| === matsubara is now known as matsubara-lunch | ||
| === salgado-lunch is now known as salgado | ||
| === deryck[lunch] is now known as deryck | ||
| === leonardr changed the topic of #launchpad-reviews to: On call: leonardr || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| === gary_poster is now known as gary-lunch | ||
| === benji is now known as benji-lunch | ||
| === gary-lunch is now known as gary_poster | ||
| === benji-lunch is now known as benji | ||
| === mars changed the topic of #launchpad-reviews to: On call: leonardr, mars || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| === benji changed the topic of #launchpad-reviews to: On call: leonardr, mars || reviewing: - || queue: [benji] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| benji | leonardr and mars: you two can thumb wrestle over https://code.edge.launchpad.net/~benji/launchpad/bug-669701/+merge/45171 | 20:39 |
| leonardr | i'll get it | 20:40 |
| leonardr | mars, then maybe you can review a couple small branches of mine | 20:42 |
| leonardr | benji, can you make diff_rules a method of the view instead of a free-floating function? | 20:45 |
| benji | leonardr: I can. | 20:46 |
| leonardr | can you do anything about that lines_of_context = 99999? is there some 'unlimited' constant? | 20:47 |
| benji | leonardr: unfortunately not; it's required to be a number (arithmatic is performed on it) | 20:49 |
| leonardr | ok | 20:49 |
| 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:50 |
| 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:52 |
| 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:53 |
| 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:54 |
| benji | ok, I'll change that (that'll be a hard one for me to remember) | 20:56 |
| leonardr | same futher along in the diff | 20:59 |
| === salgado is now known as salgado-afk | ||
| 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:00 |
| benji | oh, yeah I already changed that one | 21:01 |
| leonardr | ok | 21:01 |
| leonardr | i think that's it | 21:01 |
| benji | cool, thanks | 21:02 |
| === matsubara is now known as matsubara-afk | ||
| benji | leonardr: given that this has a (admin-only) UI component, should it get UI review too? | 21:10 |
| leonardr | benji: yes, they may want to change the way you display the diff or something | 21:11 |
| 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:12 |
| benji | thanks | 21:13 |
| benji | sinzui: do you have a moment for a (hopefully quick) UI review? | 21:13 |
| sinzui | yes | 21:13 |
| 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:14 |
| === leonardr changed the topic of #launchpad-reviews to: On call: leonardr, mars || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| 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:22 |
| 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:23 |
| * 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:24 |
| 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:25 |
| 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:26 |
| 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:27 |
| 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:28 |
| 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:32 |
| sinzui | yes | 21:33 |
| benji | cool, thanks | 21:33 |
| mars | leonardr, still reading the second one, found a note or two so far | 22:10 |
| leonardr | mars, great | 22:16 |
| mars | just finished | 22:17 |
| === gary_poster is now known as gary-afk | ||
| 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:38 |
| leonardr | thanks | 22:39 |
| === leonardr changed the topic of #launchpad-reviews to: On call: mars || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| === mars 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 | ||
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!