[ 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 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 -David -- 𝄞 L. David Baron http://dbaron.org/ 𝄂 𝄢 Mozilla http://www.mozilla.org/ 𝄂 _______________________________________________ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
