On 7/9/13 3:14 PM, Taras Glek wrote:
Conversely, poor code review practices hold us back.
Agreed. At the same time, poor patch practices make reviews _much_
harder. We should generally expect good patch practices from
established contributors; obviously expecting them from new contributors
is not reasonable.
I believe that getting fast review turnaround is a collaboration between
the reviewer and review requester; it shouldn't be solely on the
reviewer to do a fast review no matter how hard the requester makes it.
More on this below.
a) Realize that reviewing code is more valuable than writing code as it
results in higher overall project activity.
I agree that it results in higher activity. Whether it results in
overall higher value depends on the activity. But as a first cut, I
agree with this claim.
If you find you can't write
code anymore due to prioritizing reviews over coding, grow more reviewers.
Easier said than done, of course. ;) Especially for people who get
flagged to review abandoned code because there is no one else who is
willing to learn it.
b) Communicate better.
Yes, absolutely.
The flip side of this is "don't request review from people who are
labeled as being away in Bugzilla and expect fast turnaround". I really
wish Bugzilla could let me flag myself as not available for reviews when
I'm on vacation, say. Expecting people to comment about being on
vacation while on vacation is, imo, not reasonable.
Does anyone disagree with my 3 points above?
Modulo the caveats above, no, but I would like to add some points about
what makes a patch easier to review. A lot of our contributors are not
very good on these points:
* Split mass-changes or mechanical changes into a separate patch from
the substantive changes.
* If possible, separate patches into conceptually-separate pieces for
review purposes (even if you then later collapse them into a single
changeset to push). Any time you're requesting review from multiple
people on a single huge diff, chance are splitting it might have been a
good idea.
* Actually address the review comments before requesting another review.
It's very common to just ignore (miss?) some of the review comments,
which means the reviewer then needs to triple-check that all the things
they pointed out got fixed.
* When requesting a second review on a patch, provide an interdiff so
that the reviewer can just verify that the changes you made match what
they asked for. Bugzilla's interdiff is totally unsuitable for this
purpose, unfortunately, because it fails so often.
* When requesting review or feedback on just part of the patch, make
that very clear.
All of the above are a consequence of a simple observation: time to
review, except for simple mass-changes, is nonlinear in patch size. For
a single review pass, I would say it's probably quadratic in most cases.
Since the number of review passes is itself typically non-constant in
patch size, this means that the common modus operandi of posting a huge
diff with a bunch of issues, then addressing some of the review comments
and posting another huge diff, etc, takes up a huge amount of reviewer
time, most of it basically wasted.
-Boris
_______________________________________________
dev-platform mailing list
[email protected]
https://lists.mozilla.org/listinfo/dev-platform
