Repository navigation
feat: respect suppressions during autofix - #146
Conversation
mdjermanovic
added
the
Initial Commenting
|
In |
… 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?
There was a problem hiding this comment.
Sorry for the late reply, I missed the notification. I added a definition paragraph to the RFC explaining what the “suppressions snapshot” is.
There was a problem hiding this comment.
Is the idea to add a new option to the ESLint constructor that allows passing in the suppressions snapshot?
There was a problem hiding this comment.
Yes, but strictly internal. I was thinking we could use an internal Symbol to pass the snapshot through the constructor options, similar to how disableCloneabilityCheck works, to avoid expanding the public API.
There was a problem hiding this comment.
Makes sense to keep the public API unchanged.
nzakas
left a comment
nzakas
left a comment
There was a problem hiding this comment.
I think this makes sense as a feature and the plan looks sound to me.
|
@sethamus there's some feedback for you to review here. |
| A **suppressions snapshot** is the in-memory representation of the `eslint-suppressions.json` file contents, read once at the start of the lint run. By capturing the suppressions data before linting begins, autofix decisions are based on a stable, point-in-time view of the file rather than a version that may change during the run (for example, if `--suppress-rule` writes new entries to the file during the same invocation). | ||
|
|
||
| - `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 snapshot will be passed via the `ESLint` constructor options using an internal `Symbol`. This makes autofix depend on the suppressions file contents loaded before linting begins. | ||
| - `lib/eslint/eslint.js`: initialize `SuppressionsService` when suppressions are needed for fix filtering as well as reporting. For Node.js API calls with `applySuppressions` and `fix` enabled, load suppressions early enough in each `lintFiles()` or `lintText()` run before building the fixer. In `lintText()`, compute the suppressed rules only when `filePath` is provided and build the suppressions-aware fixer before passing it to `verifyText()`. For worker threads, pass the suppressions snapshot through `workerData`. |
There was a problem hiding this comment.
Just a note that the ESLint constructor already initializes a SuppressionsService (with filePath and cwd) and associates it with the new instance. I think that's all we need. For fix filtering in particular, a SuppressionsService shouldn't be necessary as the suppressions file location would be irrelevant.
fasttime
left a comment
fasttime
left a comment
There was a problem hiding this comment.
LGTM. Leaving open for @mdjermanovic to review.
mdjermanovic
left a comment
mdjermanovic
left a comment
There was a problem hiding this comment.
LGTM, thanks! Moving to final commenting.
mdjermanovic
added
Final Commenting
|
There have been no further comments during the final comment period, so we can merge the RFC. |
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