On 03:05 pm, exar...@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.
_______________________________________________
Twisted-Python mailing list
Twisted-Python@twistedmatrix.com
http://twistedmatrix.com/cgi-bin/mailman/listinfo/twisted-python
