Hello fellow Twisted maintainers,
There are lots of things that could be done to make contributing to Twisted easier and more fun. I know, because people are constantly buttonholing me to tell me about how we should use this or that source hosting or continuous integration website :). However, there's one persistent problem - in my opinion, the largest problem - that I think we could address without revving any of our technology[1] or changing our process (as documented, anyway; there are some changes to it as it is practiced).
That problem is perfectionism; particularly perfectionism in code reviews, but also sometimes on the part of change authors. The "one weird trick" I am referring to in the subject is trusting the process, and not attempting to second-guess it or add extraneous bells and whistles to it because you're worried it may not work.
According to our official code review process, in order to be acceptable to land on trunk, a change must have:
Coding-standard compliance.
Test coverage for all new or changed code.
Docstrings for new or changed code.
Narrative documentation for any new or changed user-visible features.
No backwards-incompatible changes.
A NEWS file.
I think most would agree that reviewers might also reasonably block a change for:
Exploits or other security issues introduced in any new or changed functionality, or
Performance regressions (ideally in existing benchmarks, and not in new performance criteria that the reviewer just invented!) in existing functionality.
Maybe those last two things should be added to the official process documentation.
However, these 8 things really ought to be the only things that ought to cause a reviewer to block a change. Right now, reviewers also routinely check for, and block changes for:
A perfect, future-proof public API for everything new
Any un-handled use-cases
Hypothetical alternative approaches which would yield a more flexible implementation that could enable different functionality in the future
Optimal performance for new functionality
Re-factoring to use existing code if a change introduces duplication
Worse yet, multiple reviewers will sometimes show up on the same ticket, demanding increasingly intricate and sometimes conflicting implementation improvements. This is understandably very frustrating for the author and can make changes get stalled for long periods of time.
So here are some suggestions:
First of all, we do not have to have a perfect public API for everything all the time. You can deprecate stuff later. You can remove stuff later. We have a process to do this: trust it. Sure, it takes a release cycle or two, but that is not nearly the hardship that it seems. Experience has taught us that realistically, neither the reviewer nor the author of a change is going to have the energy to fix up new public APIs to be perfect within the time frame of a single release cycle anyway, so you might as well have people using the pretty-good-but-not-perfect API in the meanwhile. So if you're authoring a change that you think might have a better public API but you don't know what it is, just submit for review anyway. If you're reviewing a change that has a public API that you think might be made better with a bunch of additional work, just approve it and ask the author to file a ticket for improving it in the future.
Secondly, it's OK to do a code review that isn't perfect. I suspect that one reason so many code reviews are so nitpicky is that nobody wants to be the one to approve a change that gets reverted. My advice here is: don't worry about it. If you're reviewing some code, make a good-faith effort to find the issues with it according to the documented criteria, but if you don't find any problems, just approve it, as the process says you should. If you make a mistake, that's why we have a process for reverting changes. If you want another reviewer to look at it after the fact, just ask.
Third, code review is not just about the formal requirements; of course there can be discussion about design, API, or implementation techniques. As a reviewer, you may notice anything in my second list, or more, and you can definitely mention it to the author of the change. But you should mention it in a clearly delineated separate section of your review feedback which you explicitly state is not required to accept the change. Always feel free to say "file a ticket for X before landing this" so that we don't lose track of improvements for the future that occur to you as you're reviewing, but don't feel like you have to block a change for any possible flaw.
And finally, if you really don't know whether you want to commit to a public API, there's always the option of putting something in a private implementation module so that it can go into a release and users can start importing it with the understanding that they typed an underscore so they're on their own with regard to compatibility guarantees :-).
In closing, if you can think of a review you've done in the past that asked the author to do too much work above and beyond the requirements, consider going back and amending the review to separate the required stuff from suggestions and feedback you have.
-glyph
[1]: We're also making good progress towards revving that technology, but I'll have more to say about that in a separate post.
