| wallyworld | StevenK: ping? | 01:01 |
|---|---|---|
| === _thumper_ is now known as thumper | ||
| lifeless | mwhudson: so, will you be up for reviewing my branch?[no panic, just wanting to know if you're 'tagged' or not] | 02:44 |
| mwhudson | lifeless: looking at it now, yes | 02:44 |
| lifeless | cool, thanks | 02:45 |
| mwhudson | lifeless: um TacTestSetup calls Fixture.setUp() twice | 02:45 |
| lifeless | heh, fortunately thats idempotent. | 02:45 |
| lifeless | fixing | 02:45 |
| mwhudson | lifeless: why is that lib/lp/testing/fakelibrarian.py shim still needed? | 02:52 |
| mwhudson | lifeless: couple more questions/comments on the proposal, but reviewed | 03:00 |
| lifeless | mwhudson: because db-devel has more uses, I suspect | 03:01 |
| mwhudson | lifeless: ah ok | 03:01 |
| lifeless | I seriously doubt that there is just one use in the entire codebase | 03:01 |
| lifeless | IMBW | 03:01 |
| lifeless | the docstring in __init__ is a pep8 variable docstring | 03:02 |
| lifeless | updated the audit bit | 03:03 |
| lifeless | sorry, pep257 | 03:03 |
| lifeless | "String literals occurring immediately after a simple assignment at the top level of a module, class, or __init__ method are called "attribute docstrings"." | 03:03 |
| mwhudson | lifeless: oh right | 03:06 |
| mwhudson | i'm not sure that idea ever really took off... | 03:06 |
| lifeless | I shall carry the flag! | 03:07 |
| lifeless | Until someone (like the friendly pydoctor maintainer) makes it work. | 03:07 |
| mwhudson | heh heh | 03:08 |
| lifeless | seriously, I can use a comment if you'd prefer. | 03:09 |
| lifeless | I do quite like the idea of that being extractable. | 03:09 |
| lifeless | particularly as I don't like documenting _ variables as :ivar: | 03:09 |
| mwhudson | i think perhaps a comment is more appropriate | 03:10 |
| mwhudson | eventually i might get around to the thing i keep meaning to do in pydoctor -- a 'maintainer mode' where _things are not shown by default | 03:10 |
| mwhudson | but you can click something and they appear | 03:10 |
| wallyworld | StevenK: you around today? | 06:26 |
| StevenK | wallyworld: Indeed, what's up? | 06:27 |
| wallyworld | hi, i just need you to do a review as per tim's request. i'll paste the link | 06:28 |
| wallyworld | https://code.edge.launchpad.net/~wallyworld/launchpad/reassign-reviewer-cancel-option/+merge/36671 | 06:28 |
| wallyworld | should be a simple one hopefully :-) | 06:29 |
| wallyworld | thanks in advance | 06:29 |
| StevenK | wallyworld: Looking as soon as Firefox deigns to listen to me | 06:29 |
| wallyworld | :-) | 06:30 |
| StevenK | wallyworld: Done | 06:31 |
| wallyworld | what, already? | 06:31 |
| StevenK | wallyworld: It's a very small branch, as you say | 06:31 |
| wallyworld | :-) | 06:31 |
| wallyworld | thanks | 06:32 |
| wallyworld | StevenK: one question. the mp screen says that my original mp request still needs to be claimed, hence the overall review status is "Pending" | 06:36 |
| wallyworld | who needs to go and and mark it as approved? me or you? | 06:37 |
| StevenK | wallyworld: It's because I can only do 'code*' not 'code' | 06:37 |
| wallyworld | what does code* mean? | 06:37 |
| StevenK | I'm only a reviwer-in-training | 06:37 |
| StevenK | *reviewer-in-training | 06:37 |
| wallyworld | oh, ok. i'll wait for tim tomorrow | 06:37 |
| wallyworld | thanks for the info. i'm still new at this :-) | 06:38 |
| poolie | bac: hi, up yet? | 09:11 |
| poolie | is anyone able to review https://code.edge.launchpad.net/~mbp/launchpad/flags-gui/+merge/36415 | 09:13 |
| jtv | poolie: I'll trade you for https://code.edge.launchpad.net/~jtv/launchpad/henninge_validate_translation-cleanup/+merge/36683 | 09:14 |
| === henninge changed the topic of #launchpad-reviews to: On call: henninge || Reviewing: sinzui || queue: [-] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| poolie | > En passant? | 09:15 |
| poolie | i think your description is incompletely translated :-P | 09:15 |
| poolie | jtv it seems plausible to me but i've never looked at this before | 09:16 |
| jtv | poolie: "In passing." It's commonly used in chess, I believe. | 09:16 |
| poolie | i know | 09:16 |
| jtv | Looks like poolie eod'ed. Can someone else review this simple cleanup for me? https://code.launchpad.net/~jtv/launchpad/henninge_validate_translation-cleanup/+merge/36683 | 09:59 |
| === 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 | ||
| === jtv is now known as jtv-afk | ||
| === jtv-afk is now known as jtv | ||
| jtv | Who's free to review this _easy_ branch for me? The OCR can't do it due to conflict of interest: https://code.launchpad.net/~jtv/launchpad/henninge_validate_translation-cleanup/+merge/36683 | 12:13 |
| henninge | jtv: you might like this one: https://code.edge.launchpad.net/~henninge/launchpad/devel-update-copyright/+merge/36701 | 12:57 |
| * jtv makes a quick guess | 12:58 | |
| * henninge lunches | 12:58 | |
| === henninge changed the topic of #launchpad-reviews to: On call: henninge || Reviewing: lunch || queue: [-] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| jtv | henninge: you're saying that rewriting all our source files just before committing them to mainline does not need testing? :-) | 12:58 |
| jtv | Non-OCR reviewer needed! https://code.launchpad.net/~jtv/launchpad/henninge_validate_translation-cleanup/+merge/36683 | 13:22 |
| === mrevell is now known as mrevell-lunch | ||
| === 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 | ||
| noodles775 | salgado: hi! I've done a new demo based on your ui review (for blacklisting distro series differences): http://people.canonical.com/~michaeln/tmp/644328-review-salgado.ogv | 14:33 |
| noodles775 | Let me know if you've time to take a look. | 14:33 |
| salgado | noodles775, sure, checking | 14:34 |
| noodles775 | Thanks. Just for reference, it's this MP: https://code.edge.launchpad.net/~michael.nelson/launchpad/644328-blacklist-ui/+merge/36442 | 14:34 |
| === mrevell-lunch is now known as mrevell | ||
| noodles775 | salgado: thanks for the review - can you pls add an approve review with the review type as ui*, if you're approving? | 15:11 |
| salgado | noodles775, oh, sure, forgot about that | 15:12 |
| noodles775 | salgado: and I just found that wrapping the radio inputs in an empty form element gets the checked radio displaying too :) | 15:16 |
| salgado | cool! | 15:16 |
| achuni | henninge: hi! have a minute for a one-byte review? :) | 15:25 |
| henninge | achuni: sure, fling it over | 15:26 |
| henninge | the byte ;) | 15:26 |
| achuni | :D | 15:26 |
| achuni | https://code.launchpad.net/~elachuni/launchpad/use-latest-shipit/+merge/36720 | 15:26 |
| henninge | achuni: r=me ;) | 15:27 |
| achuni | henninge: neat, thanks :) | 15:28 |
| === noodles775 changed the topic of #launchpad-reviews to: On call: henninge || Reviewing: - || queue: [noodles775] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| noodles775 | Hi henninge! I'm just updating the windmill test, but if you've time, can you take a look at https://code.edge.launchpad.net/~michael.nelson/launchpad/644328-blacklist-ui/+merge/36442 | 15:32 |
| === abentley changed the topic of #launchpad-reviews to: On call: henninge, abentley || Reviewing: -,- || queue: [noodles775] || 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, abentley || Reviewing: noodles775,- || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| henninge | noodles775: Hi! | 15:46 |
| * henninge was thinking UI review ... ;-) | 15:46 | |
| henninge | noodles775: sure, I can do a code review for you ... | 15:46 |
| noodles775 | henninge: nah, just code, but if you've anything to add to the ui review, feel free :) | 15:47 |
| noodles775 | Thanks! | 15:47 |
| henninge | noodles775: 38+ """A mixin for classes that share some method implementations.""" | 15:48 |
| henninge | Oh, really!? | 15:48 |
| henninge | ;-) | 15:48 |
| henninge | noodles775: I mean, that's what a mixin is all about, is it not? ;) | 15:48 |
| noodles775 | henninge: line 66 :) | 15:48 |
| henninge | ah, hadn't gotten that far ;) | 15:49 |
| noodles775 | (ie. I didn't change it, just moved it, but can change if you'd like) | 15:49 |
| henninge | yes, please | 15:49 |
| henninge | "non-sensical comments are useless" ... | 15:49 |
| noodles775 | Yep, TBH I didn't even read it when moveing the class, but I'll update it. | 15:49 |
| henninge | noodles775: "test_is_ajax" should be two test methods | 15:53 |
| noodles775 | OK, and the same will go for two other tests you'll find later then. | 15:54 |
| noodles775 | (test_show_blacklist_options and test_blacklist_options_initial_values) | 15:54 |
| henninge | noodles775: yup | 15:54 |
| * henninge is a fan of the "one assert per test" idea | 15:55 | |
| noodles775 | yeah, same, but in those cases it seemed a bit overboard, but perhaps not. | 15:55 |
| * noodles775 notices an import he forgot to move to the top too. | 15:55 | |
| === jcsackett changed the topic of #launchpad-reviews to: On call: henninge, abentley || Reviewing: noodles775,- || queue: [jcsackett] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| jcsackett | abentley: you free for a review? | 15:59 |
| abentley | jcsackett: yes. | 15:59 |
| jcsackett | abentley, fantastic! mp is at https://code.edge.launchpad.net/~jcsackett/launchpad/bad-state-transition-641266/+merge/36726 | 15:59 |
| jcsackett | it's not a very big one. :-) | 15:59 |
| === deryck is now known as deryck[lunch] | ||
| === abentley changed the topic of #launchpad-reviews to: On call: henninge, abentley || Reviewing: noodles775,jcsackett || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| abentley | jcsackett: On line 19, you have """comment""". Please put an actual docstring. | 16:10 |
| jcsackett | abentley: yeah, that would be better, wouldn't it? sorry, forgot i had put in that place holder. | 16:11 |
| abentley | jcsackett: I have never seen raise expose( done before, even for exceptions that need to be exposed. Are you sure you need it? | 16:11 |
| jcsackett | sinzui and i discussed it, and it was key to resolving a similar oops. | 16:12 |
| jcsackett | abentley: my understanding was that the entire purpose of expose() was to make certain the exception gets raised across the API appropriately. | 16:13 |
| abentley | jcsackett: IME you just need webservice_error(httplib.BAD_REQUEST) for that. | 16:13 |
| jcsackett | abentley: i'll try making that change and seeing how tests work; assuming everything still passes i'll commit the removal of expose. | 16:14 |
| abentley | jcsackett: there are only two uses of the string 'raise expose' under lib/lp. I bet they're either unnecessary, or used to wrap an externally-defined exception. | 16:17 |
| === Ursinha is now known as Ursinha-lunch | ||
| jcsackett | abentley: good to know. test is running right now. | 16:18 |
| jcsackett | abentley: results will take a bit longer; need to make build after merging in devel this morning. | 16:19 |
| abentley | jcsackett: so the "Unknown status: " exception cannot be encountered over the web service? | 16:21 |
| === salgado is now known as salgado-lunch | ||
| jcsackett | not with launchpadlib--it catches it internally; as the branch explicitly targets the invalid transition over launchpadlib that's what i focused testing on. | 16:22 |
| abentley | jcsackett: so it could be encountered with other clients that aren't launchpadlib? | 16:23 |
| jcsackett | i would assume; as i said, i felt looking through all of those options was out of scope for the branch/bug. | 16:23 |
| jcsackett | that said i'm not entirely against adding additional test coverage; i put this up for MP b/c i thought the scope was met, but i can be convinced otherwise. | 16:25 |
| noodles775 | henninge: I've just pushed r9811 which updates a windmill test to check the blacklisting UI. | 16:25 |
| henninge | oh, ok. | 16:25 |
| jcsackett | abentley: without expose() the status is returned as 500, not 400. | 16:36 |
| jcsackett | abentley: i have pushed up a change with a docstring in place of the placeholder i missed. | 16:41 |
| abentley | jcsackett: in lp/code/model/branch.py BranchMergeProposalExists is raised without expose, and lp/code/stories/branchmergeproposal.txt shows it gives a 400. | 16:46 |
| === matsubara is now known as matsubara-lunch | ||
| jcsackett | abentley: i'm simply reporting what is happening here. given there is very little complexity to what's going on, it's my intuition that expose is needed here, since otherwise expected behavior doesn't occur. | 16:51 |
| jcsackett | i will take a look at branchmergeproposal | 16:51 |
| abentley | jcsackett: It would be unfortunate if we had to start wrapping expose() around everything we were raising. I'd like to get to the bottom of this. | 16:52 |
| jcsackett | abentley: it doesn't look like branchmergeproposal is using launchpadlib for its test. | 16:53 |
| abentley | jcsackett: correct. That test pre-dates the use of launchpadlib for testing the API. | 16:54 |
| jcsackett | abentley: i've already encountered places where launchpadlib behaves differently--mightn't it be the case that therein lies the issue? | 16:55 |
| abentley | jcsackett: launchpadlib shouldn't translate 400 errors into 500 errors, if that's what you mean. | 16:56 |
| === deryck[lunch] is now known as deryck | ||
| jcsackett | of course not. i simply mean that as the clearest difference in access, i would guess something is going on there. | 16:57 |
| james_w | webservice_error isn't quite enough to do what you want | 16:59 |
| james_w | one moment while I find the file | 16:59 |
| james_w | ./lib/lp/code/interfaces/webservice.py for why BranchMergeProposalExists returns 400 in that test | 17:00 |
| james_w | registry apparently doesn't use the new preferred pattern for that yet | 17:00 |
| jcsackett | james_w: i'm not sure i understand; i'm seeing a page of imports, nothing else. is this to get around cyclical import errors? | 17:02 |
| james_w | jcsackett: no, it's to make lazr.restful aware of the errors | 17:03 |
| james_w | it introspects the classes rather than the obects when they are raised IIRC | 17:03 |
| jcsackett | james_w: is the mechanism or requirement for this documented anywhere? i can understand importing the errors, but am unsure why the other imports exist given that explanation. | 17:05 |
| * jcsackett is a trifle confused. | 17:05 | |
| james_w | the other things are things that have exported() declarations IIRC | 17:05 |
| james_w | I thought this was written on the wiki somewhere but I can't find it now | 17:08 |
| jcsackett | james_w: thanks. | 17:08 |
| jcsackett | i'll try creating a similar file and removing expose. | 17:08 |
| james_w | lib/canonical/launchpad/interfaces/ | 17:10 |
| james_w | that's where registry and the other components that haven't created their own file dump all of that stuff | 17:10 |
| james_w | code has lib/lp/code/configure.zcml: <webservice:register module="lp.code.interfaces.webservice" /> | 17:10 |
| jcsackett | ah ha! that's the magic. | 17:13 |
| jcsackett | i will set that up. may be a bit before i can ping back though as i have another branch i need to take a look at. | 17:13 |
| jcsackett | abentley: given the above, you may want to put me in Needs Info or similar and get me out of the queue for now. | 17:14 |
| === benji is now known as benji-lunch | ||
| === henninge changed the topic of #launchpad-reviews to: On call: abentley || Reviewing: jcsackett || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| henninge | sinzui: I did this after my review this morning. If you find it useful, you can approve it ;-) | 17:28 |
| henninge | https://code.edge.launchpad.net/~henninge/launchpad/devel-update-copyright/+merge/36701 | 17:28 |
| sinzui | sweet! | 17:29 |
| * henninge has to go now | 17:29 | |
| === Ursinha-lunch is now known as Ursinha | ||
| === salgado-lunch is now known as salgado | ||
| === matsubara-lunch is now known as matsubara | ||
| === sinzui changed the topic of #launchpad-reviews to: On call: abentley || Reviewing: jcsackett || queue: [sinzui, sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| sinzui | abentley, I have two chr related branches for review if you have time today, contact-launchpad-0, contact-launchpad-2 | 18:09 |
| leonardr | abentley, i'd like your opinion of https://code.edge.launchpad.net/~leonardr/lazr.restful/invalid-uri/+merge/36759 | 18:15 |
| === benji-lunch is now known as benji | ||
| === abentley changed the topic of #launchpad-reviews to: On call: abentley || Reviewing: sinzui || queue: [sinzui, leonardr] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| abentley | sinzui: singular of "emails" is "email" on line 128 | 18:42 |
| abentley | sinzui: I am confused about the sort order at 190. (doesn't look alphabetical...) | 18:44 |
| abentley | sinzui: nm. | 18:45 |
| === abentley changed the topic of #launchpad-reviews to: On call: abentley || Reviewing: sinzui || queue: [leonardr] || 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: leonardr || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| abentley | leonardr: it looks reasonable to me. | 18:57 |
| abentley | leonardr: you asked for my opinion, but I'd be happy to approve it as well. | 18:59 |
| leonardr | abentley, yes, that was a polite way of asking for a review | 19:00 |
| abentley | leonardr: r=me | 19:00 |
| === 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 | ||
| === matsubara is now known as matsubara-afk | ||
| === 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 | ||
| rockstar | Anyone around for a review? | 22:35 |
| === salgado is now known as salgado-afk | ||
| rockstar | sinzui? ^^ | 22:36 |
| sinzui | rockstar, yes | 22:37 |
| rockstar | https://code.edge.launchpad.net/~rockstar/launchpad/edit-recipe-too-new/+merge/36800 | 22:37 |
| sinzui | rockstar, r=me | 22:40 |
| rockstar | sinzui, thanks | 22:40 |
| === Ursinha is now known as Ursinha-afk | ||
| === matsubara-afk is now known as matsubara | ||
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!