[00:10] jcsackett: r=me if you add it to the NEWS.txt === Ursinha-bbl is now known as Ursinha [03:53] https://code.edge.launchpad.net/~thumper/launchpad/less-wip-emails/+merge/39489 [03:53] anyone [03:53] StevenK: are you on? [03:56] thumper: I am now. [04:07] StevenK: want to look at that review? [04:07] StevenK: we can get someone else to mentor your review [04:07] StevenK: but you can have the practice [04:09] 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] StevenK: oh, forgot [04:09] sorry, feel free to leave it [04:09] thumper: Ah, you thought I was in .au? [04:09] yeah [04:09] Heh, yes. [04:57] wallyworld: I'm looking at your review [04:58] 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] no, just ignore it [04:58] the popup says: "The site says: 'bookmark-user-auth blah blah blah' [04:59] 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] and i also get "invalid security certificate" [04:59] and it leaves zombie firefox instances eating all my cpu :-( [05:00] and sometimes i get that page where you need to click to add a security exception [05:00] makes it very hard to run the tests [05:01] is it just me/my setup or is it a general problem? [05:01] I don't get that.... [05:02] 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] :( [05:03] I gave you the wrong info [05:03] str(e) isn't good enough [05:03] has no linked branch. [05:03] is kinda crap [05:04] ah bugger. i didn't see that when i ran it up. [05:04] probably cause i had fake products foo and bar [05:04] i'll make the test better and fix the problem [05:05] leave it with me [05:05] wallyworld: cool [05:05] everything else ok though? [05:08] looks reasonable === jtv changed the topic of #launchpad-reviews to: On call: - || Reviewing: - || queue: [StevenK, jtv] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [05:11] jtv: hi [05:11] hi thumper [05:11] I'll trade you. [05:11] jtv: https://code.edge.launchpad.net/~thumper/launchpad/less-wip-emails/+merge/39489 [05:12] jtv: what do you have? [05:12] thumper: https://code.edge.launchpad.net/~jtv/launchpad/bug-662552-view-permissions/+merge/39481 [05:12] nice size for a review :) [05:13] Took enough research, I can tell you. [05:16] * jtv looks at Snapshot [05:17] jtv: Snapshot is what the LaunchpadFormView uses, and lazr.restful [05:17] jtv: the code I changed was using the delta object in error [05:17] it just happened to work [05:19] nod [05:23] 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] sure [05:23] * jtv suddenly realizes the danger of picking nits while doing mutual reviews [05:26] 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] *be* clearer, that is [05:27] jtv: probably, I was primarily just copying existing tests [05:27] (pot, kettle) [05:28] 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] jtv: where is that first line you want changed? [05:29] test_nominateReview_no_job_if_work_in_progress as well, top comment, 2nd line. [05:30] 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] jtv: because the positive test wants the one [05:31] does store.one return None or raise if not found? [05:31] Raises. [05:31] But any() does what you want here, no? [05:31] no [05:31] because if there was 4 [05:31] it would pass [05:31] Simple solution: have your helper return the result set. [05:31] thumper: can you ping me when you are done. [05:35] 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] ta [05:36] wallyworld: whazzup? [05:36] the issue is that NoLinkedBranch and friends use %r for their error messages [05:36] so i will have to catch each type and recreated the error message with %s [05:37] any idea why the exceptions don't use %s [05:37] not all types have a string representation [05:37] i guess it's to give a better indication of what the object is [05:37] hmm... [05:38] well, i would have to check but surely Product et al would have a string representation [05:39] don't expect that [05:39] thumper: and you're done. [05:39] use something like 'name' or display anme [05:39] display_name [05:39] jtv: used with person_logged_in(bmp.registrant) in those tests now [05:40] cool [05:41] 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] ? [05:41] that way i can then do a str() to get the error message [05:42] thumper: sound ok^^^^^^^^^^ ? i have to duck out and pick up the kid from school and buy more tonic water :-) [05:43] wallyworld: I think it would make more sense to have a better error string [05:43] wallyworld: perhaps another method on the exception [05:43] wallyworld: don't recreate a different one [05:43] wallyworld: the exception has all the info we need [05:43] wallyworld: it just isn't showing it to us in a nice way [05:44] i don't mean to create a new exception type [05:44] don't create a new object [05:44] ack. i didn't think we would want to dick with the exception code. but i can do that np [05:45] ie add a new method etc [05:45] creating as new object wasn't what i really wanted to do :-) === Ursinha is now known as Ursinha-afk === jtv changed the topic of #launchpad-reviews to: On call: - || Reviewing: - || queue: [StevenK] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews === allenap changed the topic of #launchpad-reviews to: On call: allenap || Reviewing: - || queue: [StevenK] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [11:25] hi allenap can i add a trivial branch to your queue? [11:25] bac: Certainly :) [11:25] great. LP is still processing the MP but it should show up shortly === bac changed the topic of #launchpad-reviews to: On call: allenap || Reviewing: - || queue: [StevenK, bac] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [11:26] i'm going to go walk around the block. brb === allenap changed the topic of #launchpad-reviews to: On call: allenap || Reviewing: bac || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews === allenap changed the topic of #launchpad-reviews to: On call: allenap || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews === matsubara-afk is now known as matsubara === Ursinha-afk is now known as Ursinha === mrevell is now known as mrevell-lunch === mrevell-lunch is now known as mrevell [14:06] allenap: couls you please review this MP: https://code.edge.launchpad.net/~adeuring/launchpad/bug-594247-unittests-for-searchtasks-3/+merge/39523 ? [14:06] adeuring: Sure. [14:06] thanks! === allenap changed the topic of #launchpad-reviews to: On call: allenap || Reviewing: adeuring || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [14:21] allenap, can I jump in your queue? [14:22] rockstar: Sure, I've had my eyes on your merge queue proposal already. [14:22] allenap, ooh, kinky. :) [14:22] allenap, for easy lookup: https://code.edge.launchpad.net/~rockstar/launchpad/merge-queue-index/+merge/39488 === rockstar changed the topic of #launchpad-reviews to: On call: allenap || Reviewing: adeuring || queue: [rockstar] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [14:42] 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] adeuring1: Sure. === allenap changed the topic of #launchpad-reviews to: On call: allenap || Reviewing: rockstar || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [14:42] allenap: thanks, and sorry for the confisuon... [14:43] adeuring1: Have you pushed the branch? [14:43] allenap: a few seconds ago... === matsubara is now known as matsubara-lunch [16:08] allenap, hey, my password fix doesn't actually clobber the password argument. [16:08] rockstar: Really? I must have misread. [16:09] 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] rockstar: Okay, cool. I'll add a comment to the mp. === allenap changed the topic of #launchpad-reviews to: On call: allenap || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [16:52] EdwinGrubbs, https://code.edge.launchpad.net/~leonardr/lazr.restfulclient/enhanced-consumer/+merge/39447 now incorporates the changes you wanted [16:55] leonardr: that looks good. === matsubara-lunch is now known as matsubara [16:55] ok, great === allenap 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 [18:10] lifeless: feel free to call me === benji is now known as benji-lunch [18:46] edwingrubbs: as long as you're reviewing my branches, want to look at a short follow-up? [18:46] https://code.edge.launchpad.net/~leonardr/lazr.restfulclient/system-wide-consumer/+merge/39552 [18:47] leonardr: I'm headed to lunch now, but I could look at it later today. [18:47] ok, i'll let you know if someone else takes it up [19:09] 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] leonardr: I really don't know which way would be better. === benji-lunch is now known as benji [19:16] edwingrubbs: i'll create it with no consumer. it's simpler, and i can change it later === matsubara is now known as matsubara-afk [21:54] 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] leonardr: I'll start on the review now. [21:56] thanks [22:38] 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] "lsb_release -si" before trying dist(). [22:48] EdwinGrubbs: my gut reaction to that news is to get rid of the dist() altogether and go right to system() [22:50] leonardr: ok, if "Linux" is distinct enough, that sounds fine. [22:51] "Linux" is fine [22:51] leonardr: btw, I already submitted my review, but it just rehashes this issue for the record. [22:51] ok, thanks [22:56] lifeless: ping [23:04] nm. [23:21] hi