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

Change Request: error by default for unused disable directives #18665

Closed
1 task done
JoshuaKGoldberg opened this issue Jul 7, 2024 · 2 comments
Closed
1 task done
Labels
core Relates to ESLint's core APIs and features enhancement This change enhances an existing feature of ESLint

Comments

@JoshuaKGoldberg
Copy link
Contributor

JoshuaKGoldberg commented Jul 7, 2024

ESLint version

9.6.0

What problem do you want to solve?

As of #17879, the default value of linterOptions.reportUnusedDisableDirectives is "warn". That means that unused disable directives do get reported by default - but unless the user sets a --max-warnings, those reports don't cause a build failure / failing status code.

What do you think is the correct solution?

I'd like ESLint to "error" by default when there's an unused disable directive. It's very rare that a user would want those directives in the first place.

Following #16696 -> #16882, it seems like an odd discrepancy that ESLint would use warnings by default in its options.

Note that this would be a breaking change for a future major version of ESLint.

Participation

  • I am willing to submit a pull request for this change.

Additional comments

The paper trail I could find leading up to the default value being "warn", in chronological order:

  1. Change Request: Enable reportUnusedDisableDirectives config option by default #15466
  2. feat!: flexible config + default reporting of unused disable directives rfcs#100
  3. feat: Support custom severity when reporting unused disable directives #17212
  4. feat: warn by default for unused disable directives #17879

There are deep discussions in the RFC about whether to use "error" or "warn by default -in particular, eslint/rfcs#100 (comment) - but it looks to me that they're partially overlapped with the discussions of what to make as a breaking change. I'm not sure how much of that is open to discussion - especially now that the option has been in the wild and enabled by default.

@mdjermanovic
Copy link
Member

There are deep discussions in the RFC about whether to use "error" or "warn by default -in particular, eslint/rfcs#100 (comment)

I still agree with the conclusions from the linked thread. Unused eslint disable directives don't seem like something that should break CI by default, and users can always opt-in by setting the value to "error". Also, setting the default to "error" would mean that a semver-patch release of ESLint can by default break CI in case the release includes a fix for a false positive that was suppressed by a disable directive. Per our semver policies, patch releases are intended to not break lint builds.

@nzakas
Copy link
Member

nzakas commented Jul 8, 2024

I also agree with the result of the original discussion. I do not believe unused disable directives, by default, should be considered build-breaking errors for most users.

@nzakas nzakas closed this as completed Jul 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Relates to ESLint's core APIs and features enhancement This change enhances an existing feature of ESLint
Projects
Archived in project
Development

No branches or pull requests

3 participants