On Jul 1, 2011, at 9:06 PM, exarkun@twistedmatrix.com wrote:

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.

I don't think I disagree, actually. It's just a different emphasis. Issues certainly need to be brought up. I shouldn't have said otherwise. They just need to be clearly separated into blocking/not-blocking.

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 :).

You're right, on a subsequent reading, that's what it sounds like I was saying. And I'm definitely not saying that; the review requirements are the same on every review.

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.

The word "functional" is pretty broad, so let me be more specific.

Any change which is part of the coding standard or the review policy needs to be mentioned. 100% test coverage is a requirement. Docstrings for public and private are a requirement. No matter how many reviewers have missed these things, they need to be brought up and no feature which is missing them may land. I think we are all pretty clear about that.

Beyond that there's an infinite spectrum of diversity of possible review comments, and I can't speak to all of them. For example, maybe a change causes the performance characteristics of an existing, but un-benchmarked, change to regress. Maybe the API is designed in such a way that it will be difficult or impossible to scale. Perhaps something is treated as blocking where it should return a Deferred. I can't say for sure whether one of these issues would or wouldn't be a blocker without a lot of context about that specific change.

But I think there's a general category of feedback that can be classified as "this is OK, but I can think of a way to do it better". In this case, a change might meet all the formal requirements, have a reasonable API that works, and not raise compatibility issues, but the reviewer has an epiphany while reading it and realizes it might be simpler/faster/more flexible to do something else.

The reviewer should always feel free to communicate such an insight, of course, but I think that reviewers should all have a strong bias towards separating this out and making it optional - and making it clear that it's optional. The best should not be the enemy of the better; if it's an improvement, and it works today, we should usually integrate it. In many cases it's much better to defer the improvements until later and get the reasonable API in sooner. I think this is true even if the improved version will have a totally different API, because it's possible nobody will have a chance to do the improvements for years.

Again, this is one that I know I'm guilty of, but I'd like to think that in recent reviews I've been really clear about the optionalness of my ideas :).

The purpose of the review process, as I see it, is not to produce the best possible implementation of everything always. It's to produce an implementation that meets a certain quality standard and does not cause regressions (either in functionality or test coverage, although hopefully one day also in performance too). I think there are cases where we have ended up with no implementation rather than an adequate but not awesome implementation because there have been too many competing ideas for how to do it "even better" rather than how to make it meet the required standard.

All that being said, I spent a long time looking at <http://twistedmatrix.com/trac/report/16>, and I can't find any tickets where comments like this are the only reason for blocking a ticket that's been through many round-trips; they all still have compliance issues. The larger problem is cursory early reviews where the patch is obviously bad, and later reviews that get more detailed once it starts to shape up. Apparently the more serious issue is that we just need to be more thorough earlier in the process. (Although another way to look at that data is that we weren't terribly thorough 3 years ago, since old tickets necessarily have old initial reviews.)

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.

If you're a new reviewer (or even if you're an old one who hasn't gotten much practice), it might be worthwhile to put a review comment on the ticket but ask a more experienced reviewer for a meta-review before getting rid of the 'review' keyword. A meta-review is usually a lot easier than a review, so it should be easier to get one by just showing up in #twisted and asking; hopefully we won't need an elaborate meta-review tracker.

I hope this suggestion will make someone who has thus far been hesitant to do a review contemplate doing one with this set of training wheels.

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.