Hacker News Reader

Comment by matthewwolfe | Hacker News Reader

I’ve found that pull request size solves a lot of the issues that people have with code reviews and quality. When people see a very large pull request, there is a tendency to skim and then slap on an Approval. Keeping pull requests small typically leads to a more thorough review because it’s much easier to parse the changes and build a mental model. This usually leads to better feedback. This also helps prevent less experienced devs from going crazy down the rabbit hole and making a huge code change. Small and steady is best, and fostering a culture where people are often asking each other questions and collaborating is key.

View on HN

> Keeping pull requests small typically leads to a more thorough review because it’s much easier to parse the changes and build a mental model.

Corollary, the practice that exists in some organizations or some peoples minds to change everything (code structure, names, formatting, ...) that doesn't match current definitions and/or personal taste as part of a change for some specific feature slows things down, and hides potential problems by covering the intended modification in the fog of the other changes.

Separation helps. Even for some intended changes, by splitting into incremental steps. Of course starting with N+1 requires a speedy review of N. Which requires N to be focused, to be easy to check.

If something needs a complete rewrite, that's a different story.

Alternatively, as small PRs can be hard (depending on codebase, language, etc), you can try to do small commits.

Aiming for each commit telling a logical part of the story, that together make a PR that is easy to review.

Why is this easier than small PRs? Typically PRs can’t break anything. Whether it’s CI or breaking the build on the main branch or breaking the workflow of other devs, normally main needs to be stable and PRs are the unit of stable addition to that. However, each commit does not necessarily need to be so, especially if you squash or merge so that there are clear working points to revert to on the main branch.

In many cases my commits might not even compile, but this frees me up to do things like: put bulk renaming or code moves in commits that can mostly be ignored, keep business logic changes in small commits that are easily reviewed, write all test function definitions before filling them in to help summarise the testing that is done, etc. thankfully over the last 4 years or so GitHub’s handling of commit level review on PRs has improved a lot.

Small PRs should still be the goal for many reasons, but when it’s going to take way too much work to do, small commits that lead the reviewer through a story of the change being made can be very effective.

Reply on news.ycombinator.com