=== Ursinha-dinner is now known as Ursinha === salgado-afk is now known as salgado [15:01] #startmeeting [15:01] Meeting started at 09:05. The chair is barry. [15:01] Commands Available: [TOPIC], [IDEA], [ACTION], [AGREED], [LINK], [VOTE] [15:01] hello everyone and welcome to this week's ameu reviewer's meeting. who's here today? [15:01] me [15:01] me [15:01] me [15:01] me [15:01] me [15:01] me [15:02] me [15:02] me [15:02] me [15:02] cprov is at debconf [15:02] splitter [15:02] curveball [15:03] danilos: ping [15:03] me [15:03] jtv: ping? [15:03] barry: on phone [15:03] jtv: np! hi! [15:03] hi [15:04] me [15:04] me [15:04] cool, let's start [15:04] [TOPIC] agenda [15:04] New Topic: agenda [15:04] * Roll call [15:04] * Next meeting [15:04] * Action items [15:04] * Queue status [15:04] * Mentoring update [15:04] * Review process [15:04] * merge proposals update [15:04] [TOPIC] next meeting [15:04] New Topic: next meeting [15:05] week +=1 ? anybody know they won't be here? [15:05] I won't be here. [15:05] * danilos , for a change, will be around in the next few months :) [15:05] abentley: cool, danilos :) [15:06] [TOPIC] action items [15:06] New Topic: action items [15:06] intellectronica: any status on your three? :) [15:06] sorry, haven't touched my action items yet. busy times :( [15:07] intellectronica: understood. we'll carry these forward [15:07] [TOPIC] queue status [15:07] New Topic: queue status [15:07] first, thanks everybody for taking on those extra reviews and clearing our queue. it was greatly appreciated [15:08] i think we actually don't look too bad right now, though there are 4 pink branches [15:08] notice testsuite2 is no longer at the top of the heap [15:09] any comments on the queue from y'all? [15:09] Are the three top items the ones that were assigned? [15:10] sinzui: good question. i'm pretty sure jml is aware of the mpt branch [15:10] abentley: what's up with tim's karma branch? [15:10] stub's got a branch for abentley and bigjools has a branch for adeuring [15:10] I think they are...I confess that I worked on my assigned branch during my on-call day [15:10] barry: He replied to my review last night. I'll be re-reviewing today. [15:11] yeah I did abel's today [15:11] abentley: cool. i still don't think i saw that branch to mentor it. maybe it got stuck in my junk folder or something. i will look around [15:11] bigjools: thanks [15:12] barry: You're not cc'ed, but launchpad-reviews is. [15:12] abentley: okay, i'll search my folder [15:12] thanks [15:13] anything else on the queue? [15:13] [TOPIC] mentoring [15:13] New Topic: mentoring [15:13] nothing much to report here from me. open to comments from the floor [15:14] I think that people who are being mentored should be referred to as "mentats" :-) [15:14] or mentos [15:14] abentley: :) i just watched dune on hbo a few nights ago :) [15:15] okay cool, moving on [15:15] [TOPIC] review process [15:15] New Topic: review process [15:15] * merge proposals update [15:16] bac: thanks for guinea pigging it and emailing your experiences [15:16] np. hope it was helpful. [15:16] definitely [15:16] Thou shall know a mentat bug the colour of his lips [15:17] I regret the confusion between review votes and actually approving/disapproving a merge proposal. [15:17] anybody else planning on trying it out this week? [15:17] barry: I'll see how loaded I am tomorrow. [15:17] gmb: cool [15:17] Some projects, e.g. bazaar, require more than one review. [15:17] abentley: do you have any thoughts on changes to make that easier to understand? [15:18] Not yet, but I'm open to ideas. [15:18] abentley: we need to update the wiki page to add in the step about doing the actual approval. the last step mentioned is still 'vote approve'. [15:19] abentley: what do you think about being able to establish automatic rules, e.g. two approvals and no rejections == Approve [15:19] bac: Okay. [15:20] abentley: you'll update the wiki? [15:20] barry: I will update it. [15:20] abentley: thanks [15:20] barry: I think automatic rules are nice. See: Bundle Buggy. [15:20] * barry should have known that bb already has it :) [15:21] Code Reviews are not wholly my design; sabdfl and thumper also had significant input. [15:22] sabdfl didn't want per-project customizations, IIRC. [15:22] I agree [15:22] abentley: i'm still not sure i agree with that, but we can continue discussing it on the ml [15:23] i have nothing else on the agenda, so we can continue to discuss this or call it a day [15:23] if we don't want per-project customizations, wouldn't it be better for merge proposals not to have a status of their own, and instead display the number of votes? [15:24] I should mention that I've started work on supporting "bzr send" for launchpad. So that should land sometime in August. [15:25] BjornT: interesting idea [15:25] abentley: would that be hookable so we can replace review-submit, or structures so r-s could wrap it? [15:26] Right now, I'm only looking at changing LP, not bzr. But yes, r-s could send merge directives today if mwhudson decided to implement that. [15:27] abentley: cool [15:28] anything else? [15:28] Is now a good time to discuss diff trimming? [15:28] abentley: sure [15:28] abentley: floor is yours [15:29] So I think it makes sense to trim diffs when sending a review, instead of just inserting comments in the middle of the diff. [15:29] It makes the review more readable, and makes it harder for the reviewee to miss anything you've said. [15:30] Do people agree, disagree, not care? [15:30] I don't know what you mean by trim. sorry [15:30] abentley: i have no idea what you mean by 'trimming' [15:30] sinzui: high five [15:30] i definitely disagree for mentats. the mentor needs to see even the stuff the mentor deemed uninteresting. [15:31] trim: Remove the sections of the diff that you don't want to comment on. [15:31] bac: i agree [15:31] bac: The mentor has a mail client that supports threading, one hopes? [15:31] bac: er, with you. mentats should not trim [15:31] bac: yes [15:31] I think it's down to the branch being reviewed [15:31] abentley: i think that's not a good idea, because lots of people other than the reviewer look at those emails and want to see the full diff [15:31] abentley: yes, but that's much less convenient for mentoring [15:31] abentley: sure, but if i'm mentoring i don't want to have to compare the review with the original, threads or not. [15:32] intellectronica: Why wouldn't they look at the full diff when it was submitted, then? [15:32] I prefer trimming. When I mentor, I read the entire change set after the mentoree has brought the review to merge-conditional* [15:32] fwiw, i never trim the original review, though i do trim follow ups [15:32] Me too. [15:32] barry: +1 [15:32] abentley: oh, you mean in replies ... [15:33] also as a developer i don't think it's that easy to miss comments in a properly formated review. [15:33] intellectronica: I mean if you want to look at the full diff, read it when it is submitted for review. [15:33] Trimming replies I have no problem with. [15:33] also, i find that as i'm reviewing, i'll often want to go back to and comment on an earlier section. if i trim as i'm reviewing, that's much harder to do, so i just don't trim [15:33] still, i think most mail clients format this conveniently enough for the reader [15:33] intellectronica: I do not mean replies, except in the sense that the initial review is a reply to the request for review. [15:34] abentley: yes, i mean reply in that sense. i sometimes do reviews like this, by replying to the request and leaving comments interleaved with the original diff [15:34] i always trim the diffs when i do a review. that's the last thing i do, though, so there isn't a problem going back commenting on other sections [15:35] abentley: i sympathize with the readability issue. it's definitely mua-specific though. for me, mail.app makes it trivially easy to find the comments. claws is more difficult [15:35] * sinzui ponders how an app that supports block annotation and folding would work [15:36] finally, it's more work which i won't really benefit from, so -1 from me on having a policy that requires that :) [15:36] intellectronica: So when you do an initial review, you remove sections of the diff that you don't want to comment on? [15:36] abentley: almost never [15:36] I too backtrack when I review. a comment may relate to 3 parts of the diff and It would be nice to select the comment to see all three chunks [15:36] i only remove the most mundane sections [15:36] intellectronica: I think you may have misread me, then. [15:37] oh [15:37] have we had complaints from devs about unreadable reviews, or in your experience, do devs often miss comments? [15:38] i'm trying to understand if this is in response to an existing problem [15:38] intellectronica: I mean that in an initial review, and in replies, it makes sense to me to remove unnecessary sections of the diff. [15:39] barry: No. I just consider it good protocol, on any ML, to only quote sections that I want to reply to. [15:39] abentley: +1 [15:40] i think then a reasonable policy would be to leave it up to the reviewer, with mentats deferring to what works best for their mentor [15:40] abentley: right, so i did get you, and as i said, i wouldn't benefit from having this done by others, and it would be more work for me, so -1 on a policy that requires doing that [15:40] I think it should be down to the reviewer's discretion [15:40] barry: +1 [15:40] intellectronica: Are you okay with a policy that *permits* it? [15:40] IOW, status quo [15:40] abentley: sure [15:41] Because right now, policy *forbids* it, IIUC. [15:41] only for mentees [15:42] barry: I'm happy with the policy you proposed. [15:43] [VOTE] allow reviewers to trim initial reviews at their desecration, mentats defer to mentor choice. +1 agree, -1 disagree [15:43] Please vote on: allow reviewers to trim initial reviews at their desecration, mentats defer to mentor choice. +1 agree, -1 disagree. [15:43] Public votes can be registered by saying +1/-1/+0 in the channel, private votes by messaging the channel followed by +1/-1/+0 to MootBot [15:43] E.g. /msg MootBot +1 #launchpad-meeting [15:43] +1 [15:43] +1 received from barry. 1 for, 0 against. 0 have abstained. Count is now 1 [15:44] +1 [15:44] +1 received from BjornT. 2 for, 0 against. 0 have abstained. Count is now 2 [15:44] +1 [15:44] +1 received from bigjools. 3 for, 0 against. 0 have abstained. Count is now 3 [15:44] +1 [15:44] +1 received from abentley. 4 for, 0 against. 0 have abstained. Count is now 4 [15:44] +1 [15:44] +1 received from gmb. 5 for, 0 against. 0 have abstained. Count is now 5 [15:44] +1 [15:44] +1 received from bac. 6 for, 0 against. 0 have abstained. Count is now 6 [15:44] + [15:44] +1 [15:44] +1 received from sinzui. 7 for, 0 against. 0 have abstained. Count is now 7 [15:44] +1 [15:44] +1 received from allenap. 8 for, 0 against. 0 have abstained. Count is now 8 [15:44] +1 [15:44] +1 received from EdwinGrubbs. 9 for, 0 against. 0 have abstained. Count is now 9 [15:44] +1 [15:44] +1 received from intellectronica. 10 for, 0 against. 0 have abstained. Count is now 10 [15:45] [AGREED] allow trims of initial reviews at reviewer discretion, mentats to follow mentor preferences [15:45] AGREED received: allow trims of initial reviews at reviewer discretion, mentats to follow mentor preferences [15:45] we're out of time. thanks for bringing this up abentley, so... [15:45] #endmeeting [15:45] Vote is in progress. Finishing now. [15:45] Final result is 10 for, 0 against. 0 abstained. Total: 10 [15:45] Meeting finished at 09:49. [15:45] thanks everyone, have a good day [15:45] cheerio! [15:45] thanks barry [15:45] Thanks. [15:45] Ta barry. === Moot2 is now known as MootBot === salgado is now known as salgado-lunch === salgado-lunch is now known as salgado === salgado is now known as salgado-afk === salgado-afk is now known as salgado === salgado is now known as salgado-afk