One of the reasons why I’m a big advocate of rebasing and cleaning up feature branches, even when the changes get squash-merged to the mainline, is that it makes the PR reviewer’s life a little easier. I’ve written about my rebase workflow before1 and learned a few new things from the Hacker News discussion2 around it.

While there’s been no shortage of text on why and how to craft atomic commits3, I often find those discussions focus too much on VCS hygiene, and the main benefit gets lost in the minutiae. When working in a team setup, I’ve discovered that individual commits matter much less than the final change list.

Also, I find some of the prescriptive suggestions for easier review, like keeping the PR under ~150 lines, ensuring that the tests pass in each commit, and tidying the commits to be strictly independent, quite cumbersome. Stacked PRs4 sometimes help to make large changes a bit more tractable, but that comes with a whole set of review-conflict-feedback challenges. So this piece will mainly focus on making large PRs a wee bit easier to work with.

Here’s a quick rundown of the things I find useful to make reviewing the grunt work of pull requests a bit more tractable. I don’t always strictly follow them while doing personal or OSS work, but these steps have been helpful while working on a large shared repo at work.

  • Avoiding the temptation to lump tangentially related changes into a PR to speed things up.

  • Having a ton of fragmented commits makes filtering useless when navigating the PR diff in a platform like GitHub. I really like to filter diffs on GitHub, but it wouldn’t be useful if the commits are all over the place.

    commit filter

  • To make diff filtering better, I often rebase my feature branch after a messy development workflow and divide the changes into a few commits clustered around the core implementation, tests, documentation, dependency upgrades, and occasional refactoring.

  • Rebasing all the changes into a single commit is okay if the change is small, but for bigger changes, this does more harm than good.

  • I’ve rarely spent the time to ensure that the individual commits are perfect5—in the sense that they’re complete with passing tests or documentation. As long as the complete change list makes sense as a whole, it’s good enough. YMMV. The main goal is to make sure the diff makes sense to the person reviewing the work.

  • Annotated comments from the author on the PR are great. I wish they’d take up less space and there was a way to collapse them individually.

    annotated comments

  • Each PR must be connected to either an Issue or a Jira ticket, depending on how the team works.

  • Adding context, screenshots, gifs, and videos to the PR description makes things so much easier for me when I do the review. Being able to see that the changes work as intended without running the code has its benefits.

    feature preview

  • Keeping the PR in draft state until it’s ready to be reviewed. I’m not a fan of getting a notification to review some work only to find that it’s not ready yet.

  1. I kind of like rebasing ↩︎

  2. HN discussion on my rebasing workflow ↩︎

  3. Make atomic git commits ↩︎

  4. In praise of stacked PRs ↩︎

  5. The perfect commit ↩︎

Recent posts

  • HTTP requests via /dev/tcp
  • Log context propagation in Python ASGI apps
  • Please don't hijack my Python root logger
  • The *nix install command
  • Here-doc headache
  • I kind of like rebasing
  • Protobuffed contracts
  • TypeIs does what I thought TypeGuard would do in Python
  • ETag and HTTP caching
  • Crossing the CORS crossroad