Code review isn't just about code
April 6, 2021
When you review a pull request, what do you look for?
Bugs? Misspelled words? Broken error handling? Missing unit tests?
That’s all fine and good. But there’s one aspect of pull request review that I think usually does not get enough attention: Metadata
The pull request title and description, as well as the individual commit messages (and each commit’s contents) are all important parts of a pull request, and deserve attention during a review.
To get you started thinking about this, here are some things I always look for when reviewing a PR:
- Does the content of the PR match the title?
- Does the PR description adequately explain the intention? (i.e. If you must ask for clarification, the description is probably lacking)
- Does the PR address one concern (i.e. a single bug fix, a single refactor, or a single feature)
- Does each commit message make sense in isolation? (i.e. “Bugfix” or “commit a missing file” don’t pass muster)
"Readability" is subjective
Ask yourself: Will the least experienced developer likely to read this code be able to understand it?
Go Code Roast #2: readability.js port
Go Code Roast
In this video, I roast some Go code! That is, I review it as if it were submitted as part of a job application screening. I talk about what I like, what I don't like, and how I would do things differently.