Also on The ReadME Podcast
Python enthusiast, Twitch streamer, et. al.
When I first encountered code review I got a completely wrong view of what it was about. I'd submit a patch for review and the vast majority of comments were about naming conventions, import ordering, the type of quotes I used, and even whitespace! My reviewers and I were spending a vast majority of our effort deliberating over relatively trivial problems and missing important architectural and structural issues. Fortunately, computers are really good at identifying these types of "nitpicks". In this article I'll discuss some techniques and strategies for removing noisy style and convention discussions from code reviews. Along the way we'll address some processes that can improve reliability without sacrificing productivity (in fact, they might actually improve productivity!) as well as some learnings of implementing these systems at scale.
Introducing linters
Fortunately, we don't need to spend human time nitpicking these things — computers are very effective at finding problems in code. Linters (or "checkers") are programs that identify common problematic patterns. Beyond preventing common pitfalls, linters can also be a useful tool for improving reliability (beyond preventing common pitfalls).
Start with the least controversial and highest value tools — ones that identify correctness issues. Most languages have some form of tool to help you identify things like unknown variables, missing imports, or other common mistakes that could be caused by typographical errors. Emphasizing high signal-to-noise checkers while introducing new tooling is the most successful approach: they produce immediate value without being annoying or getting in the way of progress. Identify a problematic code pattern that caused an incident? Write a linter to prevent it from ever happening again!
The next step is to take any of your "coding conventions" and encode them into a checker of your own. Many popular linting frameworks make it easy to extend existing rules or write your own custom rules to suit your development organization or domain-specific needs. When rolling out an internationalization framework at Lyft the team leaned heavily on a custom flake8 (Python linter) plugin to enforce best practices. At Stripe we did similarly to prevent misuse of a security framework. Encoding institutional knowledge into custom lint rules helps ensure best practices without needing domain or framework experts to be present. If it's difficult to encode a rule: throw it out!
But formatters are better than linters!
What's better than a computer telling you about a problem? A computer automatically fixing that problem! When possible, prefer a code formatter (also known as an "autofixer" or simply "fixer") over a linter as they're lower friction to add and less frustrating to use. Formatters can fix issues as simple as removing trailing whitespace or fixing newlines to or as complex as automatically rewriting code to match best practices. Especially tedious or inconsequential things should be delegated to a formatter such as import sorting, whitespace, trailing-commas, etc. If your language has an agreed upon formatting standard (such as `go fmt`) use it.
Version code quality along with your repository
A common mistake when rolling out code quality tooling is managing it outside of the target repository. This may work acceptably for the initial implementation but will make it much more difficult to upgrade down the road. When a linter or formatter changes in a new version one will have a difficult time performing a lockstep upgrade—some contributors may be on feature branches with outdated conventions.
This can be especially difficult if your formatting tools are written in many different programming languages—a place where pre-commit shines. Though written in Python, pre-commit supports a number of different programming languages. You specify your tools and versions in its configuration file and it makes sure the proper versions are installed automatically. Because this configuration file is versioned with the repository, developers are always running exactly the same linter versions at a particular revision.
Run early and often
To speed up your development pipeline, you should be running your linters and code formatters as early as possible. Many popular tools have editor plugins to point out checker issues or format your code on save. Git hooks can detect issues as you commit or as you push to catch issues before review. And even though you have all of these tools set up, it's still a good idea to validate your code quality as part of your continuous integration pipelines—sometimes you may be developing away from your normal setup or maybe one of your developers doesn't run the local tooling. Even better than checking for issues in CI is to fix them automatically!
Ok I'm sold, now what?
There are several approaches to enabling code quality tooling, let's review some of the more common ones and their pros and cons.
First is an "incremental" approach. This is by far the easiest from an implementation perspective. While this approach works well for checkers—it is less than ideal for code formatters. With formatters, you'll need many incremental changes to roll out to the entire code base, which increases the number of times another developer can encounter a merge conflict. The best approach to incremental adoption is to start by opting the entire codebase to the new rule but allowlisting all current violations. Then track the current violations as an externally visible metric and do some "graph driven development" to drive the violations down to zero. With any migration, the more automation you can leverage to automatically fix violations the better!
Next is the "do it all" approach. This can work well for new linters that have very few existing violations because the implementer can simply fix the existing problems. This is also the recommended approach for introducing code formatters. While a mass reformat can be disruptive it tends to only be disruptive once—avoiding potential waves of merge conflicts from an incremental rollout. This limits the merge conflicts to at most one per feature branch—which is still a lot! Try and schedule large code-mods at the least disruptive times, such as weekends or off hours rather than midday on a no-meeting-Wednesday. For example, we switched indentation from tabs to spaces over December at Yelp over a holiday break.
The last approach is an "on touch" incremental adoption. In this mode, the quality tooling is left in an unenforced mode but enabled locally. When a developer touches a file the tooling requires changes to be made to bring the code in line with standards. his is often the easiest to adopt because it requires little to no infrastructure investment, but it leads to an undesirable result: the actual changes are obfuscated by unrelated formatter / linter changes. For that reason, this approach isn't recommended.
But mah git blame!
One additional downside to mass reformats is it can make it more difficult to trace the history of code through `git blame`. Fortunately, git has a feature which makes ignoring formatting changes very easy: --ignore-revs-file. Set up a file which includes your reformatting revisions:
```
# code was reformatted using pre-commit + black
c58a4662d8920cf70ea688edd9eaf9d783a856a7
```
and then `git blame --ignore-revs-file=.git-ignore-revs` will preserve blame from before the reformat!
A related tip is git's `-w` option (available for blame, diff, show, etc.) which ignores whitespace changes in patches. GitHub even supports this as well if you append `?w=1` to the url.
Take home
Adoption of code quality tooling can be arduous and disruptive, but it’s well worth the effort. I now rarely spend time in code reviews on style and conventions—that responsibility falls almost entirely to computers. When I identify opportunities to make style improvements, instead of conversations about the more pedantic aspects of programming, I now think about how to fix the issue with tooling. It’s a shift that makes code reviews more effective — and a lot more fun.