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

Using --max-warnings in combination with --quiet should error #14202

Closed
gewoonwoutje opened this issue Mar 12, 2021 · 6 comments · Fixed by #14242
Closed

Using --max-warnings in combination with --quiet should error #14202

gewoonwoutje opened this issue Mar 12, 2021 · 6 comments · Fixed by #14242
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion bug ESLint is working incorrectly core Relates to ESLint's core APIs and features
Projects

Comments

@gewoonwoutje
Copy link

The version of ESLint you are using.
v7.21.0

The problem you want to solve.
Using the --max-warnings cli option is useless in combination with --quiet.
I'm running this in

Note the following, first running without --quiet, then with.
image

Your take on the correct solution to problem.

Ideally: The --quiet flag should be overriden when the amount of warnings exceeds max-warnings, logging the warnings.
Also acceptable: The exit code should be 1 when max-warnings is exceeded.

Are you willing to submit a pull request to implement this change?
Not really 😇

@gewoonwoutje gewoonwoutje added core Relates to ESLint's core APIs and features enhancement This change enhances an existing feature of ESLint triage An ESLint team member will look at this issue soon labels Mar 12, 2021
@nzakas
Copy link
Member

nzakas commented Mar 13, 2021

It looks like there is probably a bug when quiet and max-warnings are used together. In that case, it should still exit with a 1 exit code saying that there were too many warnings.

I think it makes sense to not show warnings in this case.

@nzakas nzakas removed the triage An ESLint team member will look at this issue soon label Mar 13, 2021
@nzakas nzakas added this to Needs Triage in Triage via automation Mar 13, 2021
@nzakas nzakas moved this from Needs Triage to Ready for Dev Team in Triage Mar 13, 2021
@nzakas nzakas moved this from Ready for Dev Team to Evaluating in Triage Mar 13, 2021
@nzakas nzakas added accepted There is consensus among the team that this change meets the criteria for inclusion bug ESLint is working incorrectly and removed enhancement This change enhances an existing feature of ESLint labels Mar 13, 2021
@mdjermanovic
Copy link
Member

It looks like there is probably a bug when quiet and max-warnings are used together. In that case, it should still exit with a 1 exit code saying that there were too many warnings.

I agree that --max-warnings indicates that the user wants a non-zero exit code when there are too many warnings, even with --quiet. Otherwise, this combination wouldn't make sense, and if a non-zero exit code isn't desired behavior then they can just remove --max-warnings from the command.

I think it makes sense to not show warnings in this case.

I don't have a strong opinion on this. Not showing warnings looks less surprising, but the list of warnings might be valuable in this case.

@btmills
Copy link
Member

btmills commented Mar 18, 2021

I agree that this is a bug and that --max-warnings should trigger a non-zero exit code when the limit is exceeded even if used with --quiet.

I think it makes sense to not show warnings in this case.

I also agree that this is the correct behavior because we should respect --quiet and not show warnings even if --max-warnings is exceeded.

@nzakas
Copy link
Member

nzakas commented Mar 18, 2021

Okay, so it sounds like we've agreed that the current behavior is a bug.

Current behavior: When using --quiet with --max-warnings, we are not showing an error even when the number of warnings exceeds the threshold for errors.

Desired behavior: When using --quiet with --max-warnings, we should show the error that the maximum number of warnings has been reached and not show the actual warnings.

@nzakas nzakas moved this from Evaluating to Ready to Implement in Triage Mar 18, 2021
@mdjermanovic
Copy link
Member

Should we show the number of found warnings in the message?

Current message is: ESLint found too many warnings (maximum: 100).

eslint/lib/cli.js

Lines 315 to 318 in ebd7026

log.error(
"ESLint found too many warnings (maximum: %s).",
options.maxWarnings
);

Without --quiet, formatter is expected to show the number of warnings.

With --quiet, formatter will show either nothing or, if it's a formatter that always show a summary, something like warnings: 0.

@nzakas
Copy link
Member

nzakas commented Mar 19, 2021

I don’t think that’s necessary. The max allowable warnings is already shown so I don’t think there’s any need to show the actual number

@mdjermanovic mdjermanovic moved this from Ready to Implement to Pull Request Opened in Triage Mar 23, 2021
Triage automation moved this from Pull Request Opened to Complete Mar 25, 2021
@eslint-github-bot eslint-github-bot bot locked and limited conversation to collaborators Sep 22, 2021
@eslint-github-bot eslint-github-bot bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Sep 22, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion bug ESLint is working incorrectly core Relates to ESLint's core APIs and features
Projects
No open projects
Triage
Complete
Development

Successfully merging a pull request may close this issue.

4 participants