[00:10] <leonardr> jcsackett: r=me if you add it to the NEWS.txt
[03:53] <thumper> https://code.edge.launchpad.net/~thumper/launchpad/less-wip-emails/+merge/39489
[03:53] <thumper> anyone
[03:53] <thumper> StevenK: are you on?
[03:56] <StevenK> thumper: I am now.
[04:07] <thumper> StevenK: want to look at that review?
[04:07] <thumper> StevenK: we can get someone else to mentor your review
[04:07] <thumper> StevenK: but you can have the practice
[04:09] <StevenK> thumper: It is also currently 11pm, and I'd like to sleep so I can be useful in the 9am session tomorrow, unlike today where I didn't feel awake until 10.
[04:09] <thumper> StevenK: oh, forgot
[04:09] <thumper> sorry, feel free to leave it
[04:09] <StevenK> thumper: Ah, you thought I was in .au?
[04:09] <thumper> yeah
[04:09] <StevenK> Heh, yes.
[04:57] <thumper> wallyworld: I'm looking at your review
[04:58] <wallyworld> thumper: thanks. actually, any idea why firefox  keeps asking me to enter a username/password when it is being ised by windmill to run the tests?
[04:58] <thumper> no, just ignore it
[04:58] <wallyworld> the popup says: "The site says: 'bookmark-user-auth blah blah blah'
[04:59] <wallyworld> yeah, but i have to click cancel 100000 times since there's so many of them and sometimes if i don't the test fails :-(
[04:59] <wallyworld> and i also get "invalid security certificate"
[04:59] <wallyworld> and it leaves zombie firefox instances eating all my cpu :-(
[05:00] <wallyworld> and sometimes i get that page where you need to click to add a security exception
[05:00] <wallyworld> makes it very hard to run the tests
[05:01] <wallyworld> is it just me/my setup or is it a general problem?
[05:01] <thumper> I don't get that....
[05:02] <wallyworld> hmmmm. what should have been a 5 minute test took way longer dealing with that stuff cause the test kept failing :-(. i'll have to look into it then
[05:03] <thumper> :(
[05:03] <thumper> I gave you the wrong info
[05:03] <thumper> str(e) isn't good enough
[05:03] <thumper> <Product at 0xa592f10> has no linked branch.
[05:03] <thumper> is kinda crap
[05:04] <wallyworld> ah bugger. i didn't see that when i ran it up.
[05:04] <wallyworld> probably cause i had fake products foo and bar
[05:04] <wallyworld> i'll make the test better and fix the problem
[05:05] <wallyworld> leave it with me
[05:05] <thumper> wallyworld: cool
[05:05] <wallyworld> everything else ok though?
[05:08] <thumper> looks reasonable
[05:11] <thumper> jtv: hi
[05:11] <jtv> hi thumper
[05:11] <jtv> I'll trade you.
[05:11] <thumper> jtv: https://code.edge.launchpad.net/~thumper/launchpad/less-wip-emails/+merge/39489
[05:12] <thumper> jtv: what do you have?
[05:12] <jtv> thumper: https://code.edge.launchpad.net/~jtv/launchpad/bug-662552-view-permissions/+merge/39481
[05:12] <thumper> nice size for a review :)
[05:13] <jtv> Took enough research, I can tell you.
[05:16]  * jtv looks at Snapshot
[05:17] <thumper> jtv: Snapshot is what the LaunchpadFormView uses, and lazr.restful
[05:17] <thumper> jtv: the code I changed was using the delta object in error
[05:17] <thumper> it just happened to work
[05:19] <jtv> nod
[05:23] <jtv> thumper: so far so good, but I just have to find something.  Could you replace "there is no email job created" (test_branchmergeproposal.py) with something easier to read?
[05:23] <thumper> sure
[05:23]  * jtv suddenly realizes the danger of picking nits while doing mutual reviews
[05:26] <jtv> thumper: wouldn't test_nominateReview_no_job_if_work_in_progress clearer if it just did a "with person_logged_in(bmp.registrant): bmp.nominateReviewer(...)" instead of logging in at what looks like a somewhat arbitrary point?
[05:27] <jtv> *be* clearer, that is
[05:27] <thumper> jtv: probably, I was primarily just copying existing tests
[05:27] <jtv> (pot, kettle)
[05:28] <jtv> There's one other thing in that method: in the IStore(...).find(), the last line of the condition isn't indented enough.  I think it'd help to keep BMPJobType.REVIEW_REQUEST_EMAIL in a variable.  One with a short name.
[05:28] <thumper> jtv: where is that first line you want changed?
[05:29] <jtv> test_nominateReview_no_job_if_work_in_progress as well, top comment, 2nd line.
[05:30] <jtv> Oh!  How about putting the IStore(...).find() in a helper method, so that the positive test can check for the exact same thing?
[05:30]  * thumper was looking in old branch
[05:30] <thumper> jtv: because the positive test wants the one
[05:31] <thumper> does store.one return None or raise if not found?
[05:31] <jtv> Raises.
[05:31] <jtv> But any() does what you want here, no?
[05:31] <thumper> no
[05:31] <thumper> because if there was 4
[05:31] <thumper> it would pass
[05:31] <jtv> Simple solution: have your helper return the result set.
[05:31] <wallyworld> thumper: can you ping me when you are done.
[05:35] <jtv> thumper: there are a few other places in the test that could potentially be nicer with a "with person_logged_in()" but I'll leave that up to you.  Approving.
[05:35] <thumper> ta
[05:36] <thumper> wallyworld: whazzup?
[05:36] <wallyworld> the issue is that NoLinkedBranch and friends use %r for their error messages
[05:36] <wallyworld> so i will have to catch each type and recreated the error message with %s
[05:37] <wallyworld> any idea why the exceptions don't use %s
[05:37] <thumper> not all types have a string representation
[05:37] <wallyworld> i guess it's to give a better indication of what the object is
[05:37] <thumper> hmm...
[05:38] <wallyworld> well, i would have to check but surely Product et al would have a string representation
[05:39] <thumper> don't expect that
[05:39] <jtv> thumper: and you're done.
[05:39] <thumper> use something like 'name' or display anme
[05:39] <thumper> display_name
[05:39] <thumper> jtv: used with person_logged_in(bmp.registrant) in those tests now
[05:40] <jtv> cool
[05:41] <wallyworld> yeah, so i have to catch the exceptions and recreate a new instance of the same type with display_name as the construction  arg
[05:41] <wallyworld> ?
[05:41] <wallyworld> that way i can then do a str() to get the error message
[05:42] <wallyworld> thumper: sound ok^^^^^^^^^^ ? i have to duck out and pick up the kid from school and buy more tonic water :-)
[05:43] <thumper> wallyworld: I think it would make more sense to have a better error string
[05:43] <thumper> wallyworld: perhaps another method on the exception
[05:43] <thumper> wallyworld: don't recreate a different one
[05:43] <thumper> wallyworld: the exception has all the info we need
[05:43] <thumper> wallyworld: it just isn't showing it to us in a nice way
[05:44] <wallyworld> i don't mean to create a new exception type
[05:44] <thumper> don't create a new object
[05:44] <wallyworld> ack. i didn't think we would want to dick with the exception code. but i can do that np
[05:45] <wallyworld> ie add a new method etc
[05:45] <wallyworld> creating as new object wasn't what i really wanted to do :-)
[11:25] <bac> hi allenap can i add a trivial branch to your queue?
[11:25] <allenap> bac: Certainly :)
[11:25] <bac> great.  LP is still processing the MP but it should show up shortly
[11:26] <bac> i'm going to go walk around the block. brb
[14:06] <adeuring> allenap: couls you please review this MP: https://code.edge.launchpad.net/~adeuring/launchpad/bug-594247-unittests-for-searchtasks-3/+merge/39523 ?
[14:06] <allenap> adeuring: Sure.
[14:06] <adeuring> thanks!
[14:21] <rockstar> allenap, can I jump in your queue?
[14:22] <allenap> rockstar: Sure, I've had my eyes on your merge queue proposal already.
[14:22] <rockstar> allenap, ooh, kinky.  :)
[14:22] <rockstar> allenap, for easy lookup: https://code.edge.launchpad.net/~rockstar/launchpad/merge-queue-index/+merge/39488
[14:42] <adeuring1> allenap: thanks for the review! may I ask for another review? Your remark about the assert() let me realize that I forgot to "bzr commit" the latest changes...
[14:42] <allenap> adeuring1: Sure.
[14:42] <adeuring1> allenap: thanks, and sorry for the confisuon...
[14:43] <allenap> adeuring1: Have you pushed the branch?
[14:43] <adeuring1> allenap: a few seconds ago...
[16:08] <rockstar> allenap, hey, my password fix doesn't actually clobber the password argument.
[16:08] <allenap> rockstar: Really? I must have misread.
[16:09] <rockstar> allenap, so, the only place that password gets used is creating a user if the user isn't specified.  Since we then get a naked user (ooh la la), we're essentially getting the password back from the user, so in some cases, it's really a no-op.
[16:11] <allenap> rockstar: Okay, cool. I'll add a comment to the mp.
[16:52] <leonardr> EdwinGrubbs, https://code.edge.launchpad.net/~leonardr/lazr.restfulclient/enhanced-consumer/+merge/39447 now incorporates the changes you wanted
[16:55] <EdwinGrubbs> leonardr: that looks good.
[16:55] <leonardr> ok, great
[18:10] <flacoste> lifeless: feel free to call me
[18:46] <leonardr> edwingrubbs: as long as you're reviewing my branches, want to look at a short follow-up?
[18:46] <leonardr> https://code.edge.launchpad.net/~leonardr/lazr.restfulclient/system-wide-consumer/+merge/39552
[18:47] <EdwinGrubbs> leonardr: I'm headed to lunch now, but I could look at it later today.
[18:47] <leonardr> ok, i'll let you know if someone else takes it up
[19:09] <leonardr> EdwinGrubbs: one other minor question. i was thinking of changing OAuthAuthorizer to be able to accept a Consumer object as well as a consumer name (so that I can create it with a SystemWideConsumer), but i could just create it with no consumer and then set the consumer afterwards. let me know which you would rather
[19:15] <EdwinGrubbs> leonardr: I really don't know which way would be better.
[19:16] <leonardr> edwingrubbs: i'll create it with no consumer. it's simpler, and i can change it later
[21:54] <leonardr> Edwin: can you look at that branch? i assumed you'd done the review since you asked that question, but i don't see a review from you
[21:56] <EdwinGrubbs> leonardr: I'll start on the review now.
[21:56] <leonardr> thanks
[22:38] <EdwinGrubbs> leonardr: I did a little looking into the platform module, since I was curious. It appears that some older versions of platform.dist() don't sort the results of listdir('/etc'), so it may randomly get its info from /etc/debian_version or /etc/lsb-release. That seems like it could cause some really confusing bug reports eventually, if authentication breaks. Maybe, you should check if /usr/bin/lsb_release exists and use
[22:38] <EdwinGrubbs> "lsb_release -si"  before trying dist().
[22:48] <leonardr> EdwinGrubbs: my gut reaction to that news is to get rid of the dist() altogether and go right to system()
[22:50] <EdwinGrubbs> leonardr: ok, if "Linux" is distinct enough, that sounds fine.
[22:51] <leonardr> "Linux" is fine
[22:51] <EdwinGrubbs> leonardr: btw, I already submitted my review, but it just rehashes this issue for the record.
[22:51] <leonardr> ok, thanks
[22:56] <jcsackett> lifeless: ping
[23:04] <jcsackett> nm.
[23:21] <lifeless> hi