Believing that it’s gentler on the ego to be told off by a computer than a fellow human, and that correcting style issues in the code review process distracts from more important matters, we started rolling out Scalastyle and Scapegoat in one of our Scala projects under active development.
Early enforcement of these standards was largely voluntary. For example, attempting to run a build locally using SBT would fail if there were style errors, but the build servers (which run Maven instead) would happily compile it. Despite the relaxed enforcement policy, after several days of work the project had been cleaned of lint, and several bugs were found.
After this early success, we decided to expand the effort to other languages and have lint checks enforced in the code review process.
The result of this initiative was a Python module that would be called by the build script with a list of projects modified by the commit. The script would walk each project’s directory, and run Pylint on any files ending in
.py, and shellcheck on any file ending with
.sh or starting with an appropriate shebang. If a
.scala file was encountered, it would look up a list of directories to project name mappings generated using
sbt baseDirectory and add the project name to a set of projects to run
sbt scapegoat on after the directory walk was finished. If a
project.clj file was found, it would run
lein kibit, and
lein bikeshed. While errors from Kibit and Bikeshed would be collected, only those from Eastwood were allowed to signal a failure as neither Kibit nor Bikeshed allowed the silencing of certain types of lint.
Since a file that hasn’t been modified recently is more likely to be obselete, if a Python or shell script hadn’t been modified this year it is grandfathered-in and ignored by the linter. This policy helped reduce the number of shell and Python warnings by a factor of 6.
As each project was cleaned, they would be added to the list of projects to enforce lint rules on — if somebody submitted a pull request for that project containing lint, the build would fail and they would be forced to clean it up before checking in. I made a deliberate decision to enforce lint only on pull requests — if things are dire enough to require a direct push to master, the time spent checking and enforcing lint would just get in the way. This could result in lint that would block later developers’ commits, but in practice this hasn’t been a problem.
In several weeks, we managed to eliminate all Python and shell lint warnings from the codebase. Here are some lessons we learned along the way:
- We generate a nightly report of lint reports remaining. These reports are rolled up into a progress report that we render using D3.js. This makes it easier to track progress and encourages efforts to eliminate the warnings.
- Most linters can be convinced to produce machine-readable output instead of, or in addition to, their normal human-readable output. This is extremely useful not only for generating reports of lint warnings in formats other than plain text, but also for distinguishing between levels of lint severity, rather than relying on the strict pass/fail of the linter’s return code.
- Code reviews go faster and more smoothly when fixes are grouped into commits by fix type rather than location. This is particularly relevant for Python fixes, where a script that had previously been indented with two-space indents or tabs might have every almost line changed. If the whitespace changes are in their own commit, it’s easier to verify that only whitespace is being changed, and that the behaviour of the code shouldn’t change.
- This is especially important for projects with a significant legacy component as it might be very difficult to test the changes necessary to fix lint “the right way”. In these cases, it is faster and safer to just suppress the warning locally. It might rankle that the lint hasn’t been cleaned up properly, but:
- Warning suppressions appear inline with the code, are easily searchable, and make it clear to any future maintainer that there is work there to be done.
- The longer you wait before enforcing lint errors as build errors, the more new lint can creep into the codebase.
- Linting is often an educational experience. While fixing lint warnings I learned a number of new things about the languages that we use, ranging from the fascinating to the frightening.
Although there’s still a lot of work left to do in fixing up much of the legacy Scala code, already the linting effort has paid off: multiple bugs were found during the clean-up effort, and other bugs were prevented from ever entering the codebase in the short time we have implemented our linting.