Small PRs
Why Write Small PRs?
Small, simple PRs are:
- Reviewed more quickly. It’s easier for a reviewer to find five minutes several times to review small PRs than to set aside a 30 minute block to review one large PR.
- Reviewed more thoroughly. With large changes, reviewers and authors tend to get frustrated by large volumes of detailed commentary shifting back and forth—sometimes to the point where important points get missed or dropped.
- Less likely to introduce bugs. Since you’re making fewer changes, it’s easier for you and your reviewer to reason effectively about the impact of the PR and see if a bug has been introduced.
- Less wasted work if they are rejected. If you write a huge PR and then your reviewer says that the overall direction is wrong, you’ve wasted a lot of work.
- Easier to merge. Working on a large PR takes a long time, so you will have lots of conflicts when you merge, and you will have to merge frequently.
- Easier to design well. It’s a lot easier to polish the design and code health of a small change than it is to refine all the details of a large change.
- Less blocking on reviews. Sending self-contained portions of your overall change allows you to continue coding while you wait for your current PR in review.
- Simpler to roll back. A large PR will more likely touch files that get updated between the initial PR submission and a rollback PR, complicating the rollback (the intermediate PRs will probably need to be rolled back too).
Note that reviewers have discretion to reject your change outright for the sole reason of it being too large. Usually they will thank you for your contribution but request that you somehow make it into a series of smaller changes. It can be a lot of work to split up a change after you’ve already written it, or require lots of time arguing about why the reviewer should accept your large change. It’s easier to just write small PRs in the first place.
What is Small?
In general, the right size for a PR is one self-contained change. This means that:
- The PR makes a minimal change that addresses just one thing. This is usually just one part of a feature, rather than a whole feature at once. In general it’s better to err on the side of writing PRs that are too small. Work with your reviewer to find out what an acceptable size is.
- The PR should include related test code.
- Everything the reviewer needs to understand about the PR (except future development) is in the PR, the PR’s description, the existing codebase, or a PR they’ve already reviewed.
- The system will continue to work well for its users and for the developers after the PR is checked in.
- The PR is not so small that its implications are difficult to understand. If you add a new API, you should include a usage of the API in the same PR so that reviewers can better understand how the API will be used. This also prevents checking in unused APIs.
There are no hard and fast rules about how large is “too large.” 100 lines is usually a reasonable size for a PR, and 1000 lines is usually too large, but it’s up to the judgment of your reviewer. The number of files that a change is spread across also affects its “size.” A 200-line change in one file might be okay, but spread across 50 files it would usually be too large.
Keep in mind that although you have been intimately involved with your code from the moment you started to write it, the reviewer often has no context. What seems like an acceptably-sized PR to you might be overwhelming to your reviewer. When in doubt, write PRs that are smaller than you think you need to write. Reviewers rarely complain about getting PRs that are too small.
When are Large PRs Okay?
There are a few situations in which large changes aren’t as bad:
- You can usually count deletion of an entire file as being just one line of change, because it doesn’t take the reviewer very long to review.
- Sometimes a large PR has been generated by an automatic refactoring tool that you trust completely, and the reviewer’s job is just to sanity check and say that they really do want the change. These PRs can be larger, although some of the caveats from above (such as merging and testing) still apply.
Splitting by Files
Another way to split up a PR is by groupings of files that will require different reviewers but are otherwise self-contained changes.
For example: you send off one PR for modifications to a protocol buffer and another PR for changes to the code that uses that protocol. You have to submit the protocol PR before the code PR, but they can both be reviewed simultaneously. If you do this, you might want to inform both sets of reviewers about the other PR that you wrote, so that they have context for your changes.
Another example: you send one PR for a code change and another for the configuration or experiment that uses that code; this is easier to roll back too, if necessary, as configuration/experiment files are sometimes pushed to production faster than code changes.
Separate Out Refactorings
It’s usually best to do refactorings in a separate PR from feature changes or bug fixes. For example, moving and renaming a class should be in a different PR from fixing a bug in that class. It is much easier for reviewers to understand the changes introduced by each PR when they are separate.
Small cleanups such as fixing a local variable name can be included inside of a feature change or bug fix PR, though. It’s up to the judgment of developers and reviewers to decide when a refactoring is so large that it will make the review more difficult if included in your current PR.
Keep related test code in the same PR
PRs should include related test code. Remember that smallness here refers to the conceptual idea that the PR should be focused and is not a simplistic function on line count.
A PR that adds or changes logic should be accompanied by new or updated tests for the new behavior. Pure refactoring PRs (that aren’t intended to change behavior) should also be covered by tests; ideally, these tests already exist, but if they don’t, you should add them.
Independent test modifications can go into separate PRs first, similar to the refactorings guidelines. That includes:
- Validating pre-existing, submitted code with new tests.
- Ensures that important logic is covered by tests.
- Increases confidence in subsequent refactorings on affected code. For example, if you want to refactor code that isn’t already covered by tests, submitting test PRs before submitting refactoring PRs can validate that the tested behavior is unchanged before and after the refactoring.
- Refactoring the test code (e.g. introduce helper functions).
- Introducing larger test framework code (e.g. an integration test).
Don’t Break the Build
If you have several PRs that depend on each other, you need to find a way to make sure the whole system keeps working after each PR is submitted. Otherwise you might break the build for all your fellow developers for a few minutes between your PR submissions (or even longer if something goes wrong unexpectedly with your later PR submissions).
Can’t Make It Small Enough
Sometimes you will encounter situations where it seems like your PR has to be large. This is very rarely true. Authors who practice writing small PRs can almost always find a way to decompose functionality into a series of small changes.
Before writing a large PR, consider whether preceding it with a refactoring-only PR could pave the way for a cleaner implementation. Talk to your teammates and see if anybody has thoughts on how to implement the functionality in small PRs instead.
If all of these options fail (which should be extremely rare) then get consent from your reviewers in advance to review a large PR, so they are warned about what is coming. In this situation, expect to be going through the review process for a long time, be vigilant about not introducing bugs, and be extra diligent about writing tests.