Skip to content

Instantly share code, notes, and snippets.

@indiesquidge
Last active March 2, 2017 00:29
Show Gist options
  • Star 0 You must be signed in to star a gist
  • Fork 0 You must be signed in to fork a gist
  • Save indiesquidge/032fd4bc0aeb52597d03b154227d2394 to your computer and use it in GitHub Desktop.
Save indiesquidge/032fd4bc0aeb52597d03b154227d2394 to your computer and use it in GitHub Desktop.
Ideas around the flow of PRs, how to request reviewers, and PR templates

Pull Requests & Reviews

Brief Background

People are far less inclined to "drudge" through a review if they have no clue or context as to what they are reviewing.

Details and descriptions are paramount for PRs that include things like lots of new logic, lots of deletions, and/or new files.

It takes a lot of energy for the reviewer to parse through new code, so making it as seamless and easy as possible by letting them know what you did, why you made certain decisions, etc. is super helpful.

This will certainly require a bit more upfront work on the creator of the PR, but they gain a benefit as well. By forcing oneself to recap and explain their choices when adding in a new piece of software, it increases the author's awareness to their code's readability and maintainability, and may even uncover observations or refactors that would be otherwise overlooked.

Finding Dedicated Reviewers

If possible, one should try and find the last person who touched the files they are updating via git blame or file history on GitHub.

The last person who touched the file likely has a more intimate understanding of it's logic and structure, which will make them a great candidate for a PR reviewer.

Try not to add more than few potential reviewers to each PR. Adding the same 4-8 people to every PR creates fatigue and induces somewhat of a bystander effect.

Make sure to also look at other open PRs on the same repository to check if someone already has been overloaded with requests. Ideally, review requests should be spread out amongst all contributors to a specific repo.

Merging a Pull Request

A pull request needs two approvals before it can be merged into development.

There are three different states PR reviews can be in: approved, commented, or rejected ("requested changes"). If a PR has any unresolved rejected ("requested changes"), please do not merge it.

⚠️ NOTE: regardless of how many approvals there are for a PR, if someone has requested updates, make sure you follow up with them before merging

The person who created the PR should be the one to merge it, not the final reviewer. This is due to the fact that sometimes the PR creator may be in the process of adding another commit, rebasing onto another branch, squashing commits together, etc. It is impossible to know whether or not the creator is changing the PR, and merging it as a reviewer would mess up this flow if the PR is still in flux.

⚠️ NOTE: please do not merge a PR if you were not the one who created it.

Once your PR has been approved by at least two people and is free of unresolved rejections or comments, feel free to merge it into development.

If you need to make changes to your PR based on feedback, changed features, a found bug, etc., after you add your changes and push to your PR branch, please make sure to let the people reviewing your pull request know that you have modified the code.

Preferably, this should be done with a comment on the PR that tags the reviewer's GitHub handles, not as a Slack message. It is better to keep things related to a PR, including comments and discussions, co-located in the PR itself on GitHub.

Requesting reviews on Slack, or discussing critical details of the PR somewhere other than the PR comment thread makes it hard for someone who needs to reference the PR history (such as another reviewer) to follow important details as to why the code looks the way it does or was updated in a particular way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment