Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat!: skip running warnings in --quiet mode #17274

Merged
merged 14 commits into from Dec 27, 2023

Conversation

me4502
Copy link
Contributor

@me4502 me4502 commented Jun 12, 2023

Prerequisites checklist

What is the purpose of this pull request? (put an "X" next to an item)

[ ] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofix to a rule
[ ] Add a CLI option
[X] Add something to the core
[ ] Other, please explain:

https://github.com/eslint/rfcs/tree/main/designs/2023-only-run-reporting-rules

Fixes #16450

What changes did you make? (Give an overview)

This PR implements the RFC around only running rules that actually report information in quiet mode. This adds a predicate function that is passed down to the runRules function that filters out all non-error rules when the --quiet flag is entered and maxWarnings is not in use. All rules returning false from the predicate function are skipped during rule running.

TODO:

  • Tests
  • Documentation changes
  • Ignoring of unused disable directives

I believe the --max-warnings flag handling is already covered by the existing tests, as from a test perspective it only cares about if it still works with quiet mode (everything else is an implementation detail). I've added tests that the ruleFilter option does actually filter out rules to the linter.

Is there anything you'd like reviewers to focus on?

@eslint-github-bot eslint-github-bot bot added breaking This change is backwards-incompatible feature This change adds a new feature to ESLint labels Jun 12, 2023
@github-actions
Copy link

Hi everyone, it looks like we lost track of this pull request. Please review and see what the next steps are. This pull request will auto-close in 7 days without an update.

@github-actions github-actions bot added the Stale label Jun 22, 2023
@Rec0iL99 Rec0iL99 removed the Stale label Jun 22, 2023
@Rec0iL99
Copy link
Member

Not stale. Work in progress.

@github-actions
Copy link

github-actions bot commented Jul 3, 2023

Hi everyone, it looks like we lost track of this pull request. Please review and see what the next steps are. This pull request will auto-close in 7 days without an update.

@github-actions github-actions bot added the Stale label Jul 3, 2023
@me4502
Copy link
Contributor Author

me4502 commented Jul 3, 2023

Sorry am still working on it; have been a bit busy and given this is blocked by the next major release it’s not too high on my list of priorities as I’m assuming that’s still at least a little while away - I’ll try to finish it up sometime soonish then rebase it every so often to keep the bot happy

@Rec0iL99 Rec0iL99 removed the Stale label Jul 4, 2023
@github-actions
Copy link

Hi everyone, it looks like we lost track of this pull request. Please review and see what the next steps are. This pull request will auto-close in 7 days without an update.

@github-actions github-actions bot added the Stale label Jul 14, 2023
@me4502
Copy link
Contributor Author

me4502 commented Jul 15, 2023

Not stale sorry, I’ve been fairly busy

@Rec0iL99 Rec0iL99 removed the Stale label Jul 15, 2023
@github-actions
Copy link

Hi everyone, it looks like we lost track of this pull request. Please review and see what the next steps are. This pull request will auto-close in 7 days without an update.

@github-actions github-actions bot added the Stale label Jul 25, 2023
@me4502 me4502 force-pushed the feature/quiet-mode-skip-warnings branch from ca15aa6 to d7f3918 Compare July 26, 2023 00:51
@netlify
Copy link

netlify bot commented Jul 26, 2023

Deploy Preview for docs-eslint canceled.

Name Link
🔨 Latest commit 504cfa5
🔍 Latest deploy log https://app.netlify.com/sites/docs-eslint/deploys/658aa57c87fbb40008815f6c

@github-actions
Copy link

github-actions bot commented Aug 6, 2023

Hi everyone, it looks like we lost track of this pull request. Please review and see what the next steps are. This pull request will auto-close in 7 days without an update.

@github-actions github-actions bot added the Stale label Aug 6, 2023
@me4502 me4502 force-pushed the feature/quiet-mode-skip-warnings branch from 57ee39c to 451edeb Compare August 6, 2023 23:54
@fasttime fasttime removed the Stale label Aug 7, 2023
@me4502 me4502 force-pushed the feature/quiet-mode-skip-warnings branch 2 times, most recently from 44f46c5 to bd74353 Compare August 21, 2023 02:19
@me4502 me4502 force-pushed the feature/quiet-mode-skip-warnings branch from bd74353 to acff363 Compare August 28, 2023 02:46
@me4502 me4502 marked this pull request as ready for review August 28, 2023 02:52
@me4502 me4502 requested a review from a team as a code owner August 28, 2023 02:52
@me4502
Copy link
Contributor Author

me4502 commented Aug 28, 2023

I believe this should be ready for review. I looked over it and wrote some tests to validate that the ruleFilter option on the linter functions correctly, and the interactions with the --max-warnings flag appear to be fully covered by existing tests

@me4502 me4502 force-pushed the feature/quiet-mode-skip-warnings branch from 3ac45d7 to 90136a6 Compare December 22, 2023 00:28
@me4502 me4502 force-pushed the feature/quiet-mode-skip-warnings branch from 90136a6 to c93df94 Compare December 22, 2023 00:29
@me4502
Copy link
Contributor Author

me4502 commented Dec 22, 2023

As it seems some breaking changes for v9 are now in the main branch, should I mark this ready for review?

@mdjermanovic mdjermanovic marked this pull request as ready for review December 22, 2023 13:54
@mdjermanovic
Copy link
Member

As it seems some breaking changes for v9 are now in the main branch, should I mark this ready for review?

Yes, I marked this as ready for review now.

lib/linter/linter.js Outdated Show resolved Hide resolved
@mdjermanovic
Copy link
Member

One thing we forgot to mention in the RFC is caching.
If user runs eslint --cache --quiet, only errors will be stored in the cache because rules set to warn were not run. If the user then runs just eslint --cache without --quiet, results pulled from the cache can be wrong for this command because they don't include warnings that the code might have.
I don't have a concrete solution at the moment, just wanted to note that we likely need to address this.

I think this behavior is acceptable. If you're running ESLint with different command line flags, it seems like different results should be expected? But I'm guessing most people don't change the flags they're using very often.

Okay, I think it's acceptable but it would be better to avoid it if possible. Something like @me4502 suggested in #17274 (comment) might be the solution. However, given the complexity, I think this should be revisited at some later point (not in this PR). Also, it seems that the same problem exists with some other CLI options (e.g., --no-inline-config) so we could try to fix that altogether.

Copy link
Member

@mdjermanovic mdjermanovic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks! Leaving open for @nzakas to verify.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we rename to eslint.config-rule-throws.js? We generally don't have underscores in filenames.

Copy link
Member

@nzakas nzakas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because I'm blocked, I'm going to go ahead and merge this and then handle the file renaming myself.

Thanks so much for sticking with this feature @me4502! 🙏

@nzakas nzakas merged commit d1018fc into eslint:main Dec 27, 2023
17 checks passed
@me4502
Copy link
Contributor Author

me4502 commented Dec 28, 2023

Thanks :) Sorry I named the file with underscores as the ones around it had those too
image

@me4502 me4502 deleted the feature/quiet-mode-skip-warnings branch December 28, 2023 06:12
@nzakas
Copy link
Member

nzakas commented Dec 28, 2023

Yeah no worries, I think those snuck in at some point so I understand the confusion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion breaking This change is backwards-incompatible contributor pool feature This change adds a new feature to ESLint
Projects
Status: Complete
Development

Successfully merging this pull request may close these issues.

Change Request: stop running rules set to "warn" when --quiet is used
5 participants