= 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.
_______________________________________________
Mailing list: https://launchpad.net/~launchpad-dev
Post to : [email protected]
Unsubscribe : https://launchpad.net/~launchpad-dev
More help : https://help.launchpad.net/ListHelp
