Guidelines for core contributors who are closing a pull request (PR).
The following definitions are used in the article below:
creator: A person who wants some code to be merged with the master branch.
reviewer: Some person other than the creator of the pull request, who is responsible for reviewing the code.
If you are new to reviewing code, you may benefit from reading this [introductory blog article](https://10kloc.wordpress.com/2016/04/03/effective-code-reviews/). The steps for reviewing the code are.
- Any code that is to be merged with the master branch can be done so only after it has been properly code reviewed by a reviewer.
- The following are the combined responsibilities of the creator and reviewer:
- Correctness of the new code to be merged.
- Adherence to style guidelines.
- Design and code quality.
- Comments are clear and understandable, and use proper English spelling and grammar.
- Unit test coverage.
- Nothing breaks.
- Benchmarking results (The decision of whether to perform the benchmarking or not). The actual task of benchmarking should be done by the creator.
- The reviewer is the person who is responsible for merging the pull request with the master branch after all the requested changes have been addressed by the creator.
- The size of each new pull request should preferably be below 500 lines of code. This is not a hard limit since some pull requests would naturally exceed this limit. This guideline suggests that, if you think that your pull request is going to be large, try and break it down into smaller pull requests.
- Once a pull request is created and assigned to a reviewer, the reviewer should make sure that they give comments for the first review pass within 3 days from the day the pull request was first created. This applies to all the pull requests that satisfy the 500 lines of code limit as mentioned in point 1. If the reviewer thinks that it wouldn't be possible to get back within 3 days, then the reviewer should inform the creator of the pull request and, and at the creator's discretion they can assign the review to someone else or wait for the original reviewer. This applies to the subsequent review passes too.
- If a review comment specifies some personal style preference (something not mentioned in Google C++ style guide or the Quickstep style guide that builds on it) that the reviewer follows, then the creator can use their own judgement to decide what they want to do.
If you as the person closing the pull request (PR) feel uncomfortable with closing the PR, please reach out to the core contributors group.
Related articles appear here based on the labels you select. Click to edit the macro and add or change labels.