From 7401dff0b505c979829bf1fb0f39912ece6f44e0 Mon Sep 17 00:00:00 2001 From: Domantas Petrauskas Date: Fri, 10 Jun 2022 01:17:19 +0300 Subject: [PATCH] feat: add cli option for suppressing ignored file warnings --- .../README.md | 184 ++++++++++++++++++ 1 file changed, 184 insertions(+) create mode 100644 designs/2022-supress-ignored-file-warnings/README.md diff --git a/designs/2022-supress-ignored-file-warnings/README.md b/designs/2022-supress-ignored-file-warnings/README.md new file mode 100644 index 00000000..ffa0fe1a --- /dev/null +++ b/designs/2022-supress-ignored-file-warnings/README.md @@ -0,0 +1,184 @@ +- Repo: eslint/eslint +- Start Date: 2022-05-13 +- RFC PR: +- Authors: Domantas Petrauskas + +# CLI option to supress ignored file warnings + +## Summary + + + +When ignored files are explicitly linted, ESLint shows a warning. This causes problems when using tools which pass the file list to ESLint automatically together with `--max-warnings 0` option. + +## Motivation + + + +While this is warning is reasonable in cases where filenames are passed to ESLint manually, it causes issues when the process is automated with tools like [lint-staged](https://github.com/okonet/lint-staged) and [pre-commit](https://github.com/pre-commit/pre-commit). These tools pass staged filenames to any command, not just ESLint. Therefore, they are not aware of the .eslintignore. Linting before commit is commonly set up with `--max-warnings 0`, therefore ESLint will exit with an error in such case. + +For example, with + +``` +// .eslintignore +foo.js +``` + +Running + +``` +eslint foo.js +``` + +Will result in + +``` +warning File ignored because of a matching ignore pattern. Use "--no-ignore" to override +``` + +It is possible to filter out ignored files using ESLint `CLIEngine`. This was the main reasoning to reject the CLI flag idea during [TSC meeting in 2018](https://gitter.im/eslint/tsc-meetings/archives/2018/08/02). The commitee was only considering the use case of integrations. However, this problem affects end users as well. Lint-staged FAQ includes a section [how can I ignore files from .eslintignore](https://github.com/okonet/lint-staged#how-can-i-ignore-files-from-eslintignore), which suggests adding additional config file and using `CLIEngine.isPathIgnored` to filter out ignored files. Having a CLI flag for this would improve the end-user experience. + +## Detailed Design + + + +ESLint CLI should have an optional flag to disable the ignored file warning. The flag should be boolean. In order to stay consistent with current CLI flags, it should be called `--no-warning-on-ignored-files`. If it is present, ESLint should supress the `File ignored because of a matching ignore pattern.` warning. The flag should replicate `eslint.lintText`[warnIgnored option](https://eslint.org/docs/developer-guide/nodejs-api#-eslintlinttextcode-options) when it is set to `false`. + +The [3 warnings](https://github.com/eslint/eslint/blob/f31216a90a6204ed1fd56547772376a10f5d3ebb/lib/cli-engine/cli-engine.js#L299-L305) related to ignored files should be appended with `Use "--no-warning-on-ignored-files" to suppress this warning.` + +Add the flag to `optionator()` in `options.js`: + +```js +{ + option: "warning-on-ignored-files", + type: "Boolean", + default: "true", + description: "Show warning when the file list includes ignored files" +}, +``` + +It should be placed under `heading: "Miscellaneous"`. + +Add `warningOnIgnoredFiles` to `translateOptions()` in `cli.js`. It should be returned as `warnIgnored`. Forced `warnIgnored: true` should be removed [here](https://github.com/eslint/eslint/blob/f31216a90a6204ed1fd56547772376a10f5d3ebb/lib/cli.js#L298) in order for CLI to respect the `--no-warning-on-ignored-files` flag when `useStdin` is true. + +Destructure `warnIgnored` from `options` from `internalSlotsMap.get(this)` in `executeOnFiles()` function in `cli-engine.js`. + +In `iterateFiles()` of `executeOnFiles()` function in `cli-engine.js`, wrap the `results.push(createIgnoreResult(filePath, cwd));` with `if (warnIgnored)` to suppress the warning for ignored files. Keep in mind that the ignored file should not be linted. + +Destructure `warnIgnored` from `options` from `internalSlotsMap.get(this)` in `executeOnText()` function in `cli-engine.js` and rename it to `engineWarnIgnored` in order to distinguish it from the `warnIgnored` parameter. Update the `if (warnIgnored)` condition to fallback to `engineWarnIgnored` in case `warnIgnored` parameter is not provided. + +Remove the code which overrides the default value of `warnIgnored` [here](https://github.com/eslint/eslint/blob/f31216a90a6204ed1fd56547772376a10f5d3ebb/lib/cli.js#L298) and [here](https://github.com/eslint/eslint/blob/main/lib/eslint/eslint.js#L571) + +The default `warnIgnored` value should only be set in `default-cli-options.js` and `eslint.js` `processOptions`. It should be set to `true`. The CLIEngine and ESLint classes will inherit this and use it in `lintText`, `lintFiles`, `executeOnFiles`, and `executeOnText`. This is a breaking change, as previously `lintText` and `executeOnText` functions were defaulting to `warnIgnored: false`. This change will improve consistency. + +## Documentation + + + +The [CLI Documentation](https://eslint.org/docs/user-guide/command-line-interface) should be updated to include the new flag. Additionally, [Node.js API documentation](https://eslint.org/docs/developer-guide/nodejs-api#-new-eslintoptions) will need to add additional option under `new ESLint(options)`. + +[Ignoring Code](https://eslint.org/docs/user-guide/configuring/ignoring-code) page will need to be updated to reflect the changes to the error messages. + +## Drawbacks + + + +Adding additional CLI flags adds additional mental overhead for the users. It might be confusing for users who don't experience this problem. + +## Backwards Compatibility Analysis + + + +It should not affect existing users in any way. + +The `CLIEngine.isPathIgnored` solution will continue working, it is `lint-staged` and similar tools responsibility to communicate that it is possible to use the flag instead. + +## Alternatives + + + +It was considered to output the warning to `stderr` instead, but that would cause breaking changes. + +## Open Questions + + + +ESLint [collects suppressed warnings and errors](https://github.com/eslint/eslint/pull/15459). In case `--no-warning-on-ignored-files` or `warnIgnored: false` is used, should the warning be included in the suppressed messages array? + + + + + + + + + +## Related Discussions + + + +Main issue: + +https://github.com/eslint/eslint/issues/15010 + +Related issues: + +https://github.com/eslint/eslint/issues/9977 + +https://github.com/eslint/eslint/issues/12206 + +https://github.com/eslint/eslint/issues/12249