Code Review Process¶
All commits to Twisted’s trunk must follow this review process.
Both authors and reviewers should be intimately familiar with all requirements on this page.
Long story short for contributors / PR authors:
Start by creating a GitHub Issue describing the issue or the reason a change is needed.
Follow our coding standard and implement the required changes
Create a GitHub Pull Request to have the changes merged in the main trunk
Make sure all the PR checks are green and the changes are ready for review.
Leave an explicit comment on the PR containing the please review text. You can use a draft PR but once ready, leave the explicit comment asking for the review. This will trigger the review process and the required team notifications.
Wait for a review or try to find a review via IRC, mailing list or Gitter
Engage in a conversation with the reviewer to get the changes approved.
Once the changes from the PR are approved, the PR can be merged.
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. It makes no sense to review your code, and we assume that before pushing the code to a public PR, you already did the first round of self-review.
Make sure that all checks are green!
Note any unreliable / flaky tests should have a separate issue created.
Review the change, and write a detailed comment about all potential improvements to the branch (See [#Howtobeagoodreviewer below]).
Use GitHub review UI to approve or request changes to the PR.
If the author does not have commit access, merge the change for them or add the “needs-merge” label.
Who can review a PR?¶
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.
Twisted committers may review anyone’s PRs; those submitted by other committers or those submitted by non-committer contributors. If a non-committer contributor submits a PR that is acceptable to merge, it is the committer’s responsibility to commit and merge the PR. When a committer reviews a PR, they are responsible if there are any problems with the review.
Non-committer contributors may review PRs that committers have submitted. When a non-committer does a passing review, the committer may accept it and land their change, but they are then responsible for the adequacy of the review. So, if a non-committer does a review you feel might be incomplete, put it back into review and explain what they might have missed - this kind of reviewing-the-review is important to make sure that more people learn how to do reviews well!
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.
A reviewer may reject a change for various reasons, many of which are hard to quantify. Use your best judgment, and don’t be afraid to point out problems that 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 that 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 confuse 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”, 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 PR from the review queue, you may substantially increase the amount of time that the author has to wait for a real, comprehensive review, which is very frustrating.