Pull Requests (PRs) - Creating and Review

At ShakaCode, the number one job of developers is to contribute to shipping code via creating and reviewing pull requests. In this article, I’ll share my advice for success in this endeavor for both sides of the fence.

Setting up the Process for Success

Management Suggestions

1. Create a PR template that includes a checklist. See the bottom of the article for a markdown sample of common items.
2. Communicate to the team the importance of an effective review process and their respective duties. In my job leading engineering, quality code reviews are more important (or even more so) than writing new code. Prompt reviews have increased the velocity of the shipping code. Let senior team members know how much you, as the leader, value quality and quick reviews.
3. Setup static analysis as part of CI so that reviewers can avoid discussions around:
   1. Code formatting and linting, such as RuboCop, ESLint, and Prettier.
      1. These tools help enforce proper syntax usage and catch many bugs.
      2. Consistently formatted code is easier to read and, thus, reviewed.
      3. Nothing is worse as a reviewer than to have random whitespace changes unrelated to the pull request.
      4. Beyond using as many defaults for rules as possible, consider writing your own linter rules to enforce good coding practices specific to your app. Popmenu, for example, has custom Rubocop linters for XYZ.
      5. If you use TypeScript, include type checking in CI: unlike (nearly?) all other compiled languages, your tests may pass even if it doesn’t type-check!
   2. Code coverage tools can identify if newly added code has tests covering the newly added code. Reviewers do not want the tedious job of determining if tests cover freshly added code.
   3. Security analysis tools, such as the Brakeman gem, will highlight many security issues, saving effort for the reviewer.
4. Ensure developers have a backup task. The reality is that quality reviews often take time. Developers should always have the following task ready so they are not blocked from working.

Code Submitter Obligations and Suggestions

1. Make the reviewer’s job easier.
   1. Required: Keep your PR’s small
      1. Small reviews are more accessible and quicker for the reviewer, and thus able to get processed more promptly. For comparison, is it easier to commit to watching a 2-minute than a 60-minute video?
      2. With every merge to master a production system comes a risk of a severe error. Any merge might need to get reverted. Any good reviewer should be conscious of this risk and the responsibility not to pass a faulty PR.
   2. Required: Ensure your PR is focused.
      1. Small PRs, by their nature, tend to be focused. While it may be convenient for you to throw all your great ideas, refactorings, minor fixes, etc., in your current masterpiece PR, you’re creating a nightmare of complexity for the reviewer. For example, if you want to do some refactoring and code cleanup, then create a PR that clearly explains that. Your reviewer will evaluate your code mainly for improved readability, rather than a bug fix or feature.
      2. Types of focused PRs. PRs generally should not do more than one of:
         1. Features: Include before and after screenshots, videos, and other descriptive information. Screenshots, both in mobile and desktop formats, are essential for UX changes and fixes. Don’t expect that your reviewers will parse CSS in their heads!
         2. Performance Improvements: Where possible, provide measurement data before and after your changes that justify the complexity and risk of your changes. For example, if you’re fixing an N+1 query, show evidence that it exists and the resulting SQL after your fix.
         3. Bug fixes. Include a link to a crash report.
         4. Cleanups, including refactorings like better naming, DRY’ing up code, and deleting dead code.
   3. Required: Pass CI unless you know that an early round of the review or other collaboration would help.
   4. Avoid extraneous changes, such as formatting. Self-review will help you avoid this rookie mistake.
   5. Required: Do a self-review first.
      1. This is the number one thing that I tell junior developers. First, it’s embarrassing to have silly mistakes you would have caught if you only double-checked the code diffs on GitHub. It’s a little bit like printing your term paper and reviewing a paper copy, even though you reviewed it in your word processor. There’s something about having the reviewer’s view of the code that lets you catch some things, even small ones like a missing return at the end of a file or an additional file submitted.
      2. Use your review tools to add comments to lines, explaining anything non-obvious. Generally, avoid code comments that explain what code does. With good code, what is happening should be self-evident. However, feel free to add some comments that could provide clarity on why you did something. Is there anything that you’d tell the reviewer if reviewing in person? If so, add those comments next to the relevant lines. Maybe you might be unsure if the way you did something is appropriate? Ask that in a code comment. Provide links to documentation, other areas of the code, etc., if that can help the reviewer.
      3. Read your PR description. Did you miss anything that could help the reviewer?
   6. Add unit tests! Especially for Ruby and JavaScript code, it’s easy to introduce a silly syntax error when you lack code coverage.
   7. Promptly respond to review comments.
2. Consider DM’ing to request a review. Send a direct message to a reviewer with a link to the PR. I like that, as I’ll create a reminder on the Slack message. Unless a reviewer is typically quick to respond to GitHub notifications, don’t wait for a reviewer to detect your need for a review. DM if possible! Yet be polite and persistent. Your project leader will not want to know that your PR has been stuck for days or weeks because somebody missed your request for a review.
3. Required: Escalate if you can’t get a timely review. Project owners need code delivery, which is blocked if reviews are stuck.

Techniques to break up PRs

When coding on a feature, I’ll often discover something else I’d like to improve. Here’s what I do.

1. Separate the code into separate commits such that each can be a PR. It pays to develop proficiency with interactive rebasing. If you use JetBrains tools like RubyMine, try their tool by right-clicking the commit to start the interactive rebase. GitHub client and Git Kraken are other excellent choices.
2. Force-push the rebased branch.
3. Create a new task in Shortcut to describe the upcoming PR. That gives me the corresponding branch name for the new PR.
4. I’ve got multiple copies of the current project. I’ll create the new branch off of master, and then I’ll cherry-pick the commit, push the branch, and create the focused PR.
5. The smaller, focused PRs should get merged first, and then I’ll rebase my bigger feature on top of master. In doing so, the smaller commit should automatically fall out of the commits for the bigger branch.

Code Reviewer Suggestions

1. Did the code submitter follow the guidelines for an easy-to-review PR? If not, suggest that first, indicating that you’ll review ASAP X, Y, and Z are done.
2. Sometimes, do an interactive, live, or screen-share session going over the code. This can significantly accelerate the review process and facilitates programmer development.
3. Be polite. Be friendly. Be helpful. Be clear. If you’re a reviewer, you’re in a senior position. Building any bad feelings with your teammates during code reviews is a surefire way to limit your career potential, no matter how right or intelligent you are.
4. Know your review tools. This is essential for reviewing large PRs. Know how to mark files as reviewed. Learn how to view diffs since your last review.
5. Consider a MERGE with a required follow-up PR. Don’t nitpick and block good code from merging. Often it’s best to get code merged and to plan a quick follow-up PR to address nice-to-have suggestions.
6. Send a DM when the review is done. This gives the submitter the fastest notification and chance to respond to your requests and questions.