Pull request etiquette and review

How can we create a pull request (PR) that reflects our thought process and intention?

When creating your first ever pull request, it can seem daunting. You might think: ‘What will others think of my code or changes?’ There are a few things we can do to better improve our chances of a timely and well intentioned review.

• Ensure there are tests
• Have tests pass before committing
• Lint changes before committing
• (Optional) have commits tell a story
• Your changes should be small

Let’s address some of these more in depth.

Ensure there are tests, at the right level of abstraction

We want to have tests for our change, as it helps increase the confidence in our changes. Everyone makes mistakes, but tests help us catch these. They also improve the ability for us to refactor code in the future.

If you have to ask yourself if you need tests, you likely do, at least at some level of abstraction. We want to test the right things, at the right level of abstraction. This means writing an assortment of unit tests, integration tests, end-to-end tests.

Have fast, accurate, resilient tests. Slow tests impact others and your overall test suite run time. No one likes flaky tests, so it’s important that they aren’t brittle either. Tests that flake increase the likelihood that you’ll have to fix it in the future. And you’ll definitely have to fix it when it flakes right before a hot fix.

Have tests pass before committing

The entire test suite should already be passing before we commit, so that we can reduce the amount of commits we have like ‘make tests pass’ and ‘fix test bug’.

Lint changes before committing

We want less commits like ‘fix linting’ or ‘increase linting count’.

(Optional) have commits tell a story

Some want the commits in their branch to tell a story, so that each commit builds upon the next.

Your changes should be small

Changes that are large (over 250 lines) are more difficult to reason about. Having a smaller changeset helps reviewers who have dedicated their time to reviewing.

The larger the changeset, the more likely review quality goes down. It’s harder to reason about a change when the line count and complexity is higher. This means we can’t review everything, or understand how it all works, and we start to zone out.

Once we’ve checked off some, if not all of these, it’s time to create the pull request.

Writing a good description:

• A clear, succinct title sets context before anyone reads a single line
• Link any related tickets or issues so reviewers can get background if they need it
• Explain the thought process and how you reached your goal
• Call out edge cases and error handling you considered
• Attach a short video recording if it helps the reviewer see the final result

Be sure to respond to all review comments and make adjustments where needed.

Review

When reviewing, look at everything mentioned above.

Tone and language:

The purpose of code review is to refine changes and reduce defects. We are after a common goal of improving the codebase, so the focus should always be on the code, not the person. Use language that focuses on the changes and offers concrete suggestions. Pointing out that something is outright wrong is not constructive.

If you have nothing to contribute, don’t block the PR. Either pass it to another reviewer or approve.

Time and scope:

Set aside 30 to 60 minutes per review, depending on line count and complexity. If a review regularly runs over 60 minutes, that’s a signal to break the PR into smaller ones. Many smaller PRs are easier to review, approve, and merge in the same total time as one large one.

Standards and principles:

Ensure the pull request follows your team’s standards. Consider principles like DRY (Don’t Repeat Yourself) and SOLID where they apply. These are not hard requirements, just items worth considering when reading and writing code.

In closing

Hope this helps with making pull requests a smoother experience for everyone involved. When the author puts in the work upfront, reviewers can focus on what actually matters instead of chasing down context.