Pull Requests became popular for two reasons:
- Asynchronous communication. Developers can review and respond at any time, allowing them to continue development without interruption and review PRs when they come to a natural pause in their flow.
- Quality Assurance. Reviewing and testing code before it hits the target branch ensures that the target branch stays clean.
- Authoring
- Keep them small: it's very tempting to rewrite, refactor, boy scout and reformat the code as you develop but in general, it's best not to do it all at once.
- Perform a Self-Review: although it's very tempting to just dump your changes in a PR and let other people find the mistakes, create a draft PR and do a full review yourself. You can also use this “self-review” to point things out to your reviewers.
- Create a meaningful title: keep the title short and meaningful and avoid titles generated from the branch name or related ticket. PRs are the new Documentation, making PR history easy to browse makes searching for past decisions and thought processes much easier.
- Write a helpful description: be as thorough as possible, but be concise. A useful way to promote inspiration is to establish a PR Template:
- Overview of changes: things to address here are often what is not in the PR — alternatives that you have assessed and discarded.
- Questions/Notes for reviewers: notifying reviewers that they can safely ignore a chunk of the PR is going to be well received.
- How to test/demonstrate: anything that will help the reviewer test the changes (QA).
- Attachments: Screenshots, Videos. A picture is worth a thousand words, a video is worth a million.
- Acknowledge each and every comment: with any remote and asynchronous communication, it’s important to communicate that you have at least seen a comment that was meant for you. It might be with a simple emoji reaction.
- Don't merge until approved by all reviewers. Work out a system for determining who should review each PR.
- No emergency Pull Requests:
- If you absolutely have to merge a branch in the next 10 minutes (and there are plenty of situations where this is a totally sane thing to do), don't send out a message asking people to review it right now, just merge it.
- If you have a branch that must be merged in a short time frame, interrupt one person and do some pair programming or just merge it and suffer the quality hit.
- Reviewing
- Check out the code.
- Read the title and description: prime your expectations for what you are about to review.
- Validate your suggestions locally.
- Convert large suggestions to a Pull Request. If you found that your suggestion became quite large, don't waste your effort: branch off their branch and create a PR to merge into the original PR.
- Resist the urge to comment. The hardest thing about making comments is resisting the urge to make comments.
- If you don’t have a concrete alternative, don’t make a comment. If you simply don’t like something, come up with something better and make your case, otherwise, zip it.
- Be confident, not lazy. Don’t use words like “maybe”, “I don’t know”, “What if”, “I’m not sure”… anything that implies doubt. If you’re not sure, self-reflect — why aren’t you sure?
- No solution is ever perfect.
- Mimic style conventions before changing them. People are strangely attached to style and become very opinionated very quickly.
- Nitpicking is a good sign.
- Take long threads to a meeting room. Go into the meeting with the goal of finding the best solution, not to get your own way.
- Keep your tone of voice even. Be critical. Poke, prod and challenge — but do so professionally, not personally. Avoid “you” try "we". Be mindful of how you interpret comments — chances are someone didn’t mean to be an asshole, they just weren’t thinking, or were in a rush, or English is their second language, or they got distracted. Assume that people have good intentions.