Skip to content

Latest commit

 

History

History
59 lines (51 loc) · 3.61 KB

review_process.md

File metadata and controls

59 lines (51 loc) · 3.61 KB
Raw

Process for a smooth code review

We put great emphasis on code reviews, every pull requery (PR) , big or small, has to be approved by a reviewer with write access. Unfortunately, GitHub's review tool has a lot of shortcomings, especially for longer review threads. It can also make it hard to follow the changes that has been done to address a review comment (other people have complained about this too).

To address some of these shortcomings, we follow the following process. Before sending a PR, please spend some time and fully understand this process to help your reviewers:

  • Please try to have a single commit per each review round. When you want to send your PR, if you have multiple commits, squash them. Also, when you are responding to review comments, make sure to create exactly one commit to address all required changes. You can use --amend feature of git commit. This is to help your reviewer see all of the changes since last review round in a single commit.

  • Please never rewrite (e.g., --amend) an old commit that has been reviewed. This will lose GitHub comment threads attached to that commit. Note this also means never use git rebase on a branch that is under review, because that basically rewrites the old commits. To merge changes to the upstream repo that happen during the review, you can go to your master branch, pull the changes from the upstream repo, and then merge your master branch into your feature branch from which you sent the PR.

  • When sending a PR, disable "Allow edits by maintainers”"; and if you are a reviewer, never push a change (even a master merge) to a review branch. This will make it harder to maintain the local dev branch.

  • When responding to unresolved comments, please make sure you respond to all of them. Even if you simply do what the reviewer has suggested, add a "Done" comment before resolving the thread. This acts as a reminder to the reviewer about all of the requested changes from the last review round.

  • Finally, please make sure that you fill the TESTED: part of your PR description, especially for larger PRs. This will help your reviewer better understand what your change is supposed to do and how you have made sure it does that.

Let's look at a sample PR. This PR is sent for review on 3 Apr 2021 and it included two commits; it would have been better if the author had squashed those. After the first round of review comments was received, on 12 Apr 2021 this commit was created to address all of those comments. Note that it is very easy for reviewers to see every changes that have been done to address review comments. At the same time this merge commit was also added to update the PR branch with other changes that happened to the upstream repo while this PR was being reviewed (note git merge was used not git rebase). Finally, take a look at the TESTED: part of the PR description.