L. David Baron wrote:
[ responding to the two months worth flood of email that just
resulted from https://bugzilla.mozilla.org/show_bug.cgi?id=891906 ]
On Tuesday 2013-07-09 12:14 -0700, Taras Glek wrote:
a) Realize that reviewing code is more valuable than writing code as
it results in higher overall project activity. If you find you can't
write code anymore due to prioritizing reviews over coding, grow
more reviewers.
Agreed, as long as the reviews are for things that we actually agree
are important.
b) Communicate better. If you are an active contributor, you should
not leave r? patches sitting in your queue without feedback. "I will
review this next week because I'm (busy reviewing ___ this week|away
at conference). I think bugzilla could use some improvements there.
If you think a patch is lower priority than your other work
communicate that.
c) If you think saying nothing is better than admitting than you
wont get to the patch for a while**, that's passive aggressiveness
(https://en.wikipedia.org/wiki/Passive-aggressive_behavior). This is
not a good way to build a happy coding community. Managers, look for
instances of this on your team.
I think there's a distinction between review requests: some of the
review requests I recieve are assertions "I believe this code is
right, could you check?". Some of them aren't; they're "this seems
to work, but I really have no idea if it's correct; is it?".
I added a low-priority flag request to
https://wiki.mozilla.org/Code_Review#Bugzilla_Improvements
I agree that we should focus on clear rules for good patches. If a patch
author doesn't quite know how to accomplish something, that's more of f?
or a low-priority flag.
I think we should perhaps be able to have an expectation of fast
response on the first set of review requests, but I don't think we
should have an expectation of fast response on the second half,
since many of them require the reviewer to do more work than the
patch author. (I think I get a pretty substantial number of this
form of review requests, at least when counting percentage of time
rather than percentage of requests.)
But sometimes it's also not clear which category the review request
is in, or sometimes it's somewhere in-between. (Maybe we should ask
people to distinguish the types? Should people then be embarrassed
to get a review- on a patch of the first type where they're told to
go back to the drawing board?)
Or maybe I should just summarily minus review requests that appear
to be of the second form, perhaps with pointers as to how the patch
author should learn what's needed to figure out the necessary
information?
I also agree with Boris's comments about things that patch authors
should do to make patches easier to review. I should probably be
better about using review- when patch authors don't do these things,
though I often feel bad about doing that when I've been away for a
week and spent a few days catching up, and it's a patch that's
already been sitting there for ten days. I guess I should just do
it anyway.
My list of things would be:
* make the summary of the bug reflect the problem so that there's a
clear description of what the patch is trying to fix
* split things into small, logical, patches
* write good commit messages that describe what's changing between
old and new code (which, if it can't be summarized in less than
about 100-150 characters, should have a short summary on the
first line and a longer description on later lines)
* write good code comments that describe the state of the new code,
and if the patch is of nontrivial size, point to the important
comments in the non-first lines of the commit message
Would you mind reworking above wiki with your points?
Taras
-David
_______________________________________________
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform
