[08:58] #startmeeting [08:58] Meeting started at 08:58. The chair is mrevell. [08:58] Commands Available: [TOPIC], [IDEA], [ACTION], [AGREED], [LINK], [VOTE] [08:58] Welcome to February's Launchpad users meeting! [08:59] [TOPIC] Roll call [08:59] New Topic: Roll call [08:59] Let's get an idea of who's here for the meeting. If you're here to take part, say me. [08:59] me [09:00] Okay, let's assume you're shy :) [09:01] We'll move straight onto user questions, as it appears there aren't many people here for the meeting [09:01] [TOPIC] User questions [09:01] New Topic: User questions [09:01] We don't have any user questions on the agenda, so if you have a question or suggestion or comment for the Launchpad team, go ahead! [09:03] okay, if you do have a question or comment for the Launchpad team, you can find someone from the team just about 24 hours a day in #launchpad. Otherwise mail feedback@launchpad.net. [09:03] #endmeeting [09:03] Meeting finished at 09:03. [09:03] mrevell: that was short [09:03] k [09:04] jamesh: Yeah, if no one says "me" in the roll call I tend not to drag it out. === salgado is now known as salgado-afk === salgado-afk is now known as salgado === mrevell is now known as mrevell-lunch === mrevell-lunch is now known as mrevell [15:00] me [15:00] #startmeeting [15:00] Meeting started at 15:00. The chair is barry. [15:00] Commands Available: [TOPIC], [IDEA], [ACTION], [AGREED], [LINK], [VOTE] [15:00] hello everyone and welcome to this week's ameu reviewer's meeting [15:00] who's here today? [15:00] me [15:00] me [15:00] me [15:00] me [15:00] gmb has problems with his internet connection atm [15:01] BjornT: cool, thanks [15:01] me [15:01] me [15:01] me [15:01] me [15:02] salgado: ? [15:02] allenap is off today i think [15:02] me [15:02] schwuk: ? [15:02] me [15:03] sorry - was concentrating on another channel [15:03] me [15:03] his first Soyuz review probably put him off ;) === barry changed the topic of #launchpad-meeting to: agenda [15:03] bigjools: :P [15:03] Sorry - I've been having connectivity issues === barry changed the topic of #launchpad-meeting to: Launchpad Meeting Grounds | Channel logs: http://irclogs.ubuntu.com/ | Meeting Logs: Logs available at http://kryten.incognitus.net/mootbot/meetings/ [15:03] [TOPIC] agenda [15:03] New Topic: agenda [15:03] * barry needs more coffee [15:03] == Agenda == [15:03] * Roll call [15:03] * Next meeting [15:03] * Action items [15:03] * Queue status [15:03] * Mentoring update [15:03] * Review process [15:03] * sabdfl request: make sure enums to be in the same interface files as the schemas where they are expressed. [15:04] * thumper - unwrapping import statements (line wrapping that is) [15:04] [TOPIC] next meeting [15:04] New Topic: next meeting [15:04] same time and place? anyone know they won't be here? [15:04] * bigjools will be sprinting [15:04] but I might make it [15:05] k, thanks [15:05] sprinting too [15:05] * jtv same [15:05] quite a lot of people will be sprinting next week [15:05] good point [15:05] which sprint is it? [15:05] team lead sprint? [15:05] Team Leads and Soyuz [15:05] oh, soyuz too? [15:05] yarp [15:05] should we still have a meeting then? [15:06] Soyuz Guy #3 (tm) is starting next week [15:06] and bzr [15:06] not that we have any bzr people in this meeting [15:06] me me me [15:07] flacoste: right. i think mwhudson is the only guy who's gonna be at the next asiapac meeting ;) [15:07] well, i guess we'll still have the ameu meeting. if nobody shows up, it'll be blissfully short :) [15:07] [TOPIC] * Action items [15:07] New Topic: * Action items [15:08] * (continued) intellectronica to put cover letter draft on wiki [15:08] i think this was done, right? [15:08] barry: yup [15:08] url? [15:08] https://launchpad.canonical.com/ReviewRequestTemplate [15:08] intellectronica: awesome! can you email the link? [15:08] can we link off PR to that too - I have enough bookmarks already :) [15:09] bigjools: +1 [15:09] intellectronica: do you mind adding that link and emailing it to the launchpad list? [15:09] barry: sure, i'll do that [15:09] thanks! [15:10] shouldn't sumit-review [15:10] modified to use that template? [15:10] [ACTION] intellectronica to email link to new review template and add a link to PR [15:10] ACTION received: intellectronica to email link to new review template and add a link to PR [15:10] flacoste: let's get some feedback on it, but then, probably so [15:10] * bigjools just added it to PR [15:10] great, thanks! [15:11] * (continued) sinzui to look into outputting PR stanza by default in `review-submit` [15:11] I submitted my change to automatically output the PR block. I wait for mwhudson's approval. [15:11] sinzui: rock on! [15:11] [TOPIC] * Queue status [15:11] New Topic: * Queue status [15:12] general queue is clear [15:12] and only 3 needs-review branches in the pink [15:12] queue looks pretty good, any comments? [15:12] the general queue is clear, but we've had to reject a branch of jml's [15:13] intellectronica: ah yes, i see it. has jml been notified? [15:13] i think something went wrong with his submissions, since there are quite a few of them on the mailing list with no wiki entries [15:13] yes, danilo and i wrote to him [15:13] intellectronica: great, thanks [15:14] any other comments about the queue? [15:14] 5 [15:14] 4 [15:14] 3 [15:14] 2 [15:14] 1 [15:14] [TOPIC] * Mentoring update [15:14] New Topic: * Mentoring update [15:15] nothing from me here... [15:15] 5 [15:15] I think schwuk needs to get more reivews. I think he has had only two since he started. [15:15] mentoring with gavin is going well, but slow. please allocate him some branches if you clear out the general queue. [15:16] sinzui, bac good points, thanks [15:16] sinzui: i think gavin has done two also [15:16] so we should favor mentorees when assigning general queue branches? [15:16] +1 [15:16] +1 [15:16] +1 [15:16] but does it happen often enough? [15:16] sinzui: I'm going to try going on call this Friday, so I should get more then. [15:17] schwuk: Fab [15:17] as long as we mentorees don't get bombarded with branches [15:17] if gavin does any reviews next week i may need to ask one of you to mentor it, since i'll be sprinting. [15:17] [AGREED] favor (but don't overwhelm) mentorees when assigning general queue branches [15:17] AGREED received: favor (but don't overwhelm) mentorees when assigning general queue branches [15:17] bac: I'm happy to mentor if needs be. [15:17] bigjools: unlikely. it really doesn't happen very often these days [15:17] gmb: thanks [15:18] intellectronica: true - most of them are done on-call [15:18] i'll need someone to help mentor schwuk during pycon, but that's not for a few weeks yet [15:18] er, sorry, bigjools [15:19] we can sort that out nearer the time [15:19] yep! [15:19] anything else? [15:19] 5 [15:19] 4 [15:19] 3 [15:19] 2 [15:19] 1 [15:19] [TOPIC] * Review process [15:19] New Topic: * Review process [15:20] * sabdfl request: make sure enums to be in the same interface files as the schemas where they are expressed. [15:20] so mark was pretty clear that we reviewers need to be more aware of where interfaces are defined [15:21] i moved an interface from person.py to teammembership.py because it was in the wrong place [15:21] so he just wanted me to communicate to you to keep an eye out for these things while reviewing code [15:21] (over) [15:22] barry: That reminds me that there was never an announcement that ISchemas can be defined in browser.* [15:22] * thumper - unwrapping import statements (line wrapping that is) [15:22] barry: did you mention it to the developer who put it in person originally? [15:22] bac: i did not [15:22] bac: i think mark's point was more that reviewers need to be on guard for this [15:23] barry: true, but it might have been a good "teachable moment." [15:23] sinzui: i don't remember that decision specifically [15:23] sinzui: it's still status quo on this i think [15:23] bac: true [15:24] barry: i'm probably the one at fault here, since I took care of the move of all generic launchpad ones [15:24] barry: i probably put it person.py to avoid a cyclic import problem [15:24] or maybe i'm just fabulating [15:24] or maybe it was me [15:24] bzr annotate [15:24] flacoste: no it makes some sense, because it was actually /used/ in person.py [15:25] but it was easy to avoid the circular import [15:25] sinzui: if it's worth it (i don't think it is) [15:25] I don't think it is worth it [15:25] or if you're curious :) [15:26] anything else on this topic? [15:26] ... [15:26] * thumper - unwrapping import statements (line wrapping that is) [15:26] a big hairy +1 from me for that [15:26] so in the meeting agenda thumper suggests putting multi-line imports each on a separate line [15:26] * barry hasn't actually talked to him about htis [15:27] it occurs to me too that __all__ has the same issue [15:27] i think this is crack [15:27] -1 [15:27] that's crack [15:27] this is to avoid conflicts [15:27] it's a nice idea, but the risk is that when merging we'll get too many imports that aren't being used [15:27] but these are trivial conflicts [15:27] bug salgado said he didn't like this in a recent review of mine [15:27] live with it [15:27] also, these conflicts are easy to solve [15:27] -0 [15:27] it's not the conflicts that are the problem [15:27] and it uses a lot of screen estate [15:27] it's the diff [15:27] I can live with that in the __all__, but not in imports [15:28] * barry agrees with salgado [15:28] working out wtf changed when 4 lines are reformatted [15:28] bigjools: using meld will solve that one. [15:28] (or something similar) [15:28] who cares about these changes? [15:28] we have make lint to see if they are spurious [15:28] bigjools, make lint should tell you if the changes are correct or not [15:28] fair point [15:29] [VOTE] one __all__ entry per-line but not per multiline import [15:29] Please vote on: one __all__ entry per-line but not per multiline import. [15:29] 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:29] E.g. /msg MootBot +1 #launchpad-meeting [15:29] -1 [15:29] -1 received from flacoste. 0 for, 1 against. 0 have abstained. Count is now -1 [15:29] -1 [15:29] -1 received from salgado. 0 for, 2 against. 0 have abstained. Count is now -2 [15:29] -1 [15:29] -1 received from gmb. 0 for, 3 against. 0 have abstained. Count is now -3 [15:29] i don't care much either way, as long as we're consistent. if we're going to change the coding style, we should change all imports to conform with it. i don't thing that's worth the trouble [15:29] -1 [15:29] -1 received from sinzui. 0 for, 4 against. 0 have abstained. Count is now -4 [15:30] -1 [15:30] -1 received from bac. 0 for, 5 against. 0 have abstained. Count is now -5 [15:30] -1 [15:30] -1 received from BjornT. 0 for, 6 against. 0 have abstained. Count is now -6 [15:30] +0 [15:30] Abstention received from bigjools. 0 for, 6 against. 1 have abstained. Count is now -6 [15:30] okay, i think you all misunderstood the sense of my question, but we'll just say "i know what you mean" :) [15:30] the -1's are /against/ one-line per import [15:30] so... [15:30] -1 [15:30] -1 received from barry. 0 for, 7 against. 1 have abstained. Count is now -7 [15:30] -1 [15:31] -1 received from intellectronica. 0 for, 8 against. 1 have abstained. Count is now -8 [15:31] -1 [15:31] -1 received from schwuk. 0 for, 9 against. 1 have abstained. Count is now -9 [15:31] [AGREED] thumper's suggestion of one-line per import is rejected [15:31] AGREED received: thumper's suggestion of one-line per import is rejected [15:31] [TOPIC] free-for-all [15:31] Vote is in progress. Finishing now. [15:31] Final result is 0 for, 9 against. 1 abstained. Total: -9 [15:31] New Topic: free-for-all [15:31] Denied [15:32] i'm done, anybody have anything not on the agenda? [15:32] nope [15:32] one quick thing [15:32] I'd like to suggest that when large branches are submitted, they are looked over for how they can b broken into smaller branches [15:33] sinzui: shouldn't that happen before? [15:33] * barry looks sheepish [15:33] I had a looks a one of celso's branches, and saw four separate deliverables. [15:33] i noticed 'rocketfuel-branch' no longer updates trunk. this led to a much larger than expected diff i generated to review. just mentioning [15:34] it's down to the dev to manage this - if the review doesn't like it the branch should be rejected. The rules have been well announced. [15:34] s/review/reviewer/ [15:34] jtv: it should, yes. but people will submit large branches anyway. [15:34] bigjools: +1 [15:34] bigjools: True, but the chances of getting parts reviewed and landed is better than a giant branch [15:35] for big branches, i think the reviewer should have to explain why the branch couldn't be split up [15:35] sinzui: that's why we have the 800 line rule though isn't it? [15:35] bigjools: I was able to review two parts of celso's large branch and he landed them before you and schwuk got the backend and frontend pieces [15:35] sinzui: what about the time expended helping get the branch split? That's taken from other reviews. [15:35] Maybe review-submit should wc -l the diff and warn the dev before they submit something oversized. [15:36] schwuk: I spent 10 minutes to find the parts [15:36] schwuk: i think the idea is that the reviewer will learn how to split up branches, so he won't do the same mistake in the future. [15:36] sinzui: if it makes it that future branch are smaller that's good [15:36] My core point is that during the preimp, the bug/spec needs to break the branches down to sane sizes [15:37] branch size is something I try to stay aware of during the branch's whole lifetime so I am not surprised when I submit it [15:37] sinzui: +1 [15:37] I think in celso's case, he could have delivered the parts through out the week instead of on one day [15:38] pre-imp should discuss this as part of the "ready to code" criteria [15:38] * sinzui 's branch in pending-reviews is special. there are no code or test changes. [15:39] and [15:39] i'd like to recall a point [15:39] big branches needs an arrangement with a reviewer [15:39] *before* they are submitted [15:39] i think we should just reject branches over 800k as a matter of policy [15:39] 800 lines [15:39] flacoste: +1 [15:40] Especially for oncall work. [15:40] and leave the dev to find a reviewer [15:40] 800k lines or 800 lines? [15:40] So 800+ in the general queue is forbidden? [15:40] +1 [15:40] It dies up a reviewer for far too long. [15:40] flacoste: i would support that (even as a recent offender) IF lpreview complained bitterly about > 800 line diffs [15:40] s/dies/ties [15:40] gmb: it can kill them too :) [15:40] larger than 800 lines is ok [15:40] barry: It should be fairly easy to patch lpreview to do that. [15:40] let there be a --isuck flag [15:40] if you have a reviwerer that commited to it [15:40] * gmb is looking at the source now. [15:40] this has been a policy for some time now [15:40] but we didn't really enforce it [15:40] now is probably time to do it [15:41] * sinzui branches lpreview for 800 line slap test [15:41] so > 800 with prior agreement with a reviewer only? [15:41] lpreview will only submit a 800 lines patch if the -r option is used [15:41] yes [15:41] sinzui: lpreview/__init__.py:313 would be an ideal place for the slap code. [15:41] bigjools: yes [15:41] flacoste: sounds good to me [15:41] sinzui or gmb: who wants the action item on this one? hack lpreview to enforce (with manual override) 800 line limit? [15:42] idea being that the reviewer will be able to help split the branch earlier [15:42] barry: I'll take it. [15:42] gmb: thanks! [15:42] barry: no manual override [15:42] the manual override is the -r option [15:42] no manual [15:42] [ACTION] gmb to hack lpreview to enforce 800 line limit [15:42] ACTION received: gmb to hack lpreview to enforce 800 line limit [15:42] -r being the reviewer who agreed to review the branch [15:42] flacoste: +1 [15:42] flacoste: using -r has a disadvantage, though. [15:42] if the review is not specified and the diff is > 800 lines, die [15:43] should lpreview also have a -mentor argument? [15:43] BjornT: what is it? [15:43] or at least a -cc [15:43] it's not uncommon to ask a reviewer for a review, without telling him how big the diff is [15:43] schwuk: Yes. I think there's abug for that already., [15:43] lol [15:43] BjornT: right! [15:43] BjornT: what's your suggestion? [15:43] flacoste: manual override :) [15:43] BjornT: --isuck [15:44] this shouldn't happen often, so supplying an extra argument shouldn't be a problem [15:44] I think the implicit meaning of -r should suffice [15:45] sinzui: i think BjornT has a point though: i might add -r without being warned about branch size [15:45] I regularly use -r to direct the mail to the reviewer though [15:45] i think even if -r is specified there should be an explicit acknowledgment the branch is too big [15:45] sinzui: Ah, but people use that when subitting stuff for PR... [15:45] sinzui: how often do you ask how big the diff is before accepting to review a branch? [15:45] i'd rather, first get the warning [15:45] you need a new option [15:45] bac: +1 [15:45] okay, we're out of time for this meeing! gmb it's in your court now [15:45] "You're submitting a 1200 line diff for review. Don't be silly now. Are you sure? [y/N]" [15:45] barry: Okay. [15:45] #endmeeting [15:45] Meeting finished at 15:45. [15:46] thanks everyone! [15:46] sinzui: branches over 800 should not be common, so no need to optimize for that case. a bit extra work shouldn't be a problem. [15:46] thanks barry [15:46] thanks barry [15:47] thanks baw === Edwin is now known as EdwinGrubbs === salgado is now known as salgado-lunch === salgado-lunch is now known as salgado === jamesh_ is now known as jamesh