[01:01] <wallyworld> StevenK: ping?
=== _thumper_ is now known as thumper
[02:44] <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:45] <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:52] <mwhudson> lifeless: why is that lib/lp/testing/fakelibrarian.py shim still needed?
[03:00] <mwhudson> lifeless: couple more questions/comments on the proposal, but reviewed
[03:01] <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:02] <lifeless> the docstring in __init__ is a pep8 variable docstring
[03:03] <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:06] <mwhudson> lifeless: oh right
[03:06] <mwhudson> i'm not sure that idea ever really took off...
[03:07] <lifeless> I shall carry the flag!
[03:07] <lifeless> Until someone (like the friendly pydoctor maintainer) makes it work.
[03:08] <mwhudson> heh heh
[03:09] <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:10] <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
[06:26] <wallyworld> StevenK: you around today?
[06:27] <StevenK> wallyworld: Indeed, what's up?
[06:28] <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:29] <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:30] <wallyworld> :-)
[06:31] <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:32] <wallyworld> thanks
[06:36] <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:37] <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:38] <wallyworld> thanks for the info. i'm still new at this :-)
[09:11] <poolie> bac: hi, up yet?
[09:13] <poolie> is anyone able to review https://code.edge.launchpad.net/~mbp/launchpad/flags-gui/+merge/36415
[09:14] <jtv> poolie: I'll trade you for https://code.edge.launchpad.net/~jtv/launchpad/henninge_validate_translation-cleanup/+merge/36683
=== 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
[09:15] <poolie> > En passant?
[09:15] <poolie> i think your description is incompletely translated :-P
[09:16] <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:59] <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
=== 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
[12:13] <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:57] <henninge> jtv: you might like this one: https://code.edge.launchpad.net/~henninge/launchpad/devel-update-copyright/+merge/36701
[12:58]  * jtv makes a quick guess
[12:58]  * henninge lunches
=== 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
[12:58] <jtv> henninge: you're saying that rewriting all our source files just before committing them to mainline does not need testing?  :-)
[13:22] <jtv> Non-OCR reviewer needed!  https://code.launchpad.net/~jtv/launchpad/henninge_validate_translation-cleanup/+merge/36683
=== 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
[14:33] <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:34] <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
=== mrevell-lunch is now known as mrevell
[15:11] <noodles775> salgado: thanks for the review - can you pls add an approve review with the review type as ui*, if you're approving?
[15:12] <salgado> noodles775, oh, sure, forgot about that
[15:16] <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:25] <achuni> henninge: hi! have a minute for a one-byte review? :)
[15:26] <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:27] <henninge> achuni: r=me ;)
[15:28] <achuni> henninge: neat, thanks :)
=== 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
[15:32] <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
=== 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
[15:46] <henninge> noodles775: Hi!
[15:46]  * henninge was thinking UI review ... ;-)
[15:46] <henninge> noodles775: sure, I can do a code review for you ...
[15:47] <noodles775> henninge: nah, just code, but if you've anything to add to the ui review, feel free :)
[15:47] <noodles775> Thanks!
[15:48] <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:49] <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:53] <henninge> noodles775: "test_is_ajax" should be two test methods
[15:54] <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:55]  * 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.
=== 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
[15:59] <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. :-)
=== 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
[16:10] <abentley> jcsackett: On line 19, you have """comment""".  Please put an actual docstring.
[16:11] <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:12] <jcsackett> sinzui and i discussed it, and it was key to resolving a similar oops.
[16:13] <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:14] <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:17] <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.
=== Ursinha is now known as Ursinha-lunch
[16:18] <jcsackett> abentley: good to know. test is running right now.
[16:19] <jcsackett> abentley: results will take a bit longer; need to make build after merging in devel this morning.
[16:21] <abentley> jcsackett: so the "Unknown status: " exception cannot be encountered over the web service?
=== salgado is now known as salgado-lunch
[16:22] <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:23] <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:25] <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:36] <jcsackett> abentley: without expose() the status is returned as 500, not 400.
[16:41] <jcsackett> abentley: i have pushed up a change with a docstring in place of the placeholder i missed.
[16:46] <abentley> jcsackett: in lp/code/model/branch.py BranchMergeProposalExists is raised without expose, and lp/code/stories/branchmergeproposal.txt shows it gives a 400.
=== matsubara is now known as matsubara-lunch
[16:51] <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:52] <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:53] <jcsackett> abentley: it doesn't look like branchmergeproposal is using launchpadlib for its test.
[16:54] <abentley> jcsackett: correct.  That test pre-dates the use of launchpadlib for testing the API.
[16:55] <jcsackett> abentley: i've already encountered places where launchpadlib behaves differently--mightn't it be the case that therein lies the issue?
[16:56] <abentley> jcsackett: launchpadlib shouldn't translate 400 errors into 500 errors, if that's what you mean.
=== deryck[lunch] is now known as deryck
[16:57] <jcsackett> of course not. i simply mean that as the clearest difference in access, i would guess something is going on there.
[16:59] <james_w> webservice_error isn't quite enough to do what you want
[16:59] <james_w> one moment while I find the file
[17:00] <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:02] <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:03] <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:05] <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:08] <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:10] <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:13] <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:14] <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.
=== 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
[17:28] <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:29] <sinzui> sweet!
[17:29]  * henninge has to go now
=== 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
[18:09] <sinzui> abentley, I have two chr related branches for review if you have time today, contact-launchpad-0, contact-launchpad-2
[18:15] <leonardr> abentley, i'd like your opinion of https://code.edge.launchpad.net/~leonardr/lazr.restful/invalid-uri/+merge/36759
=== 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
[18:42] <abentley> sinzui: singular of "emails" is "email" on line 128
[18:44] <abentley> sinzui: I am confused about the sort order at 190.  (doesn't look alphabetical...)
[18:45] <abentley> sinzui: nm.
=== 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
[18:57] <abentley> leonardr: it looks reasonable to me.
[18:59] <abentley> leonardr: you asked for my opinion, but I'd be happy to approve it as well.
[19:00] <leonardr> abentley, yes, that was a polite way of asking for a review
[19:00] <abentley> leonardr: r=me
=== 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
[22:35] <rockstar> Anyone around for a review?
=== salgado is now known as salgado-afk
[22:36] <rockstar> sinzui? ^^
[22:37] <sinzui> rockstar, yes
[22:37] <rockstar> https://code.edge.launchpad.net/~rockstar/launchpad/edit-recipe-too-new/+merge/36800
[22:40] <sinzui> rockstar, r=me
[22:40] <rockstar> sinzui, thanks
=== Ursinha is now known as Ursinha-afk
=== matsubara-afk is now known as matsubara