|Version 21 (modified by exarkun, 8 years ago) (diff)|
Twisted Code Review Process
The post-development, pre-commit process Twisted has adopted is this:
- Commits to trunk/ must be the result of the review process outlined below. The only exception to this is the Twisted.Quotes file. There are no other exceptions.
- Changes to Twisted must be accompanied by a ticket in the tracker.
- 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.
- When a ticket becomes ready for review, the trac workflow for the developer is as follows:
- Add review to the comma-separated keywords field
- Change the owner to either no one or, if a particular developer is likely to review a branch or a particular developer's input is desired on the branch, that developer. For the former case, when starting a review the reviewer should assign the ticket to themselves.
- Comment on the ticket indicating anything it might be beneficial for the reviewer to know. 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.
- A reviewer can be anyone who is familiar with the Twisted codebase, coding standard, and review requirements. 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.
- A reviewer must reject a set of changes in any of the following circumstances:
- The test suite fails on any of the supported* platforms as a result of the changes.
- The changes alter the behavior of code in a way which is not tested.
- The changes add new behavior without adding test coverage for that behavior.
- The changes add new public APIs or test methods without adding documentation for them.
- The changes modify public APIs or test methods which are undocumented (ie, a function which has no docstring is modified, or a class which has no docstring has methods added to it) without adding documentation.
- The changes modify the behavior of a documented API without updating the documentation.
- The changes violate the coding standard.
- pyflakes is an useful tool to check basic quality of 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 set of changes for other reasons, most 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 above list.
- When a reviewer has completed a review, the trac workflow is as follows:
- Remove review from the comma-separated keywords field
- Change the owner back to the developer who put the ticket up for review
- Put whatever review comments are being offered into a comment on the ticket
- After a reviewer has given a change set the okay, the author (or another developer, if the author is so inclined or lacks commit privileges) may commit the change set to trunk.
- The trunk commit must include the name of the author or authors, reviewer or reviewers, and the tickets which are being resolved by the changes being committed. The suggested format for this is:
Merge <branch name>: Brief description
Long description (as long as you wish)
Author: <names> Reviewer: <names> Fixes: #<ticket number>
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. This should be detailed enough for someone doing a release to formulate a changelog entry.
- 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.
- If a branch is used, it should be deleted a short time after after it is merged into trunk (leaving enough time for the buildslaves to run to completion is a good idea, in case a regression has been introduced and the merge must be reverted).
Some of the higher level things for a reviewer to consider about a branch:
- Does it change existing APIs in an incompatible way? Are these APIs stable enough to warrant backwards compatibility or a deprecation period?
- 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?