| leonardr | jcsackett: r=me if you add it to the NEWS.txt | 00:10 |
|---|---|---|
| === Ursinha-bbl is now known as Ursinha | ||
| 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:53 |
| StevenK | thumper: I am now. | 03:56 |
| 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:07 |
| 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:09 |
| thumper | wallyworld: I'm looking at your review | 04:57 |
| 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:58 |
| 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 :-( | 04:59 |
| 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:00 |
| wallyworld | is it just me/my setup or is it a general problem? | 05:01 |
| thumper | I don't get that.... | 05:01 |
| 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:02 |
| 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:03 |
| 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:04 |
| wallyworld | leave it with me | 05:05 |
| thumper | wallyworld: cool | 05:05 |
| wallyworld | everything else ok though? | 05:05 |
| thumper | looks reasonable | 05:08 |
| === 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 | ||
| 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:11 |
| 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:12 |
| jtv | Took enough research, I can tell you. | 05:13 |
| * jtv looks at Snapshot | 05:16 | |
| 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:17 |
| jtv | nod | 05:19 |
| 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:23 | |
| 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:26 |
| jtv | *be* clearer, that is | 05:27 |
| thumper | jtv: probably, I was primarily just copying existing tests | 05:27 |
| jtv | (pot, kettle) | 05:27 |
| 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:28 |
| jtv | test_nominateReview_no_job_if_work_in_progress as well, top comment, 2nd line. | 05:29 |
| 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:30 |
| 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:31 |
| 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:35 |
| 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:36 |
| 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:37 |
| wallyworld | well, i would have to check but surely Product et al would have a string representation | 05:38 |
| 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:39 |
| jtv | cool | 05:40 |
| 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:41 |
| wallyworld | thumper: sound ok^^^^^^^^^^ ? i have to duck out and pick up the kid from school and buy more tonic water :-) | 05:42 |
| 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:43 |
| 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:44 |
| wallyworld | ie add a new method etc | 05:45 |
| wallyworld | creating as new object wasn't what i really wanted to do :-) | 05:45 |
| === 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 | ||
| 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:25 |
| === 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 | ||
| bac | i'm going to go walk around the block. brb | 11:26 |
| === 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 | ||
| 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:06 |
| === 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 | ||
| rockstar | allenap, can I jump in your queue? | 14:21 |
| 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:22 |
| === 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 | ||
| 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 |
| === 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 | ||
| adeuring1 | allenap: thanks, and sorry for the confisuon... | 14:42 |
| allenap | adeuring1: Have you pushed the branch? | 14:43 |
| adeuring1 | allenap: a few seconds ago... | 14:43 |
| === matsubara is now known as matsubara-lunch | ||
| rockstar | allenap, hey, my password fix doesn't actually clobber the password argument. | 16:08 |
| allenap | rockstar: Really? I must have misread. | 16:08 |
| 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:09 |
| allenap | rockstar: Okay, cool. I'll add a comment to the mp. | 16:11 |
| === 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 | ||
| leonardr | EdwinGrubbs, https://code.edge.launchpad.net/~leonardr/lazr.restfulclient/enhanced-consumer/+merge/39447 now incorporates the changes you wanted | 16:52 |
| EdwinGrubbs | leonardr: that looks good. | 16:55 |
| === matsubara-lunch is now known as matsubara | ||
| leonardr | ok, great | 16:55 |
| === 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 | ||
| flacoste | lifeless: feel free to call me | 18:10 |
| === benji is now known as benji-lunch | ||
| 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:46 |
| 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 | 18:47 |
| 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:09 |
| EdwinGrubbs | leonardr: I really don't know which way would be better. | 19:15 |
| === benji-lunch is now known as benji | ||
| leonardr | edwingrubbs: i'll create it with no consumer. it's simpler, and i can change it later | 19:16 |
| === matsubara is now known as matsubara-afk | ||
| 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:54 |
| EdwinGrubbs | leonardr: I'll start on the review now. | 21:56 |
| leonardr | thanks | 21:56 |
| 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:38 |
| leonardr | EdwinGrubbs: my gut reaction to that news is to get rid of the dist() altogether and go right to system() | 22:48 |
| EdwinGrubbs | leonardr: ok, if "Linux" is distinct enough, that sounds fine. | 22:50 |
| 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:51 |
| jcsackett | lifeless: ping | 22:56 |
| jcsackett | nm. | 23:04 |
| lifeless | hi | 23:21 |
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!