Adding New Lint Rules Without the Fuss

Linters are great for maintaining code quality and encoding team conventions, but how do you add new rules that your codebase currently violates? If there's a handful of violations or the violations can be autofixed, then it may be easy to fix them before adding the rule, but what if there are hundreds of them?

Case Study

Suppose we've already set up CI for linting and want to add the ESLint rule import/extensions to ensure that every import has a file extension. Let's walk through some options at our disposal and consider the pros and cons of each option.

Option 1: Fix Every Violation

The first option available is to fix every violation that comes up from the new lint rule.

The Setup

First, add the new rule:

diff --git a/.eslintrc.json b/.eslintrc.json
   ...
   "rules": {
+    "import/extensions": ["error", "always"]
   }

There's now lint errors and we can't merge to our main branch without failing CI, so we fix every error before merging. While time consuming, this process is straightforward. You go through each lint violation in the codebase and fix it - in this case, by adding a file extension to every import that is missing one.

Pros

The codebase is 100% adhering to the new rule! There are no lint violations, and everyone in the future will follow this rule in their changes or face the wrath of a failing build. This strategy is awesome when there's time and motivation to get it done.

Cons

When there are hundreds of warnings that can't be autofixed, this strategy will postpone or prevent you from getting value out of new rules.

Option 2: Make the New Rule a Warning

What about adding the new rule as a warning instead of an error?

The Setup

First, add our new rule:

diff --git a/.eslintrc.json b/.eslintrc.json
   ...
   "rules": {
+    "import/extensions": ["warn", "always"]
   }

and we're done!

Pros

Setup was super easy - there's now a new lint rule that developers will see in their editors if they use an ESLint plugin.

Cons

There's nothing really enforcing the new rule. It's just a warning, and there may be hundreds of other warnings in the codebase. Warnings will pile up without developers noticing them.

Mitigations

ESLint has a CLI option --max-warnings which enforces a max number of warnings. Unfortunately, as you fix any existing warnings you have to keep this up to date, otherwise each fix gives slack for future warnings.

Option 3: Suppress the ESLint Errors

We could suppress the existing violations to enforce the new rule going forward while avoiding the immediate cost of fixing existing issues.

The Setup

We'll add the new rule, and then add eslint-disable-next-line for each lint violation.

First, add the lint changes to .eslintrc.json, same as option 1:

diff --git a/.eslintrc.json b/.eslintrc.json
   ...
   "rules": {
+    "import/extensions": ["error", "always"]
   }

Then run suppress-eslint-errors. The suppress-eslint-errors package notes that you may have to manually fix some of the suppressions it adds. If your setup doesn't involve ESLint, you'll need to find an alternative to suppress-eslint-errors, or may have to do this part manually.

npx suppress-eslint-errors src/**/*.{ts,tsx} --message="TODO: add extension"

diff --git a/src/App.test.tsx b/src/App.test.tsx
 import { render, screen } from '@testing-library/react
+// TODO: add extension
+// eslint-disable-next-line import/extensions
 import App from './App';

See this example of a suppress violations diff for the changes needed to add this to a small project.

Pros

Suppressing existing failures keeps our lint warnings clean and allows us to enforce future changes don't violate the new rule. You can go back and systematically fix suppressed violations in smaller chunks.

Cons

The lines suppressing warnings reduce the signal-to-noise ratio of the code. It can also make it seem ok to add eslint-disable whenever a developer writes code that violates lint rules, reducing the effectiveness of linting.

Option 4: Only Lint New Changes with New Rules

With a little extra work and a slightly more complicated setup, we can achieve linting that will ignore existing violations, while keeping us accountable in new changes. I like to call this marginal linting.

Using a tool like reviewdog (or pronto if you like ruby), we can set up GitHub checks to annotate our PRs with any lint violations.

The Setup

We'll have two separate ESLint configs now. The "main" ESLint config (.eslintrc.json) adds the new rule. This is our default config which we run in editors as well as in reviewdog.

First, add the lint changes to .eslintrc.json, same as option 1.

diff --git a/.eslintrc.json b/.eslintrc.json
   ...
   "rules": {
+    "import/extensions": ["error", "always"]
   }

Our second ESLint config will intentionally disable the newly added rule in CI. Target it in the lint workflow with eslint --config=.eslint-ci.json.

// .eslintrc-ci.json
{
  "extends": ".eslintrc.json",
  "rules": {
    "import/extensions": "off"
  }
}

Add a new GitHub workflow using the reviewdog eslint action to execute our new rules on pull requests.

# .github/workflows/reviewdog.yml
# Modified from reviewdog action eslint README
# https://github.com/reviewdog/action-eslint#githubworkflowsreviewdogyml
name: reviewdog
on: [pull_request]
jobs:
  eslint:
    name: runner / eslint
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@v1
      - name: Lint Typescript Changes 👕
        uses: reviewdog/action-eslint@v1
        with:
          reporter: github-pr-check
          eslint_flags: "--config=.eslintrc.json src/**/*.{ts,tsx}"

See this example reviewdog setup diff for the changes needed to add this to a small project.

The Result

We now receive warnings in our pull requests whenever our changes violate any lint rules, including our existing ones.

Pros

Making .eslintrc.json the more restrictive configuration ensures that any new integrations will follow it by default. Any use of .eslintrc-ci.json can be explicitly specified such as in CI.

This setup has the added benefit of including code review integration, which can be beneficial regardless of new lint rules. It also means that any new rules require a two line change: one for the lint rule in .eslintrc.json and another to disable it in .eslintrc-ci.json.

Cons

The setup for this option is more involved, and adds a new technology to the CI stack. The build time for this task in a new create-react-app was 3 minutes, and could increase depending on the project size.

Conclusion

While it's nice to have a 100% compliant codebase, it might not be possible to fix every violation immediately. Minimizing the effort of adding new lint rules helps ensure your team can adopt and enforce best practices moving forward.

Running a script to disable lint errors for new rules can quickly fix the issue but remains the same effort for every future lint rule. Adopting two lint configs, while requiring a slightly more complex initial setup, provides the same benefit and allows you to add new rules easily. Integrating it with reviewdog or pronto makes encouraging the new practices even easier in code review.