Pull request best practices
Code reviews are an integral part of developing high quality software in an agile process. Reviewing code is also really difficult and time-consuming--especially if the pull request is large and you’re not sure what it’s trying to achieve. On VA.gov, we have a general rule to try to limit pull requests to a size that can be reviewed in an hour, which is about 400-500 lines of code. Help your code reviewer get through your pull request smoothly by following our pull request best practices.
If your pull request is not ready for review, please make it a draft PR. This will prevent any codeowners from receiving notifications until it is ready.
Clearly summarize the change in the PR title and description
Title: provide a concise summary of the change.
Description: make it clear what the change is describing.
Explain how code changes are related to the pull request objectives.
Provide context for reviewers by referencing the GitHub issues your PR relates to.
Take time to break down tasks up front
You're probably already working on a project that has work broken into separate user stories or tickets, but you can often break them down further once you start diving into the technical details.
For example, maybe you have a ticket to write a new page on your application. One ticket for a single page might seem like a reasonable breakdown of work, but as you start investigating you might find that there are components on the page that are more complex than others. So you might be able to take a ticket for one new page and break it down like:
Create scaffolding for the new page
Implement component X on the new page
Implement component Y and Z on the new page
This is similar to what we do when we build a new form on VA.gov, for example. Once we get a mockup of the form from designers, we have a standard set of tickets that are created:
Create empty form config and scaffolding
Build intro page
Build page 1
Build page 2
Build page 3 (etc)
Connect form submission to API
Build confirmation page
Then, for each of the tickets for building a new page, developers look for ones that have particularly complex fields or new types of fields and break those into separate tickets.
Similarly, when you have some refactoring you're attempting to do, you can often make small, incremental changes instead of one big application wide change.
Look for more ways to break up work
The above guidance is all well and good, but you still may get into working on a ticket and have it unexpectedly explode in complexity. As painful as it can be, continually looking for ways to break down the changes into pieces that can be done later while you're writing code can be useful. Just because you've picked up a single ticket to work on doesn't mean it has to come through in a single PR.
One way to make this easier is to create another branch off your feature branch and create a PR for that branch, using your original feature branch as the base, instead of main. This will let people review smaller chunks of your PR without you having to merge code into production, since you can just merge back into your feature branch.
Separate refactoring from feature changes
One other useful tip is to try to keep refactoring changes separate from feature or bug fixes. It's easy for a PR to grow in size when in addition to fixing a bug you decide to refactor the component as well.
Stay focused on completing the tasks in your ticket
Similar to the above tip, focusing on addressing only the issues in one ticket can be a helpful rule to follow. Sometimes code reviews can feel like a checkpoint that takes time away from coding and you might be tempted to build up several changes and get them into a single PR. This often results in a code review that's convenient for the PR author, but harder on reviewers who have to follow multiple changes and their justifications to understand a PR.
Provide extra support to reviewers when a small PR isn't possible
Sometimes you just won't be able to get your changes down to 400 or 500 lines. When that happens, you should do your best to provide extra support to reviewers, as well as accounting for the increase in turnaround time that a large PR will need. Some things to think about doing:
Make comments in the PR pointing to particular important changes that need reviews
Offer to talk through changes on a video call, so reviewers can ask questions quickly
Write more in depth descriptions of the changes and point out repetative changes that a reviewer can skim
Format your code in accordance with style guidelines
Run your linter and address any linting related issues before opening your PR.
If you programmatically ignore a linting recommendation (e.g. via
rubocop:disable), please comment in the PR explaining why.If your linter does raise any issues unrelated to the PR's subject matter, please fix these in a separate PR.
Follow guidelines for automated code linting
Run your linter and address any linting related issues before opening your PR.
If you programmatically ignore a linting recommendation (Example:
rubocop:disable), please comment in the PR explaining why.If your linter does raise any issues unrelated to the PR's subject matter, please fix these in a separate PR.
Other Resources for Pull Requests
Help and feedback
Get help from the Platform Support Team in Slack.
Submit a feature idea to the Platform.