[Twisted-Python] SURVEY: Have you submitted a patch to Twisted and it never got in?

[Glyph Lefkowitz]

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

> On 1 Jul, 11:27 pm, glyph at 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).

+1

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

I do hope someone will volunteer to update the wiki as a result, and if not, I hope I remember to.  But the discussion has given me a couple of ideas I wouldn't have had otherwise; the meta-review comment above is a new(ish) idea.

Of course, the most useful place for this to end up would be as a tweet!  (Is everybody following @twistedmatrix yet?)

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://twistedmatrix.com/pipermail/twisted-python/attachments/20110701/78376971/attachment-0001.htm