What is a programmer but a series of PRs? I optimize PRs to introduce the best code I can, be easy to review, and document my work so I can make sense of it in the future. Here are some things I always check before opening a PR.
Passing Static Analyses
All of the static analysis tools in my project must pass locally.
On a JavaScript project, that’s an auto-formatter (Prettier), linter (ESLint), and type-checker (TypeScript). All of these run on files I touch automatically. If that’s not feasible for you, a Husky pre-commit hook will do the job.
Passing Tests
All of the tests in the project must pass locally.
For a JavaScript project, I run tests in an IDE window that I can always see, in watch mode:
npm test --watch
Again, a Husky hook can help.
Tests Added
Did I add or extend tests?
In the main languages I write (Ruby, Elixir, JavaScript, TypeScript, Python), if I’m not adding or extending tests, I’m failing.
Are they the right tests for the work? New screen— new integration test. Bugfix in a problematic function— unit test.
Good Names
Are my names solid?
The names I chose while writing the feature are usually just okay. They deserve a second look.
Does my function name describe itself? Does my component match the domain? Renames are easy bike-shed. I’m mostly asking: Did I try my best to choose the best names I can think of? Will somebody who hates the feature I’m adding, begrudgingly admire my naming?
I make renaming easy so I don’t have an excuse to skip this.
Relevant Changes Only
Am I checking in changes that don’t matter?
I touched ten files while exploring, but I ended up only editing one. I should probably only check in changes to that one file.
“The Boy Scout Rule” says “leave the code better than you found it.” But, we don’t want a tight set of changes to be muddied by wandering refactors and tooling fixes. I rebase out anything that isn’t relevant.
Git Log Rebased
Have I gotten rid of all the “Trying this” and “Quick push 😜” commits on my branch? Is my branch up to date with its source?
These commands let me put my feature branch on the tip of the source branch and squash, delete, fixup, or reword all of the janky commit messages I wrote.
# Update my source branch
git checkout source-branch
git pull --rebase
# Update my feature branch and squash
git checkout feature-branch
git rebase --interactive feature-branch
Comments and Annotations
Did I document, as best I could, what I was trying to do?
Update the README. Name things. Write comments. Yes, comments have their drawbacks, but I think software engineers are generally way less communicative than they could be. Let’s worry about comments when we think we’re writing too many. That has almost never been my experience.
Edge Cases Considered
Have I considered obvious edge cases?
If I’m adding a form validation, what does the form look like before I enter a value? Is there a reasonable input that my change doesn’t account for?
We aren’t trying to eliminate edge cases in our work— that’s impossible. We’re trying to think ahead and be honest about the trade-offs.
Smoke Test
Does the feature actually work exactly as described in the ticket?
Sometimes my work doesn’t quite pass this test. It doesn’t sing. Fix it now.
Great PR
Is my PR a future-proof artifact of my work?
Does it tell the why, what, and how about what I was doing, link to a ticket, and include screenshots as needed? Did I tag the right reviewers?
Self-Review
I like to go through my PRs and leave a few comments in the code.
A few examples:
“This is a real gotcha! If this value isn’t numeric, the app crashes. I wrote a ticket.”
“This is probably the last time we can reuse this function before a refactor.”
“This test cheats because it mocks the main component it imports. But, we need some test coverage here.”
These comments do a lot: they show how I think, make me justify my choices, and open lines of code up to critique.
Conclusion
This is a lot, and yet, I’ve probably forgotten a few steps. I’ve picked these up over the years from making mistakes. The result is that I open PRs I’m proud of most of the time, that do the work I’ve been asked to do. That’s what it’s all about.