notes to self

How big should a pull request be?

On this page

Spoiler alert: Small, and about one thing.

Merging a pull request is the positive outcome of its actual job, which is to be reviewed. Everything good about a small change (fewer bugs getting through, a quick yes, a clean revert when it turns out wrong) comes down to one person being able to mentally map the whole thing. Size is the usual reason they can't.

One thing, then small

Ideally, every PR should exist for one reason (granted, this is easier said than done). That reason alone should justify the diff, regardless of how many lines. However, that "one reason" can often inflate to a bigger vision, instead of a cleanly encapsulated change.

Imagine you have two pull requests. The first is sixty lines that touch authentication, a button colour, and a config default. The second is four hundred lines doing a single thing. The small one is harder to review because the reviewer has to load three separate mental models and check that none of them introduces a regression or breaks the others.

When a commit title needs the word "and", it can probably be two pull requests.

In reality, size doesn't matter if the PR sticks to doing "one job", but it's still a good proxy for keeping things encapsulated. A change that's about one thing and also ends up being massive is rare, and when it does happen it usually splits cleanly anyway: the schema, then the services that read it, then the screen that shows it.

The number, and how to measure it

I've often heavily suggested keeping changes below 500 lines of code (LoC), because it's easier to give people a number than to impart a whole new delivery methodology. However, after some research it turns out that I'm pushing a bit high here. The most-cited LoC recommendation comes from SmartBear. When reviewing a large Cisco codebase, it found that reviewers start missing defects once a change runs past 200 to 400 lines, and miss more of them when they read faster than a few hundred lines an hour.

On the flip side, Google's own review guide refuses to name a figure at all: a change should do one thing and be as small as it can while still standing on its own. I like that a lot.

Both are right, and "500 lines" shouldn't be a gate that blocks a merge, but it does indicate that you may want to start looking for seams to split on.

git diff can help us keep an eye on size during development:

$ git diff --stat main...
 app/contact/form.tsx    | 48 ++++++++++++++++
 app/contact/submit.ts   | 22 ++++++++
 lib/validation.ts       | 16 +++++
 3 files changed, 86 insertions(+)

--stat gives you a per-file summary and a total. The three dots in main... measure from the point your branch left main.

Smaller batches move faster

If your CTO or Product Owner keeps banging on about "product velocity", this helps. Smaller PRs reduce your team's cognitive load, and that LGTM is a lot easier to get. Less to review leads to quicker approvals, and a more thorough QA. Ultimately the team ships faster with better code.

Speaking of QA, a reviewer reads at a fixed speed, so two thousand lines don't get twice the scrutiny of one thousand, they get half, skimmed, or parked in the queue until someone finds the time. Small changes clear in a single pass and they're gone. Across a team, your throughput is set by how fast changes get through review, not how fast they get written.

Small changes collide less, too. A branch that's open for an afternoon rarely conflicts; one that's open a fortnight drifts off from main until merging becomes its own little project. And when something does break, a small change is a small thing to undo.

Is it worth it?

Yes, for sure. There is one key downside, however: a feature could be split across six pull requests and no single diff shows the whole of it, so a reviewer can sign off every part and never see what they add up to. I handle that with a final integration PR that wires the pieces together, which doubles as the one place to test the whole flow end to end.