[Twisted-Python] Development Process Failure
Hey all,
Recently I've been bothered by a systematic shortcoming of the Twisted development process. When changes are made in response to a review, they are generally made in a way which is difficult to inspect. The value of a review is lost if the valid points raised by it are not addressed before the changes are applied to trunk. Often, there's no problem here, but it sometimes happens that /not/ all of the review points are addressed before a ticket is re-submitted for review. In these cases, one of two things happens. The re-reviewer might go back to the previous review and verify that all of the points raised in it have been addressed. This is a time consuming process, though it can be made easier if the author responds to each review point (and even easier if a changeset is linked in each response). Or, the re-reviewer might assume that the author addressed all points and just look over the branch for anything missed in the previous review.
In either of these cases, time is being wasted, but in the latter case, time is being wasted /and/ bad code is being added to trunk. I would like to consider how we might address this shortcoming of the development process.
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?
Jean-Paul
On Tue, Feb 24, 2009 at 2:05 AM, Jean-Paul Calderone exarkun@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?
In the Launchpad team, we replying to review mails just like usenet posts, and there is a convention that each point raised by the reviewer has to be addressed. When looking at someone's reply to your review, the mail quoting makes it obvious if someone has missed a point.
Given Trac, maybe numbering review points + being direct about the change would make it easier to inspect replies, which must in turn mention the numbered points. For this to work, the reviewer would have to clearly distinguish between "I would like you to change X" and "Looking at A inspires the though B."
Finally, Launchpad has an arrangement such that branches with over 500 lines of diff need to have their reviews arranged specially (you have to find a reviewer who will agree to it, and we're allowed to say "no"). Everything is easier when the diff is smaller.
jml
On 09:39 pm, jml@mumak.net wrote:
Given Trac, maybe numbering review points + being direct about the change would make it easier to inspect replies, which must in turn mention the numbered points. For this to work, the reviewer would have to clearly distinguish between "I would like you to change X" and "Looking at A inspires the though B."
Here's a thought: every review should have 3 sections. "According to process requirements, you must change XYZ", "I would like you to change XYZ", and "here are some things to think about".
I know that as I respond to a review, I know I'd like to have those things clearly separated out.
On Tue, Feb 24, 2009 at 10:07 AM, glyph@divmod.com wrote:
On 09:39 pm, jml@mumak.net wrote:
Given Trac, maybe numbering review points + being direct about the change would make it easier to inspect replies, which must in turn mention the numbered points. For this to work, the reviewer would have to clearly distinguish between "I would like you to change X" and "Looking at A inspires the though B."
Here's a thought: every review should have 3 sections. "According to process requirements, you must change XYZ", "I would like you to change XYZ", and "here are some things to think about".
I know that as I respond to a review, I know I'd like to have those things clearly separated out.
As long as they are clearly communicated, I don't care about the ordering.
jml
On 03:05 pm, exarkun@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.
participants (3)
-
glyph＠divmod.com -
Jean-Paul Calderone -
Jonathan Lange