Formed in 2009, the Archive Team (not to be confused with the archive.org Archive-It Team) is a rogue archivist collective dedicated to saving copies of rapidly dying or deleted websites for the sake of history and digital heritage. The group is 100% composed of volunteers and interested parties, and has expanded into a large amount of related projects for saving online and digital history.
feat: skip warnings cli flag #104
Conversation
|
Hi @me4502!, thanks for the Pull Request The first commit message isn't properly formatted. We ask that you update the message to match this format, as we use it to generate changelogs and automate releases.
To Fix: You can fix this problem by running Read more about contributing to ESLint here |
|
|
Is there a reason we couldn’t make “quiet” have this behavior? |
This is one of the alternatives considered in the RFC. IMO it would make more sense to the user, but would also introduce a few confusion points around the existing flags that make use of warnings, such as |
|
|
||
| ## Alternatives | ||
|
|
||
| ### Implement this into `--quiet` instead of a new flag |
There was a problem hiding this comment.
I prefer this alternative. It seems much more intuitive to me for the --quiet option to avoid running rules that won't influence the results anyway. I also generally believe we should use breaking changes to fix existing behavior that is unintuitive, instead of simply tacking on additional options as workarounds.
There was a problem hiding this comment.
Why exactly would it be a breaking change to stop running rules that won't influence the results?
There was a problem hiding this comment.
I suppose that rules could be modifying shared state or have other side effects while running? I'm not sure if that's supposed to be allowed or not or if we need to worry about it here.
Regardless of that, it sounds like there's another potential breaking change related to the interaction with --max-warnings flag.
There was a problem hiding this comment.
what if it only skipped the rules when max-warnings wasn't provided? Then it would automatically do what made the most sense.
There was a problem hiding this comment.
I suppose that rules could be modifying shared state or have other side effects while running? I'm not sure if that's supposed to be allowed or not or if we need to worry about it here.
We have one case where rule API actually encourages side effects: context.markVariableAsUsed(). For example, react/jsx-uses-vars calls this so that no-unused-vars doesn't report variables that are referenced in jsx elements. If someone has "react/jsx-uses-vars": "warn" in their config, and we don't run this rule, then they'll start getting no-unused-vars errors.
I'm also in favor of not introducing another flag, but not running rules set to "warn" indeed looks like a breaking change even without --max-warnings.
| Another potential drawback around this specific implementation is that the existence of | ||
| both `--quiet` and `--skip-warnings` might be confusing to some users, however this can be | ||
| partially alleviated by updating the `--quiet` docs to mention that warnings will still be | ||
| run, and to use `--skip-warnings` instead to not run them. |
There was a problem hiding this comment.
It's quite hard for me to guess the difference between --quiet and a hypothetical --skip-warnings option just from their names. While the docs could explain this, I think it's a sign that the design and naming may not be intuitive to users.
| Another downside of this alternative is that it would introduce confusion around the | ||
| `--max-warnings` flag and other checks that require warnings to run. Currently the quiet | ||
| flag works fine alongside these. If we were to implement this into the quiet flag, we would | ||
| have to either break that functionality, or have the quiet flag only sometimes stop actual | ||
| running depending on what other flags are in use, which is confusing and unclear. |
There was a problem hiding this comment.
Since this alternative is a breaking change, I think we could reconsider interactions between flags as needed, and even throw an exception when multiple flags are provided that don't make sense to be specified together.
It would help to create a table explaining the current behavior with each combination of flags and also how the behavior would change for each combination (and whether it would still be allowed).
| This change adds a new command line flag, as well as a new API method. This adds further | ||
| maintenance burden. However, the implementation is relatively simple, and the benefits | ||
| are significant. |
There was a problem hiding this comment.
Since this new flag would have interactions with several existing related flags, the potential implementation/maintenance/documentation burden is multiplied (making sure every possible combination of flags behaves correctly and that users understand the expected behavior of each combination).
| @@ -0,0 +1,129 @@ | |||
| - Repo: eslint/eslint | |||
| - Start Date: 2023-01-16 | |||
| - RFC PR: (leave this empty, to be filled in later) | |||
There was a problem hiding this comment.
| - RFC PR: (leave this empty, to be filled in later) | |
| - RFC PR: <https://github.com/eslint/rfcs/pull/104> |
mdjermanovic
added
Initial Commenting
Summary
Currently, ESLint will run all rules that are not marked as
offin the configuration.This RFC proposes adding a way to configure which rules are actually run, to reduce linting
time and better match the reporting outcome. Currently, when a rule is marked as
warn,ESLint will still run the rule but not report the results when run under
--quiet. Theintended outcome of this RFC is to allow users to not run these rules when unnecessary.
Related Issues
eslint/eslint#16450