feat!: skip warnings cli flag

[me4502]

Summary

Currently, ESLint will run all rules that are not marked as off in 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. The
intended outcome of this RFC is to allow users to not run these rules when unnecessary.

I have a very rough test implementation patch here, https://gist.github.com/me4502/e75e4677d3ff17ad585fcab0ac5dc6e9. It is missing a few things and is not very clean, but was done to assist in writing the design details of the RFC itself.

Related Issues

eslint/eslint#16450

[eslint-github-bot]

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.

• The commit message tag wasn't recognized. Did you mean "docs", "fix", or "feat"?
• There should be a space following the initial tag and colon, for example 'feat: Message'.
• The first letter of the tag should be in lowercase

To Fix: You can fix this problem by running git commit --amend, editing your commit message, and then running git push -f to update this pull request.

Read more about contributing to ESLint here

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: me4502 / name: Maddy Miller (416404b, 425e124)

[ljharb]

Is there a reason we couldn’t make “quiet” have this behavior?

[me4502]

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 --max-warnings which currently work fine alongside --quiet. It'd also be a breaking change

[nzakas]

Thanks for putting this together! It seems like a good idea although the consensus seems to favor implementing this behind the --quiet flag rather than adding a new one. Can you please update your proposal with that in mind?

[me4502]

I'm currently going through processes to get the corporate CLA signed - Will make all requested alterations once that is all sorted :)

[nzakas]

@me4502 sounds good!

[nzakas]

@me4502 there are a couple of other comments left by @mdjermanovic. Can you please take a look at those?

[me4502]

I'll apply the changes by the end of the week - just been a bit busy :)

[nzakas]

Maybe filterRules() would be clearer than rulePredicate here?

[nzakas]

It looks like this would be called once for each rule -- if someone has hundreds of rules enabled, I'm wondering about a perf hit of this approach vs. having a function that just inspects the entire rules section of a config and just returns the pieces that should be run. What do you think?

[bmish]

or ruleFilter

[me4502]

The name change makes sense; will do

My only concern around the performance change there is, it'd still be constructing an array from the configuredRules and grabbing all the data, and then having to re-construct the configuredRules list to later iterate on. There'd be a fair bit of double-ups on data access, creation of an array, and filtering/mapping/etc. I feel that overhead might outweigh function calls here in most cases

[nzakas]

Sorry, I'm not quite following. What change are you referring to here?

I think running this function over every rule is the biggest perf hit we'll take, so I'd like to see if there's a way to avoid that.

[me4502]

Yeah that was my main concern with switching to this style given in any case it'd be checking against every rule and this might be slower due to the extra array allocation (#104 (comment)), although my original concern of duplicated data access didn't end up being an issue in my implementation. While it could be replaced with a for loop and a new array, I feel I trust the inbuilt filter function here to perform the array side of things faster than writing it in userland would be.

I can't think of a way that this overhead could be entirely negated, aside from allowing this function to be undefined and not calling it when it's not in use. That would remove the overhead in cases where it won't likely lead to a performance improvement, but might complicate the API a little more

[nzakas]

Sorry, finally getting back to this. What I was thinking is instead of doing this:

{
    ruleFilter(ruleId, rule, severity) {
        return severity === 1;
    }
}

