feat: respect suppressions during autofix#146
Conversation
mdjermanovic
added
the
Initial Commenting
|
In |
|
|
||
| The implementation would likely touch the following files: | ||
|
|
||
| - `lib/cli.js`: for CLI runs that use suppressions, resolve the active suppressions file before linting and make that suppressions context available to the `ESLint` instance used for fix generation. |
There was a problem hiding this comment.
Maybe we could clarify this point? I'm having a hard time figuring out what a "suppressions context" will include that isn't already available to the ESLint instance.
|
|
||
| - `lib/cli.js`: for CLI runs that use suppressions, resolve the active suppressions file before linting and make that suppressions context available to the `ESLint` instance used for fix generation. | ||
| - `lib/eslint/eslint.js`: initialize `SuppressionsService` when suppressions are needed for fix filtering as well as reporting, and load suppressions early enough in `lintFiles()` and `lintText()` for fix-producing runs. | ||
| - `lib/eslint/eslint-helpers.js`: compose the existing fixer created in `lintFile()` and `verifyText()` with a suppressions-aware predicate. For the current file, if `message.ruleId` appears in that file's suppressions entry, return `false` so the fix is skipped. |
There was a problem hiding this comment.
It seems that a fixer is only created in lintFile(). verifyText() receives it as a parameter.
|
|
||
| ## Open Questions | ||
|
|
||
| Should ESLint provide any user-facing indication that fixes were skipped because the corresponding file-and-rule pairs are suppressed? |
There was a problem hiding this comment.
Saying that fixes were skipped because of suppressions doesn't imply they would have been applied otherwise, so the information may be less interesting to users than it sounds at first.
There was a problem hiding this comment.
Fixes for suppressed lint problems are already available to custom formatters and API consumers in LintResult#suppressedMessages. I don't think there should be an additional indication provided by ESLint CLI. ESLint API, or built-in formatters, because the lint problems they are fixing are effectively ignored.
… get inadvertently formatted eslint/rfcs#146
nzakas
left a comment
nzakas
left a comment
There was a problem hiding this comment.
Thanks for putting this together. Generally I'm in favor of the behavior described in this RFC. I left a note about the technical implementation as I'm not quite following what you're envisioning.
| - `lib/cli.js`: for CLI runs with `--fix` or `--fix-dry-run`, resolve the active suppressions file before linting, load its current contents, and pass that suppressions snapshot into the `ESLint` instance used for fix generation. This makes autofix depend on the suppressions file contents loaded before linting begins. | ||
| - `lib/shared/translate-cli-options.js`: pass that internal suppressions snapshot through CLI option translation so it reaches the `ESLint` instance. |
There was a problem hiding this comment.
Can you explain what the "suppressions snapshot" is?
5 participants
Summary
This RFC proposes that ESLint should stop applying autofixes for a rule in any file where that rule is suppressed via the suppressions file.
Related Issues
eslint/eslint#20062