[Twisted-Python] Development Process Failure

[glyph at divmod.com]

On 03:05 pm, exarkun at divmod.com wrote:
>One possibility is to explicitly adjust the review guidelines and 
>direct
>reviewers always to verify that previous review points have actually 
>been
>addressed.  What ideas do other people have?

For starters, I think we should have a division between process 
"recommendations" and process "requirements".  It would be nice to make 
the requirements page as lightweight as possible, and then have a large 
library of recommendations (like these) for developers to peruse.

Even if we encourage that everyone follow the recommendations as well as 
the requirements, "requirements" sounds like an imposition but 
"recommendations" sounds helpful ;-).

On to the issue at hand: reviewers should always check to make sure that 
previous points have been addressed.  I always try to do that already. 
As you said, the value of the review is lost if the author fails to 
respond to it.  In fact, re-reviewers should take care to avoid adding 
unnecessary *new* work; one should prefer verifying the previous review 
points to doing new analysis.  Obviously there are always issues that 
can arise in a re-review, but especially in the case of bug fixes, it's 
always possible to keep recommending better and better ways to fix it in 
the review.  It's better to just diligently enforce the first review, 
unless the first reviewer missed a requirement (coding standard 
violation, missing test coverage, missing doc coverage, compatibility 
breakage).  Design issues, especially those spotted past the first 
review, should generally be deferred to later tickets.

However, I think the author should really help the reviewer.  It's a lot 
easier for the author to enumerate their responses to review points than 
for the reviewer to look at the diff, the HEAD, or the revision history 
of the branch.

A few times, when responding to a review, I've tried to address each 
review point with an individual commit, and a 're #XXXX' ticket comment 
in the commit message.  You alluded to such an approach.  If the author 
does this, then verifying that the points have been addressed is 
relatively easy.  exarkun, you've done the same thing, and when 
reviewing those tickets I can say that I *really* appreciated it.  It's 
a good habit to get into.

Actually, it would be nice if the "re #XXXX" weren't necessary.  Ideally 
every branch would have the commit history of all of its branches 
interspersed in its comment log.  But that's more trac hackery and 
possibly not worth it.

If it's too difficult for the reviewer to identify where the author has 
responded to feedback, I think the reviewer should feel free to reject 
the re-review by saying "please make a list of revisions where you've 
addressed each of these points and put this back into review".

However, as implied by my suggestion for requirement/recommendation 
division, I don't think the author should necessarily *have* to do this. 
In many cases I've found it pretty easy to verify that the review has 
been addressed.  They should just be responsible for doing it in the 
case that the reviewer needs it :).  For example, patches in the tracker 
tend to be smaller than branches, and I've rarely needed this level of 
detail in the tracker.

Also I think we should take it up with authors after the fact, outside 
the bounds of the review process :).  Too often we (and I am very 
consciously including myself in "we" here, I've done this a bunch of 
times) treat a ticket as "over" if it's managed to make its way through 
the review process, and doesn't cause any buildbot failures or otherwise 
need to be reverted.  We should conduct post-hoc meta-reviews in more 
specific cases rather than waiting for general impressions to bother us.

I'm starting to think that we should have a separate mailing list for 
such discussions, or maybe even join one that already exists.  There 
must be a million blogs, forums, and lists for talking about development 
processes.

As for communicating the results of these discussions, the wiki is good 
for recording conclusions, but we can also use labs.twistedmatrix.com 
for drawing attention to process updates and tools, techniques, and tips 
for working on Twisted.