launchpad-dev team mailing list archive

[Brad Crittenden <bac@xxxxxxxxxxxxx>]

= ReviewerMeeting20100120 =

== summary ==

 * Maris suggested to the lazr-js task force that other teams become reviewers for lazr-js reviews.  Sidnei and Zac were eager to participate.  So, feel free to ping sidnei or urbanape for lazr-js reviews.
 * Curtis landed the branch to clean up import violations.  Thanks!  We can now resume our zero-tolerance policy for those warnings.
 * Henning introduced the topic of ensuring community contributions are valid/desirable/sane.  We agreed that 1) pre-implmentation calls are '''required''' for community contributions, 2) the calls need to be done with a Launchpad staff member who works on the given app or has significant domain knowledge, 3) reviewers are encouraged to seek help from domain experts if they need it.  Henning was to update the wiki page with these new ideas.

== actions ==
 * intellectronica to draft guidelines for drive-by cleanups
 * Gavin to start discussion on the ML about doctest size, refactoring, moving corner cases to unittests, etc
 * Gary to do timing tests for try/except, examine current usage of check_permission, and we'll discuss again next week.
 * Henning to update wiki page for pre-imp requirement for community contributions, etc.

== 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.