= ReviewerMeeting20081210 = == summary == * print strings instead of returning them (barry to update reviewer docs) * use `pretty()` except for API stuff where you should use `pprint_*()` functions * flacoste to work on API reviewer cheatsheet (from https://launchpad.canonical.com/API/StyleGuide) * rs=barry for any branch that cleans up pretty() and lazr's pretty printer == log == {{{ Dec 10 10:00:07 #startmeeting Dec 10 10:00:07 Meeting started at 09:00. The chair is barry. Dec 10 10:00:07 Commands Available: [TOPIC], [IDEA], [ACTION], [AGREED], [LINK], [VOTE] Dec 10 10:00:20 hello and welcome to this week's ameu reviewers meeting. who's here today? Dec 10 10:00:24 me Dec 10 10:00:40 me Dec 10 10:00:44 me Dec 10 10:01:28 me Dec 10 10:02:13 That's it? Dec 10 10:02:16 wow, light attendance today Dec 10 10:02:25 allenap: ping Dec 10 10:02:32 me Dec 10 10:02:35 bac_uds: hopeful ping Dec 10 10:02:39 bigjools: ping Dec 10 10:02:44 BjornT: ping Dec 10 10:02:47 cprov: ping Dec 10 10:02:49 me Dec 10 10:02:58 gmb: ping? Dec 10 10:03:03 intellectronica: ping Dec 10 10:03:07 mars: ping Dec 10 10:03:16 salgado: ping Dec 10 10:03:18 it's 7 in the morning for cprov at UDS Dec 10 10:03:18 me Dec 10 10:03:28 me Dec 10 10:03:35 bigjools: what's his cell #? let's wake him up :) Dec 10 10:03:45 * barry is not sure who's at uds Dec 10 10:03:46 Ooh, that's bad... Dec 10 10:03:49 1-800-EAT-SHIT Dec 10 10:03:53 ! Dec 10 10:03:53 * barry dials Dec 10 10:03:57 me Dec 10 10:04:03 bigjools: hey, waiiittt a minute! Dec 10 10:04:09 :) Dec 10 10:04:21 [TOPIC] agenda Dec 10 10:04:22 New Topic: agenda Dec 10 10:04:30 * Roll call Dec 10 10:04:30 * Printing strings in doctests (barry) Dec 10 10:04:30 * Gavin's `pretty()` function (allenap) Dec 10 10:04:30 * Do we need a standard cover letter template for merge proposals? (barry) Dec 10 10:04:30 * [[attachment:cover-quick.txt]] Dec 10 10:04:30 * [[attachment:cover.txt]] Dec 10 10:04:30 * Peanut gallery (anything not on the agenda) Dec 10 10:04:30 * If there's time, the old boring script Dec 10 10:04:30 * Next meeting Dec 10 10:04:30 * Action items Dec 10 10:04:30 * Mentoring update Dec 10 10:04:30 * Queue status Dec 10 10:04:38 [TOPIC] * Printing strings in doctests (barry) Dec 10 10:04:47 New Topic: * Printing strings in doctests (barry) Dec 10 10:05:06 so, i mentioned this in a couple of reviews as well as in the asiapac meeting, with some positive feedback Dec 10 10:05:22 the idea is that in docstrings, you should (usually) be printing the value of strings instead of returning them Dec 10 10:05:42 me Dec 10 10:05:44 the advantage of that is that you usually don't care if its unicode or str, and you're not saddled with extraneous noise like u' and ' Dec 10 10:05:48 e.g. Dec 10 10:05:50 instead of Dec 10 10:05:57 >>> foo['key'] Dec 10 10:05:59 u'baz' Dec 10 10:06:01 you'd do Dec 10 10:06:11 >>> print foo['key'] Dec 10 10:06:12 baz Dec 10 10:06:16 thoughts? Dec 10 10:06:28 me! Dec 10 10:06:31 sorry, I'm late Dec 10 10:06:33 yes, i also prefer printing Dec 10 10:06:59 barry: +1 Dec 10 10:07:07 Unless we fix/hack the the testrunner, translations and answers tests will fail Dec 10 10:07:51 sinzui: right. hence the "usually" :) Dec 10 10:07:58 barry, +1 Dec 10 10:08:30 so, as reviewers just be aware of that in code you review and suggest conversion to printing strings Dec 10 10:08:34 any objections? Dec 10 10:08:45 none, just a questin: Dec 10 10:09:00 adeuring: go4it Dec 10 10:09:21 I think that the bug really needs to be fixed in the testrunner, since it doesn't tell you which line the error is on when you try printing out a unicode character, the whole doctest just fails, so you end up commenting out half your test to find the offending line. Dec 10 10:09:37 what, if we want to test, if we want to check that we have for eaxmpale a string object, but not a unicode object? Dec 10 10:09:45 print repr(foo) Dec 10 10:09:46 EdwinGrubbs, +1 Dec 10 10:09:54 EdwinGrubbs: totally agree. it's not like we don't actually have proposed fixes either :) iirc it's a bug in zope upstream Dec 10 10:10:15 I mean, whould we use explicity the print statment, Dec 10 10:10:28 adeuring: in that case you can either return the value and show the u'' Dec 10 10:10:28 or should we allow the simple >>> foo Dec 10 10:10:40 adeuring: or, use isinstance(foo, unicode) Dec 10 10:10:58 adeuring: or even print foo.encode('utf-8') Dec 10 10:10:59 I'd prefer the second choice. Dec 10 10:11:14 rockstar: for a pure type test, i agree Dec 10 10:11:37 barry, yea' and if you want the value, you'd use print. Dec 10 10:11:45 barry: OK, though explicit encoding can make things more difficult to read Dec 10 10:11:47 rockstar: right Dec 10 10:12:08 adeuring: yep. if we had a fixed testrunner we could just print it Dec 10 10:12:19 flacoste: can we ask gary to look into fixing this upstream for us? Dec 10 10:12:29 barry: ...or use repr(u'äüö') Dec 10 10:12:37 barry: gary doesn't have on the testrunner Dec 10 10:12:40 slightly better to read than the encoded string Dec 10 10:12:44 barry: but he can send emails Dec 10 10:12:52 barry: but anyway, we are way behind upstream here Dec 10 10:13:00 barry: so we should just fix our stuff for now Dec 10 10:13:12 flacoste, +1 Dec 10 10:13:16 flacoste: maybe when we move to 2.6 :) Dec 10 10:13:22 and zc.buildout :) Dec 10 10:13:32 2.6 is crack! Dec 10 10:13:38 :-D Dec 10 10:13:50 anyway for now, just be aware of this in reviews and make friendly suggestions Dec 10 10:14:00 [TOPIC] * Gavin's `pretty()` function (allenap) Dec 10 10:14:01 New Topic: * Gavin's `pretty()` function (allenap) Dec 10 10:14:17 allenap landed a branch with a nice pretty() function in doctest globals Dec 10 10:14:29 don't use it for API tests btw! Dec 10 10:14:29 although i've since heard that lazr has something similar, right flacoste ? Dec 10 10:14:37 zope.testing.doctest:1486 Dec 10 10:14:37 sys.stdout = self._fakeout Dec 10 10:14:37 Becomes Dec 10 10:14:37 from codecs import EncodedFile Dec 10 10:14:37 sys.stdout = EncodedFile(self._fakeout, "ascii", "utf-8") Dec 10 10:14:39 flacoste: too late. Dec 10 10:14:48 flacoste: why? Dec 10 10:14:54 sinzui: yes, excactly Dec 10 10:14:57 because there is pprint_entry and pprint_collection Dec 10 10:15:03 that does that + other stuff Dec 10 10:15:08 flacoste: ah Dec 10 10:15:11 for example, it omits the etag key Dec 10 10:15:24 so pretty() is fine for non-API stuff Dec 10 10:15:36 flacoste: do we need a dev/reviewer style guide for apis? Dec 10 10:15:47 yeah, we probably do Dec 10 10:15:52 flacoste: Yeah, agreed, the pprint_* functions are better for API. I'll clean up my API stuff that uses pretty(). Dec 10 10:15:54 salgado started something if i recall Dec 10 10:16:11 or maybe it was for the coder perspective Dec 10 10:16:26 but yes, i'll do a API reviewer cheatsheet Dec 10 10:16:38 [ACTION] flacoste to do an API reviewer cheatsheet Dec 10 10:16:39 ACTION received: flacoste to do an API reviewer cheatsheet Dec 10 10:16:41 flacoste: thanks! Dec 10 10:16:43 or add a section to the reviewer cheet sheet Dec 10 10:17:12 btw, rs=barry for any cleanup that consolidates pretty() and lazr's pretty printer Dec 10 10:17:38 flacoste, barry, https://launchpad.canonical.com/API/StyleGuide is what I wrote Dec 10 10:18:05 salgado: nice, thanks Dec 10 10:18:28 [TOPIC] * Do we need a standard cover letter template for merge proposals? (barry) Dec 10 10:18:29 New Topic: * Do we need a standard cover letter template for merge proposals? (barry) Dec 10 10:18:58 i mentioned this last week. this week i have two examples. one a quick outline and the other more detailed, with examples Dec 10 10:19:05 https://dev.launchpad.net/ReviewerMeetingAgenda?action=AttachFile&do=view&target=cover-quick.txt Dec 10 10:19:17 https://dev.launchpad.net/ReviewerMeetingAgenda?action=AttachFile&do=view&target=cover.txt Dec 10 10:19:43 putting aside the issue of tool support for this, what do you think about encouraging devs to use this in mps? Dec 10 10:19:54 the thing i like about it is that everything's in one comment Dec 10 10:20:07 so when you get the mp email, you've already got the description, the lint, and the diff Dec 10 10:20:15 and it makes it very easy to respond in email Dec 10 10:20:46 i really hate it when i have to review a branch, but it's spread out over several emails Dec 10 10:21:02 barry, I don't like that either. Dec 10 10:21:16 I usually try and consolidate it into one reply. Dec 10 10:21:37 rockstar: right, which isn't always easy depending on your mua :) Dec 10 10:22:01 Yea. Dec 10 10:22:09 (vim ftw) Dec 10 10:22:27 rockstar: claws + emacs + emacsclient :) Dec 10 10:22:33 jeez, copy and paste ffs :) Dec 10 10:22:47 anyway. +1, -1 for the cover standard? Dec 10 10:22:59 +0 Dec 10 10:23:04 barry, which one is would be the standard? Dec 10 10:23:22 rockstar: they're the same except for the explanatory text and examples in the long one Dec 10 10:23:32 rockstar: so cover-quick.txt is the template most people would use Dec 10 10:24:22 how much of the stuff in the template should we / can we put in the MP form? Dec 10 10:24:43 * sinzui has been pasting it all Dec 10 10:25:26 sinzui: right. the idea is that you begin filling out the form right when you start working on the branch. lots will be empty, but you fill it in as you think about the fix, get a pre-imp, hack the code, etc Dec 10 10:25:35 when you're ready for the mp, you paste the whole thing Dec 10 10:25:37 barry, why do we need bug XXXXXX section? Dec 10 10:25:58 rockstar: as the longer example says, it's to translate the bug description into human Dec 10 10:26:15 barry: correct. I do, Dec 10 10:26:26 from the native "user" :) Dec 10 10:26:41 Hm. I think if I read the bug itself, and then read the solution, I could deduce the issues myself. Dec 10 10:27:03 rockstar: remember, we're trying to make things fast and easy for reviewers Dec 10 10:27:10 doesn't the branch link to the bug if you say --fixes? Dec 10 10:27:11 rockstar: so the dev should do more work upfront (imo) Dec 10 10:27:18 bigjools, yes Dec 10 10:27:24 * salgado is now known as salgado-lunch Dec 10 10:27:39 rockstar: the idea is. i'm going to review your branch in 5 minutes. i should have everything i need right there in that one email Dec 10 10:27:43 barry, I'm all for people making the reviewer's life easier. Dec 10 10:27:50 rockstar: don't make me go looking around the world unless i want to Dec 10 10:27:54 I think that doing that and having the reviewer follow a link is better than the dev re-writing the bug description Dec 10 10:28:15 bigjools: the problem is that many bug have poor descriptions, or long conversational threads Dec 10 10:28:37 bigjools: i think it's the dev's resposibility to boil it down to: "Bug XYZ describes this very specific problem" Dec 10 10:28:40 barry, so why can't that translation to human be in the bug. Dec 10 10:28:45 barry: so I would rather the root cause was fixed then - lets update the bug Dec 10 10:29:18 i guess the point is that i don't want to have to hunt around in the bug. i want to read my email, do my review and be done with it Dec 10 10:29:24 barry, maybe that would make sense as a sentence in the pre-implementation of implementation section. Dec 10 10:30:02 I hear we've got this funky tool called Launchpad that stores all this stuff for us ;) Dec 10 10:30:19 i should try to find a real-world example to show you what i mean. it's really just a 2 sentence summary Dec 10 10:30:39 and you can just cut-n-paste the bug description if it's in human readable form Dec 10 10:30:57 If there's good information about the bug itself, I feel it really belongs in the bug, not the merge proposal. Dec 10 10:31:12 but if it's not, or the bug has a long conversational thread that eventually gets around to the real bug, then 2 sentences, boom you're done, and you've helped reduce reviewer friction Dec 10 10:31:35 It may be just two lines, but it's still segmentation of information. Dec 10 10:31:38 let me propose this: if the bug's description was included in the branch page (is it already?) and the email, and the description was updated if it's inaccurate, would that suffice? Dec 10 10:31:44 rockstar: this isn't a big detailed thing, it's a summary. the cover can always say there are important details in the tracker issue Dec 10 10:32:03 QA might not know to go look at the merge proposal for the actual explanation. Dec 10 10:32:07 rockstar: I summarize the bug because I don't want to make you spend 20 minutes reading all the comments that were not relevant to diagnosing the root cause Dec 10 10:32:42 bigjools: sure. if it were included in the email that would help Dec 10 10:32:55 sinzui: exactly Dec 10 10:33:04 sinzui, I understand that, and I agree it's helpful. I just think that we have all these buckets where certain types of information go. Dec 10 10:33:05 barry: great - I just hate the idea of re-writing the bug description and not including it in the bug report! Dec 10 10:33:06 this is all about reducing reviewer friction Dec 10 10:33:22 barry, and I agree it needs to happen. Dec 10 10:33:41 I also like to make sure that the branch is linked to the appropriate bugs it fixes. Dec 10 10:33:44 rockstar: secondly, I may land five branches to fix a bug, each branch focuses on an aspect of the bug that is strictly scoped so that I have small branches Dec 10 10:33:49 bigjools: i have no problem if the workflow is: dev updates the bug description to accurately reflect the root cause of the problem being reported, then cut-and-pastes that into his cover letter :) Dec 10 10:33:54 i think we should rename 'Bug XXX' to 'Summary', and concentrate on describing what the branch contains, rather than what the bug was. Dec 10 10:34:12 sinzui: excellent point! Dec 10 10:34:14 BjornT, +1 Dec 10 10:34:50 +1, and we already have that in the submission form, so the only thing is to make sure that it's displayed conveniently both on the web and in email Dec 10 10:34:54 BjornT: again, I think that should be recorded in LP, with the branch. It can be sent with the email. Dec 10 10:34:59 BjornT: Summary of Bug XXXXX and i'm on board with that Dec 10 10:35:16 barry: not all branches are bug fixes though Dec 10 10:35:24 bigjools: hmm. okay Dec 10 10:35:30 barry: well, as sinzui said, he might land five branches to fx Dec 10 10:35:33 but they should be, right? :) Dec 10 10:35:34 fix a single bug Dec 10 10:35:43 barry: blueprints? :) Dec 10 10:35:46 barry, two lines in of the faur line Summary can be devoted to it. Dec 10 10:35:53 barry: should he include the same bug description in all five merge proposals? Dec 10 10:35:55 s/faur/four Dec 10 10:35:58 bigjools: i've heard it's a great way to increase your karma Dec 10 10:36:14 barry: you get more for blueprints! :) Dec 10 10:36:36 BjornT: when i've done that, the first sentence is the same, but then the rest of that summary section goes on to explain the sub-issue this branch is addressing Dec 10 10:36:44 but i'm okay with Summary Dec 10 10:37:10 barry: right. in that case you're not summarizing the bug report; you're summarizing your branch Dec 10 10:37:19 as long as i don't have to guess which bug it's addressing Dec 10 10:37:32 * EdwinGrubb (n=egrubbs@216-188-227-97.dyn.grandenetworks.net) has joined #launchpad-meeting Dec 10 10:37:36 BjornT: cool, i'm convinced. Summary sounds good Dec 10 10:38:05 i'll make that change and take it to the ml Dec 10 10:38:17 i do agree that the summary should explain in words what issue was fixed, rather than just saying 'fixes bug xxx'. Dec 10 10:38:35 btw, i really appreciate all the good feedback. ultimately this is about making things easier for us reviewers! Dec 10 10:38:50 you should work harder when your dev hat is on :) Dec 10 10:39:00 BjornT: +1 Dec 10 10:39:29 as someone once said to me, we're being worked like camels already :) Dec 10 10:39:56 bigjools: :) reducing dev friction is a whole 'nuther topic Dec 10 10:40:16 but not for today :) Dec 10 10:40:29 5 mins left! Dec 10 10:40:32 [TOPIC] # Peanut gallery (anything not on the agenda) Dec 10 10:40:33 New Topic: # Peanut gallery (anything not on the agenda) Dec 10 10:40:44 bigjools: what a great straight man you are! Dec 10 10:40:56 so, the floor is open. do you have anything not on the agenda? Dec 10 10:41:16 How are merge proposals going? Dec 10 10:41:37 We're fixing bugs to get things easier to use. Dec 10 10:41:39 I think they're great, but I think they could prompt us for the info that barry wants :) Dec 10 10:41:44 Is anyone noticing? Dec 10 10:41:56 bigjools: honestly, i don't think so Dec 10 10:42:13 form filling is boring Dec 10 10:42:33 rockstar: you had me at "PendingReviews is dead". i think they're great Dec 10 10:42:39 flacoste: you'd rather copy & paste a template? Dec 10 10:42:39 flacoste: but note taking is safe and satisfying. Dec 10 10:42:44 barry, Hurray! Dec 10 10:42:54 yeah, free form text FTW! Dec 10 10:42:56 tool support would definitely make it easier Dec 10 10:43:09 rockstar: plus i want a button to re-send me the mp email :) Dec 10 10:43:13 I like being reminded of things, I'm not getting any younger! Dec 10 10:43:34 flacoste: +1 Dec 10 10:44:07 flacoste, rockstar how much of the mp stuff is exposed in the api? Dec 10 10:44:09 bigjools: you should be able to use 'bzr send' and have the template come up in your text editor. Dec 10 10:44:28 BjornT: bzr send creates MPs? Dec 10 10:44:31 barry, the basic read only stuff landed last week. Dec 10 10:44:36 BjornT: though of course, you've already started the template when you began to think about the bug, had your pre-impl, etc. Dec 10 10:44:50 bigjools: well, somehow you should be able to submit MPs from bzr Dec 10 10:44:53 bigjools, it will, as soon as abentley gets back from UDS. Dec 10 10:45:06 BjornT: big +1 there Dec 10 10:45:07 BjornT, yea, that's the idea. Dec 10 10:45:11 BjornT: agree Dec 10 10:45:18 rockstar: that would be very cool! Dec 10 10:45:20 personally, i would rather have web forms (and perhaps the ability to bypass them using the API) Dec 10 10:45:31 and i think they would be very good for are users too Dec 10 10:45:38 well both - choice is good Dec 10 10:45:41 So diffs and bzr send are the big ones for mp right now then? Dec 10 10:46:05 rockstar: i think so Dec 10 10:46:31 oops, look at the clock. we're the time go? Dec 10 10:46:35 Cool. I'll bring that up in the meeting. I know the code has been landing, just don't know the specifics. Dec 10 10:46:42 anything else before we break? Dec 10 10:46:50 5 Dec 10 10:46:52 rockstar: especially the possiblity to attach diffs (rather than having them generated automatically, which is also nice of course) Dec 10 10:47:00 4 Dec 10 10:47:07 3 Dec 10 10:47:09 BjornT, agreed Dec 10 10:47:13 2 Dec 10 10:47:18 1 Dec 10 10:47:19 * EdwinGrubbs has quit (Connection timed out) Dec 10 10:47:20 Break! Dec 10 10:47:21 #endmeeting }}}