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