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