ReviewerMeeting20100120

Not logged in - Log In / Register

ReviewerMeeting20100120

summary

actions

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

asiapac

There was no asiapac meeting due to LCA.

ReviewerMeeting20100120 (last edited 2010-01-25 14:11:55 by bac)