On 1 Jul, 11:27 pm, glyph@twistedmatrix.com wrote:
On Jul 1, 2011, at 6:57 PM, David Ripton wrote:
Working with patches because you don't have svn commit rights is annoying, but this annoyance is a relatively minor fixed cost.
It's still important for us to reduce this cost; even if it's not the bottleneck, we have to optimize first where we can optimize :).
The real issue, for controversial features, is achieving consensus, and then getting your feature in before consensus is lost.
Yes, absolutely. And there's are some important guidelines for reviewers that can be inferred from this:
Try to stick to coding-standard stuff as much as possible, especially if there's been a previous review. Don't bring up "I think it would be better if..." things, except to say "file an additional ticket".
We've disagreed about this in the past, so I don't think you'll be surprised if I say that I don't think this is a good idea. :) If an earlier review misses *functional* issues with a change, then they need to be brought up. Scope creep should be avoided at *all* stages of the process, but an incomplete first review doesn't exempt a change from the development requirements (and I don't think you think it should, even though it sounds like you're saying it here :).
If there's a previous review, as much as possible, stick to the points brought up in the previous review. Make sure they're addressed, and try not to add a pile of conflicting stylistic suggestions.
Stylistic issues should all be known in advance (read the coding standard, etc) and brought up in the first review (because the first reviewer should know them too). Stylistic issues that aren't covered by the coding standard definitely shouldn't be sprung in a subsequent review (or the reviewer should address them himself or herself) - or even a first review, really. This is a separate case from pointing out *functional* issues that the first review missed.
When you do a review, try to be as thorough as possible. Don't ever do a review that says "update @since markers" or "2 blank lines between methods" and nothing else; at least, you need to say "... and then it will be ready to merge". Remember that when you take it out of review, no other reviewer is going to look at it until the author fixes it and resubmits it, which may be quite a while. If you feel like adding some partial commentary to help the next full reviewer, just add a comment, don't remove the review keyword.
This is very important, since it should reduce the instances in which a later review does have to introduce a new point. I don't think anyone benefits from forgoing resolving functional issues that are detected after the first review but before the change actually lands.
Be explicit about what happens next, even if it's going to be redundant. Always say "... and it will need another review" or "... and then merge". Try not to voice a vague dissatisfaction with the architecture of something without an explicit suggestion about (A) what should be done, and (B) whether it needs to be done before the feature can be merged.
For contributors, one suggestion: make implementation details private as much as possible, so that the reviewer will have to consider the aesthetics of the implementation details less. The smaller the public API of the contribution, the easier it is to avoid bikeshedding around method names and class placement :).
There are plenty of issues on the contribution-accepting side which I don't want to minimize, but I think there's another thing contributors can do to help the overall process. If a review results in more work than you're interested in doing, say so. Make it clear that you're no longer taking responsibility for the ticket. Then there's some chance that another contributor might take it the rest of the way (without waiting 5 years before deciding the original contributor has lost interest).
None of this would have helped in particular on the IPv6 stuff, but given that that affected an extremely core API, and had a ton of fiddly little details, I'm not sure much could have helped on that one...
Sooooooo fiddly aarrrrghhghhhhh.
I know I've broken these rules myself on occasion, and I'd like to encourage other reviewers to call me out on it when they notice it :).
This raises another point, which is that the mailing list isn't a terribly useful place for these points to end up. If anything is actually expected to change, then the need to update the review documentation (such as it is) and probably also get serious about meta- reviews. Jean-Paul