In this case, we are running the function once on every rule when two of those parameters are actually already available inside of the rule config (and it seems like we don't have a use case for the second parameter currently?).

We would instead just pass in the rules config and return a filtered one.

{
    ruleFilter(rulesConfig) {
        return Object.fromEntries(Object.entries(rulesConfig).filter(([,options]) => options[0] === 1));
    }
}

[nzakas]

Okay, so after pondering this a bit more, it seems clear to me that there is no performance difference between the two options I listed in preceding comment. Either way, we run a filter on every rule, so the proposal in the RFC is fine as-is.

I think the next question is around the arguments to the predicate. I'd propose we just start with the ruleId and the severity, as that's our only use case right now. If we end up with other use cases in the future, we can add more.

[me4502]

Thanks - I've removed the rule option from the RFC document

[me4502]

In this case, would you be happy for me to go back to how it was beforehand where it ran per-rule while iterating through the configuredRules array? The current method where it creates a new array from my testing is actually slower

Edit: actually it might make more sense to keep it as-is, as it means the API can be used to contextually disable a rule if another rule is enabled (eg, disabling a non-type-aware rule if the type-aware one is enabled). The performance difference is negligible anyway

[nzakas]

I think this is in good shape now, thanks for your patience throughout this process. We know that the RFC process can take a while and we really appreciate you sticking with us.

I'd like @mdjermanovic to review one more time but overall I think we are good to go.

[mdjermanovic]

Since this is a new API option that can be used independently of this feature, I'm more in favor of the previous version: ruleFilter({ ruleId: string, severity: number }): boolean, as it is simpler and there's no ambiguity about what's allowed because it can only filter out rules. A function that gets a list and returns a list may appear to be allowed to change severities or even add new rules, which may or may not work by chance depending on how we implement handling return value.

If we get a request to pass all the rules so that the function can contextually disable a rule if another rule is enabled, as noted in #104 (comment), we could add them as a second argument. That would be similar to Array.prototype methods.

@nzakas what do you think?

[nzakas]

I agree. I actually misread the latest update and thought that's what we were doing now, so thanks for pointing it out.

[nzakas]

In the interest of time and since we are already so close, I went ahead and added suggestions to get this RFC into the state we'd like before merging. Please take a moment to review and make sure it makes sense.

[nzakas]

Suggested change

	From an API perspective, this would be implemented by a filter function that filters down to
	which rules should be run. The function would take a list of the rule configuration `(ruleId, severity)`,
	and return the rule that should be run. For simplicity, this function should not be
	given rules marked as `off`, as if this function handled existing behavior then users of the
	API would have to mimic that when attempting to extend it.
	From an API perspective, this would be implemented by a filter function that filters down to
	which rules should be run. The function would take `(ruleId, severity)` and return true to include and rule or false to exclude it. For simplicity, this function should not be
	given rules marked as `off`, as if this function handled existing behavior then users of the
	API would have to mimic that when attempting to extend it.

[nzakas]

Suggested change

	In `cli.js`'s `translateOptions` function, the `filterRules` option should be assigned to
	a function that filters to rules with a `severity` of 2 (error) when the `--quiet` flag is applied, otherwise
	acts as an identity function. In `eslint/flat-eslint.js`, the `filterRules` should be taken from
	the `eslintOptions` object, and passed down to the `linter.verifyAndFix` call.
	In `cli.js`'s `translateOptions` function, the `ruleFilter` option should be assigned to
	a function that filters to rules with a `severity` of 2 (error) when the `--quiet` flag is applied, otherwise
	acts as an identity function. In `eslint/flat-eslint.js`, the `filterRules` should be taken from
	the `eslintOptions` object, and passed down to the `linter.verifyAndFix` call.

[nzakas]

Suggested change

	Within `linter.js`, the API should be added to `VerifyOptions`, and will be passed down into and
	utilized within the `runRules` function before the current `configuredRules` loop. Rather than the
	current configuredRules loop, this should extract the `severity` and `rule` existence checks and
	build a list of `{ ruleId, severity }` objects. This new list should be passed to `filterRules`,
	and the resulting list should be iterated on instead of `configuredRules`.
	Within `linter.js`, the API should be added to `VerifyOptions`, and will be passed down into and
	utilized within the `runRules` function before the current `configuredRules` loop. Rather than the
	current configuredRules loop, this should use `ruleFilter` as a filter to check the `severity` and `ruleId` to
	build a list of `ruleId`s. This new list should be iterated on instead of `configuredRules`.

[nzakas]

Suggested change

	`normalizeVerifyOptions` should verify that the `filterRules` option is a function, and replace it
	`normalizeVerifyOptions` should verify that the `ruleFilter` option is a function, and replace it

[nzakas]

Suggested change

	validation check that the `filterRules` option is a function.
	validation check that the `ruleFilter` option is a function.

[nzakas]

Suggested change

	The new `filterRules` function when implemented would look like this, using the `--quiet` flag
	The new `ruleFilter` function when implemented would look like this, using the `--quiet` flag

[nzakas]

Suggested change

	filterRules: (rules: { ruleId: string; severity: number }[]) => {
	return rules.filter((rule) => rule.severity === 2);
	},
	ruleFilter: ({ ruleId: string; severity: number }) => {
	return rule.severity === 2;
	},

[nzakas]

Suggested change

	This function would return all rules which have a severity of 2 (error), effectively filtering out
	This function would return true for all rules which have a severity of 2 (error), effectively filtering out

[me4502]

Thanks, I noticed a few extra places that needed to be changed back to the old predicate function setup so I pushed it as a separate commit rather than including your suggestions via the GitHub UI

I believe it should all be back to working that way now

[mdjermanovic]

LGTM, thanks! Moving to final commenting. I left one suggestion to add to the Backwards Compatibility Analysis section so that we don't forget why this is a breaking change and what to advise users in the migration guide.

[mdjermanovic]

I'd add here the case we discussed in #104 (comment). Rules with side effects, such as the react/jsx-uses-vars rule, will not be run if set to "warn", so users will need to update their configs to "error" after we make this change.

[ljharb]

It’d be nice if there was a way for a rule to know its own severity, so that this rule could warn when it’s on warn :-)

[me4502]

Thanks, I've just added this to the document

[mdjermanovic]

Looks good, thanks!

[nzakas]

Marking as approved and merging! Thanks so much for your patience @me4502. We are ready to implement.

[me4502]

Thanks, what would be the next step on my end? As it's a breaking change does that mean the implementation should be held off until a major release is happening, or is implementation able to start now?

[nzakas]

You can start the implementation and just mark the PR as a draft so we know not to merge it until the next major release.

[ColtHands]

PR in question (just for visibility)