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

[exarkun at twistedmatrix.com]

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.

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