|Version 46 (modified by exarkun, 4 years ago) (diff)|
Twisted Code Review Process
All commits to Twisted's trunk must follow this review process. The only exception is for the Twisted Quotes file. There are no other exceptions!
Both authors and reviewers should be intimately familiar with all requirements on this page.
Authors: Things your branch or patch must contain
- Code which follows the coding standard.
- 100% unit test coverage for all modified and new code (even if it didn't have tests before)
- 100% API docstring coverage for all modified and new code (even if it didn't have docs before)
- No backwards-incompatible changes. Please also be sparing when adding 'public' names to the API, as they must be supported in the future. If it can start with an underscore and not be exposed publicly, it probably should.
- Appropriate new or modified "End User" guide documentation (in the form of Lore-formatted xhtml files in the doc/ directory)
- A file in all relevant "topfiles" directories describing changes which affect users (See below)
Authors: How to get your change reviewed
- There must be a bug in the Twisted bug tracker describing the desired outcome (See below).
- Add the review keyword to the ticket.
- Remove the owner field by selecting the blank option next to "Assign to".
- Alternatively, assign it to someone who has already agreed to review it.
- Write a comment with any relevant information for the reviewer.
Reviewers: How to review a change
- Be familiar with the Twisted codebase, coding standard, and these review requirements.
- Don't be the author of the change (See below).
- Assign the ticket to yourself.
- Review the change, and write a detailed comment about all potential improvements to the branch (See below).
- Remove the "review" keyword from the ticket.
- Assign the ticket back to the author.
- If the change is ready for trunk, indicate so by writing "please merge" or "+1".
- Alternatively, if the author does not have commit access to trunk, merge the change for them.
Authors: How to merge the change to trunk
- Merge the change into a checkout of Twisted trunk and commit it. The message must follow this format (See below).
Merge <branch name>: Brief description Author: <names> Reviewer: <names> Fixes: #<ticket number> Long description (as long as you wish)
- After the change is merged and the buildbots have completed running, delete the branch.
It is now up to the authors of individual changes to write high-level descriptions for their changes. These descriptions will be aggregated into the release notes distributed with Twisted releases. If we just let each author add to the NEWS files on every commit, though, we would run into lots of spurious conflicts. To avoid this, we have come up with a scheme involving separate files for each change.
Changes must be accompanied by an entry in at least one topfiles directory (eg trunk/twisted/topfiles/ or trunk/twisted/names/topfiles/). The entry must be a file named <ticket number>.<change type>. <ticket number> is replaced by the ticket number which is being resolved by the change (if multiple tickets are resolved, multiple files with the same contents should be added). <change type> is replaced by one of the following literal strings:
- feature - for tickets which are adding a new feature.
- bugfix - for tickets which are fixing a bug.
- doc - for tickets primarily about fixing or improving documentation (any variety).
- removal - for tickets which are deprecating something or removing something which was already deprecated.
- misc - for tickets which are very minor and not worth summarizing outside of the svn changelog. These may be empty (their contents will be ignored).
The entry should contain a high-level description of the change suitable for end users. Generally, the grammatical subject of the sentence should be a Python object, and the verb should be something that the object does, prefixed with "now". Here's are some examples:
twisted.protocols.amp now raises InvalidSignature when bad arguments are passed to Command.makeArguments
The new module twisted.internet.endpoints provides an interface for specifying address families separately from socket types.
twisted.trial.util.findObject is now deprecated.
twisted.conch.insults.colors is now deprecated in favor of twisted.conch.insults.helper.
twisted.runner.procmon.ProcessMonitor's active, consistency, and consistencyDelay attributes are now deprecated.
twisted.internet.interfaces.IReactorTime.cancelCallLater, deprecated since Twisted 2.5, has been removed.
You don't need to worry about newlines in the file; the contents will be rewrapped when added to the NEWS files.
See enforcenews.py for the svn pre-commit hook which enforces this policy.
Filing bugs and writing review requests
Tickets should be described well enough that the change is already justified and the new code should be easy enough to read that further explanations aren't necessary to understand it, but sometimes diffs themselves can be more difficult to read than either the old or new state of the code, so comments like the implementation of foo moved from bar.py to baz.py can sometimes make a reviewer's job easier.
Who can review?
Changes must be reviewed by a developer other than the author of the changes. If changes are paired on, a third party must review them. If changes constitute the work of several people who worked independently, a non-author must review them.
A reviewer need not necessarily be familiar with the specific area of Twisted being changed, but he or she should feel confident in his or her abilities to spot problems in the change.
How to be a good reviewer
First, make sure all of the obvious things are accounted for. Check the "Things your branch or patch must contain" list above, and make sure each point applies to the branch. Use pyflakes to check the basic quality of the code. The following command will check all the files modified and added by a branch merge:
svn status | grep '^\M\|^A' | cut -c 8- | xargs pyflakes
A reviewer may reject a change for various reasons, many of which are hard to quantify. Basically, use your best judgement, and don't be afraid to point out problems which don't fit into the list of branch requirements laid out in this document.
Here are some extra things to consider while reviewing a change:
- Is the code written in a straightforward manner which will allow it to be easily maintained in the future, possibly by a developer other than the author?
- If it introduces a new feature, is that feature generally useful and have its long term implications been considered and accounted for?
- Will it result in confusion to application developers?
- Does it encourage application code using it to be well factored and easily testable?
- Is it similar to any existing feature offered by Twisted, such that it might make sense as an extension or modification to some other piece of code, rather than an entirely new functional unit?
- Does it require new documentation and examples?
When you're done with the review, always say what the next step should be: for example, if the author is a committer, can they commit after making a few minor fixes? If your review feedback is more substantial, should they re-submit for another review?
If you are officially "doing a review" - in other words, removing the review keyword - please make sure you do a complete review and look for all of these things, so that the author has as much feedback as possible to work with while their ticket is out of the review state. If you don't have time to do a complete review, and you just notice one or two things about the ticket, just make a comment to help the future reviewer, and don't remove the review keyword, so another reviewer might have a look. For example, say, "I just checked for a news file and I noticed there wasn't one", or, "I saw some trailing whitespace in these methods". If you remove the review keyword, you may substantially increase the amount of time that the author has to wait for a real, comprehensive review, which is very frustrating.
The commit message
Several tools exist which parse commit messages to trunk, so the Author, Reviewer, and Fixes lines should conform to this format exactly. Multiple Fixes lines will close multiple tickets. Refs may also be used to attach the commit message to another ticket which is not being closed. The commit message should also describe the change being made in a modest amount of detail.
Reverting a change
If a change set somehow introduces a test suite regression or is otherwise found to be undesirable, it is to be reverted. Any developer may revert a commit which introduces a test suite regression on a supported platform. The revert message should be as explicit as possible. If it's a failure, put the message of the error in the commit message, possibly with the identifier of the buildbot slave. If there are too many failures, it can be put in the tracker, with a reference in the message. Use the "Reopens" tag to automatically reopen the ticket:
Revert r<revision number>: Brief description A description of the problem, or a traceback if pertinent Reopens: #<ticket number>
Reverted branches are to be reviewed again before being merged.