Code review guidelines

Timely code reviews unblock people, result in more code being shipped to customers, and foster a positive and reciprocal team culture. While review tools and notifications can help with this, slow reviews and a lack of reciprocity are ultimately a cultural problem. Fixing it requires a concerted effort on everyone's part.

This document describes a set of responsibilities for reviewers and authors of code which can help to define your organization's culture around reviewing. Feel free to copy, change, or extend it as needed.

Reviewer responsibilities

• Reviewers are expected to start their day by reviewing pending pull requests. Finishing reviews early in the day helps other team members move forward and have a productive day.
• Reviewers must minimise the time between a pull request being opened and being aware that someone is waiting on them. They should set up notifications, for example, real-time slack notifications for GitHub. Regardless of the mechanism, it’s the reviewer’s responsibility to ensure that they are receiving notifications in a timely manner. A pull request that goes unreviewed simply because the reviewer was unaware that it existed is unacceptable.
• Pull requests must be reviewed or acknowledged within an agreed upon SLA: one business day from the reviewer’s perspective, assuming an 8 hour work day. For example, if John requests a review from Derek at 8am EST but Derek usually starts his day at 9am PST (~3 hours later), then Derek should review the pull request by 5PM PST. If Derek tags Jason in a pull request at 2pm PST, and Jason works from 8am * 4pm PST, then Derek should expect a review by the following day at 2pm.
• Follow-ups to a review should be initiated again via the code review tool, e.g., the request review button in GitHub. The same SLA as the initial review applies to follow-up reviews.
• If a pull request cannot be reviewed within the SLA, it must be acknowledged by the reviewer with an estimate on when they can review it. This can be done via a DM, for example, “Hi John, I saw your PR for xyz, but I’m swamped today and won’t be able to get to it. I’ll have a look by EoD tomorrow.” Alternatively, an acknowledgement can be left in a pull request comment. The reviewer must honour their commitment and follow up, or reacknowledge.
• When a pull request is lengthy or difficult to review, reviewers should suggest a call to discuss the changes with the author.
• A reviewer should request a review from someone else if they feel they are not the right reviewer and are uncomfortable giving their stamp of approval.

Author responsibilities

• Authors are expected to open pull requests in draft mode, review their own code, proofread any copy, and green it up before requesting a review. This makes the best use of the reviewer’s time.
• Authors should write succinct summaries on their pull requests to give the reviewers the context they need. This is especially helpful when the reviewer is less familiar with the work or if they context switch regularly. Good summaries describe the problem being solved, add helpful context, link to existing tickets, and provide an appropriate test plan. A summary may be omitted if the pull request is small enough that the commit message fully describes the change. No comment is acceptable only if the context is perfectly clear.
• A pull request that has not been reviewed or acknowledged by the end of the SLA period should be posted in the author’s team channel with a request for review. It no longer requires the original reviewer’s approval.
• Authors are discouraged from DMing reviewers for a review. This circumvents the agreed upon SLA and is a crutch for reviewers that have not set up effective notifications. The exception to this rule is when a diff is too urgent or time sensitive to fall within the SLA period.
• Authors should request reviews from those who own the code or who have a vested stake in maintaining it. In small organizations ownership can likely be inferred. In larger orgs, this might be automated by a CODEOWNERS file. If in doubt, ask in a team channel who should review a particular pull request.
• Authors should be intentional about the reviewers they select for review. Opting for a small number of reviewers (1-2) is preferred over the “spray and pray” method of tagging many reviewers in the hopes that one will get to it.
• When multiple reviewers are tagged authors should clearly state whose approval is required before merging. For example, can the PR be merged after either Jason or John approves, or do they both need to approve it? Was Josh only tagged for awareness and his approval is not required? It helps to have a default org policy such as "any approval is sufficient to merge" so that only special cases need expicitness. The code review tool may have built in functionality to assign roles to reviewers.
• Authors should have empathy for the fact that some reviewers are busier than others or find context switching to be more costly. It’s nice to receive fast reviews, but the SLA exists to afford time for the reviewer to prioritise it.

---

Set up slack alerts for review requests in GitHub

Here is my recommended setup for PR notifications if using GitHub and Slack.

1. On any page in GitHub, click your profile icon in the top right and go to Settings.
2. On the left sidebar, under Integrations, click Scheduled Reminders.
3. Under Configured organizations, you should see aspect-build. Click the pencil icon.

Enable a scheduled daily reminder

1. Configure a scheduled reminder on Weekdays at your usual start time.
2. Select Review requests assigned to you.

Enable real-time alerts

1. Select the Enable real-time alerts checkbox.
2. Select the following recommended options:
   • You are assigned a review
   • Your PR is approved or has changes requested
   • Someone merges your pull request

See the official docs.