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