{
  "$type": "site.standard.document",
  "canonicalUrl": "https://rednafi.com/misc/sane-pull-request/",
  "description": "Make pull requests easier to review. Learn commit organization, diff filtering, annotations, and context that helps reviewers understand changes faster.",
  "path": "/misc/sane-pull-request/",
  "publishedAt": "2024-07-14T00:00:00.000Z",
  "site": "at://did:plc:fgtm2c26vfcj74rfmeggbyqj/site.standard.publication/3mnl6f7ob462z",
  "tags": [
    "Git",
    "GitHub"
  ],
  "textContent": "One of the reasons why I'm a big advocate of rebasing and cleaning up feature branches, even\nwhen the changes get squash-merged to the mainline, is that it makes the PR reviewer's life\na little easier. I've written about [my rebasing workflow] before and learned a few new\nthings from the [Hacker News discussion] around it.\n\nWhile there's been no shortage of text on why and how to craft [atomic commits], I often\nfind those discussions focus too much on VCS hygiene, and the main benefit gets lost in the\nminutiae. When working in a team setup, I've discovered that individual commits matter much\nless than the final change list.\n\nAlso, I find some of the prescriptive suggestions for easier review, like keeping the PR\nunder ~150 lines, ensuring that the tests pass in each commit, and tidying the commits to be\nstrictly independent, quite cumbersome. [Stacked PRs] sometimes help to make large changes a\nbit more tractable, but that comes with a whole set of review-conflict-feedback challenges.\nSo this piece will mainly focus on making large PRs a wee bit easier to work with.\n\nHere's a quick rundown of the things I find useful to make reviewing the grunt work of pull\nrequests a bit more tractable. I don't always strictly follow them while doing personal or\nOSS work, but these steps have been helpful while working on a large shared repo at work.\n\n- Avoiding the temptation to lump tangentially related changes into a PR to speed things up.\n\n- Having a ton of fragmented commits makes filtering useless when navigating the PR diff in\n  a platform like GitHub. I really like to filter diffs on GitHub, but it wouldn't be useful\n  if the commits are all over the place.\n\n    ![GitHub PR diff view with commit filter dropdown for navigating changes][image_1]\n\n- To make diff filtering better, I often rebase my feature branch after a messy development\n  workflow and divide the changes into a few commits clustered around the core\n  implementation, tests, documentation, dependency upgrades, and occasional refactoring.\n\n- Rebasing all the changes into a single commit is okay if the change is small, but for\n  bigger changes, this does more harm than good.\n\n- I've rarely spent the time to ensure that the individual commits are [perfect] - in the\n  sense that they're complete with passing tests or documentation. As long as the complete\n  change list makes sense as a whole, it's good enough. YMMV. The main goal is to make sure\n  the diff makes sense to the person reviewing the work.\n\n- Annotated comments from the author on the PR are great. I wish they'd take up less space\n  and there was a way to collapse them individually.\n\n    ![GitHub PR with author annotations explaining code changes inline][image_2]\n\n- Each PR must be connected to either an Issue or a Jira ticket, depending on how the team\n  works.\n\n- Adding context, screenshots, gifs, and videos to the PR description makes things so much\n  easier for me when I do the review. Being able to see that the changes work as intended\n  without running the code has its benefits.\n\n    ![PR description with embedded screenshot demonstrating feature behavior][image_3]\n\n- Keeping the PR in draft state until it's ready to be reviewed. I'm not a fan of getting a\n  notification to review some work only to find that it's not ready yet.\n\n\n\n\n[my rebasing workflow]:\n    /misc/on-rebasing/\n\n[hacker news discussion]:\n    https://news.ycombinator.com/item?id=40742628\n\n[atomic commits]:\n    https://www.aleksandrhovhannisyan.com/blog/atomic-git-commits/\n\n[stacked prs]:\n    https://benjamincongdon.me/blog/2022/07/17/In-Praise-of-Stacked-PRs/\n\n[perfect]:\n    https://simonwillison.net/2022/Oct/29/the-perfect-commit/\n\n[image_1]:\n    https://blob.rednafi.com/static/images/sane_pull_request/img_1.png\n\n[image_2]:\n    https://blob.rednafi.com/static/images/sane_pull_request/img_2.png\n\n[image_3]:\n    https://blob.rednafi.com/static/images/sane_pull_request/img_3.png",
  "title": "The sane pull request"
}