A small PR is a beautiful PR

Often we try to adhere to the Single-responsibility principle . It makes sense not only for functions and classes, but also for pull requests.

A PR should try to do just one thing, and do it well. Remind yourself of that thing by writing a single sentence at top of PR which explains the goal, explains what good thing a merge will produce. If you need several sentences, consider splitting the PR.

Motivation for a PR will often look like “adds feature X”, “fixes bug Y”, or “refactors by extracting helper”. These statements are promises, verifiable by a reviewer.

mechanical transformations

Occasionally we want to touch lots of a files in some trivial way, such as adjusting whitespace or running isort . If you’re already making some substantive edit to a file, like adding a method, then it’s fine to include such mechanical edits. If there will be “many” such diffs accompanying your one-line change, go to the trouble of creating two branches and sending two PRs. Both will be easy to quickly read and quickly approve.

If a mechanical transformation touches a bunch of files that lack other substantive edits, definitely do that refactor in a separate, tightly focused PR. Again, it will be easy to read and approve.

Fixing lint errors, editing until flake8 runs cleanly, might not be a strictly mechanical process. But it is close enough, the same guidelines apply.

a proof

Every essay, every math theorem, is an argument. It tries to convince the reader of some proposition.

So, too, with a function or a class. It claims to preserve or establish certain invariants, and it claims the code is correct.

View a PR in the same way. It makes claims about what will be true after a merge, and we can verify those claims. Sticking to single-responsibility for each PR makes it easier to verify them.

(Image credit: smilla4 )

prev next