=== mrevell-lunch is now known as mrevell === salgado is now known as salgado-afk === salgado-afk is now known as salgado-lunch [15:00] #startmeeting [15:00] hi -- welcome to the AMEU reviewer's meeting. who is here? [15:00] me [15:00] me [15:00] where is mootbot? [15:02] sinzui, EdwinGrubbs, mars, salgado-lunch, bigjools, allenap`: ping [15:02] me [15:02] BjornT: ping [15:02] me [15:02] me [15:02] me [15:02] danilos ping [15:03] * allenap` is sprinting too [15:03] me [15:03] me [15:03] allenap`: your cohorts joining us? [15:04] bac: Apologies from all. [15:04] [TOPIC] agenda [15:04] bac: BjornT is sleeping [15:04] * Roll call [15:04] * Action items [15:04] * Verifying that community contributions don't do harm (henninge) [15:04] * Peanut gallery (anything not on the agenda) [15:05] bac, "other teams to review lazr-js branches" from last week? [15:05] [topic] action items [15:05] mars: i'm getting there... [15:05] * Maris to bring up cross-team reviews in the lazr-js task force meeting and report back. [15:05] me [15:05] mars did you have that discussion? [15:05] :) [15:06] bac, done! Zac and Sidnei both would like to start doing lazr-js branch reviews. [15:07] mars: great. [15:07] mars: how will we facilitate that/ [15:08] so, if you have a lazr-js patch, you can ping them on IRC for a review [15:08] as well as the ususal suspects [15:08] mars: what is zac's irc nick? [15:08] mars: do we have up-to-date review guidelines for lazr-js? [15:08] bac: urbanape [15:08] bac, Zac is urbanape, sidnei is sidnei [15:09] flacoste, we are still using the JS guidelines on dev.lp.net [15:09] ok, thanks for following up maris [15:10] * Gavin to start discussion on the ML about doctest size, refactoring, moving corner cases to unittests, etc [15:10] bac: I haven't done that yet; please keep that action. [15:10] allenap`: ok, we'll roll it over [15:10] allenap`: can you try to do that early next week since you're sprinting? [15:11] bac: Yes, absolutely. [15:11] great [15:11] * Gary to do timing tests for try/except, examine current usage of check_permission, and we'll discuss again next week. [15:11] * gary_poster did not do the timeit tests yet. Next week. [15:11] gary_poster: any progress? [15:11] ok [15:11] * Curtis to land a import cleanup branch and then reviewers will enforce zero tolerance for introducing new issues. [15:11] sinzui: you did get that branch landed, correct? [15:11] done [15:11] great [15:12] while OCR yesterday we discovered one outstanding but it should be fixed when branches land today or tomorrow [15:13] we have one new item today from henninge [15:13] * Verifying that community contributions don't do harm (henninge) [15:13] ? [15:13] yes [15:13] henninge: the floor is yours [15:13] IThis came up after a review I did for a community developer on Monday. [15:13] Heeding danilos reminder mail I checked that he had had a pre-imp discussion with an LP developer. [15:13] It turned out though, that the team of the affected application (soyuz) did not agree with the change, at least not the way it was done. [15:13] So, one lesson to learn would be to make sure the pre-imp was with some-one from the right team that would have domain knowledge. [15:14] But I am wondering if we should also rquire a second "sanity-check" review from a developer of the affected team. [15:14] Not a second code review. [15:14] To me, that would be the clearest sign that all agree. [15:15] henninge: in the past, before we had community contributions, we tried to do the opposite -- pre-imps across the team. but for community fixes perhaps it is a good idea. [15:15] I think that in the soyuz case it requires a lot more domain knowledge [15:15] i'm not sure pre-imp across the team is a sane choice anyway [15:15] and I would encourage a pre-imp with a soyuz dev even if the dev was an LP team member [15:15] pre-impl is where you want the more domain knowledge [15:16] so it makes most sense to do within team [15:16] i agree with bigjools with the extension to every domain [15:17] while we always encourage doing pre-imp calls do we agree they should be mandatory for community contributions? [15:17] yes [15:17] +1 [15:17] +1 [15:17] unless we give someone an exception [15:17] I can think of one person who knows quite a lot :) [15:17] as long as exceptions are only given in a pre-imp call [15:18] agreed [15:18] [agreed] community contributions require a pre-imp call [15:19] a pre-imp call with a domain dev [15:19] henninge had the further proposal of a second domain-specific review. thoughts? [15:19] there is still the chance that what was discussed in the pre-imp does not end up in the actual code. [15:19] I think that the first pre-imp should be with the domain specialist [15:20] not out of malice, just misunderstandings etc. [15:20] we don't need 2 pre-imps [15:20] a late +1 for me :) [15:20] bigjools: not 2 pre-imps [15:20] bigjools: yes, that was implied. we aren't talking about two pre-imps [15:20] ok [15:20] we are talking about that each mp from a community developer should have been looked at by a domain dev. [15:21] henninge: +1 [15:21] not for a full code review but just to make sure it's sane. [15:21] henninge, my concern there would be just that reviewers check that nothing was implemented which is not agreed upon (i.e. permission changes on certain objects, which is what I've seen) [15:21] +1 [15:22] at this time, I think that's sane, but as we get more contributions, it will become unwieldy [15:22] so +1/+0 from me (+1 now, +0 for the future :) [15:22] danilos: how does the reviewer know what has been agreed upon? [15:22] I have already had one branch to stole a day of my life [15:22] bac, i.e. reading a bug report [15:22] danilos: that's great if the bug report is specific enough [15:23] i think we could use some discretion [15:23] Maybe it's a case-to-case decission. [15:23] the reviewer could ask the pre-impl person to have a look [15:23] bac, at least that was the case with one example where I've seen it fail; bug report was very specific about what fields should be exposed, and branch exposed others as well [15:23] if the domain specialist does the pre-imp they should do the sanity check review, and it can be up to them how thorough it is [15:23] if he sees it as a tricky change [15:23] flacoste, this was a very simple change, for example [15:23] danilos: ok, that's cause for concern [15:23] barry, yeah, agreed [15:23] well [15:23] to be honest [15:24] i think the mistake that happened there could have been done by any lp dev [15:24] not familiar with the domain [15:24] so it's not a big change [15:24] my point also [15:24] bac, though, I believe pre-imp would have caught this, so I don't think it's cause for another review, or a problem with the review [15:24] flacoste: but we usually just code within our domain. [15:24] more or less [15:24] usually [15:24] but not exclusively [15:24] and that's fine [15:24] I know [15:24] and we want more of it [15:24] I like it, too ;) [15:25] to me it seems the guiding principle should be that reviewers should always be open to asking for domain specific help. [15:25] i think a pre-impl with someone with domain expertise is always mandatory [15:25] yeah, anyway, let's not introduce too many barriers; let's assume it's common wisdom that you should suggest a domain-specific reviewer if code looks too hard [15:25] more cross-domain experience would have helped the pre-imp ;) [15:25] flacoste, +1 [15:25] flacoste, though, we agreed on that already :) [15:25] bac: +1 [15:25] yeah [15:25] bac, +1 [15:25] and i like bac formulation [15:26] (I tried to say that, but you put it more nicely :) [15:26] ok, so we have no real action here except to be sensitive to getting domain specific eyes when necessary [15:26] yeah [15:26] moving on... [15:26] well, that and enforcing pre-impl [15:26] on community contrib [15:26] flacoste: well that was done above [15:26] * Peanut gallery (anything not on the agenda) [15:27] anyone? [15:27] bac, flacoste: I'll update the wiki ipage [15:27] henninge: thanks [15:27] iPhone, iGoogle, iPage ;-) [15:27] [action] henning to update wiki page regarding pre-imps for community contributions [15:28] ok, well if there's nothing else we can end the meeting. [15:28] thanks everyone for coming, except you mootbot [15:28] #endmeeting [15:28] :) [15:28] thanks bac [15:28] thanks bac [15:28] thanks bac [15:30] thanks bac :) === salgado-lunch is now known as salgado [15:43] crap, am I 1h late? [15:43] is this meeting not 1545TUC? [15:43] salgado: which? reviewer's? [15:43] yeah [15:45] oh, it's from 1500 to 1545 [15:45] damn [15:45] salgado: sorry, mate ;-) [15:48] bac, although I'm late I had an item in the agenda. did you not see it or just skipped because I wasn't here? === bigjools is now known as bigjools-otp [16:09] salgado: i didn't see it at https://dev.launchpad.net/ReviewerMeetingAgenda [16:10] salgado: perhaps it got removed in an edit. sorry. can you put it back for next week? [16:10] sure, I'll do that === bigjools-otp is now known as bigjools === salgado is now known as salgado-afk === mwhudson_ is now known as mwhudson === mwhudson_ is now known as mwhudson === Chex_ is now known as Chex === mwhudson_ is now known as mwhudson === EdwinGrubbs is now known as Edwin-afk