Published on May 26, 2025 2:20 AM GMT
I'm a big fan of small focused PRs: it's much easier to tell whetherthey're actually doing the right thing. For example, I recently raninto a common scenario where I started adding some functionality, andrealized this required some refactoring. The refactoring also made itclear that the original code was missing some important test coverage,so I added some tests. I ended up with a big set of changes whichcombined large refactors and a major (intended) change to the output.
This is a bad combination: if my refactoring accidentally changedfunctionality in a way not captured by the tests we might miss it.And with so many changes it's going to be hard for the reviewer (andeven the author) to keep it all in their head at once. Instead, itwould have been better if I had first done the refactoring and addedtests, and then followed up with the new functionality. The first PRwould be a bit noisy but conceptually straightforward and easy tovalidate because it produces exactly the same output for all inputs.The second PR would be very clean, but would require carefulvalidation to verify that the changes really are an improvement.
I might be tempted to say "oh well, I should have approached thisdifferently, I'll do better next time" but really I did need tocombine these two changes when writing them: I didn't know for surewhat I would want for the refactoring until I'd tried to use it toimplement the functionality change. So if my plan was to do betternext time I wouldn't actually do any better.
Instead I took advantage of version control to split my changesinto two sets, in an order that is optimized for validation:
I committed all my changes to a new branch,jefftk-new-feature, with a message that only mentioned thefeature I was adding, not the refactoring or the new tests.
I made a new branch off of jefftk-new-feature torepresent the refactoring and new tests: git checkout -bjefftk-refactor.
On jefftk-refactor I deleted everything related tothe new feature, leaving only the refactoring and new tests. Iverified that the output was unmodified, as you'd expect for arefactoring-only change. I committed this, with a message that onlymentioned the refactoring and tests.
I interactively rebased (git rebase -i HEAD~2)this branch to squash the two commits into one. I only kept therefactoring commit message. This branch is now ready to be my firstPR, and is ready to send out for review.
I switched back to jefftk-new-feature, and rangit rebase jefftk-refactor. This rewrote my original bigcommit as if it was a follow-up to the refactoring commit. There werea few merge conflicts I needed to resolve manually, but nothing toopainful. This branch is now ready to be my second PR, and I'll uploadit to GitHub so I can reference it in the first PR in case thereviewer is interested in taking a peek at what's coming.
This was a bit more work, but I think it's definitely worth it: I'mmuch more confident that my refactoring didn't break anything, and Iexpect it to be much easier to review.
Comment via: facebook, mastodon, bluesky
Discuss
