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: Enable reportUnusedDisableDirectives
config option by default
#15466
Comments
This is an interesting idea. I think there’s a larger question of whether or not we should make the functionality on by default and then not allow it to be turned off? As in, should we remove the command line flag completely? Worth noting from the docs:
|
@nzakas my proposal is currently only referring to changing the default of the However, I am open to being more aggressive here. Personally, the behavior I would prefer is to always error on unused directives (regardless of where ESLint is run from).
So if we can consider this more aggressive behavior, let me know, and I'll re-focus the proposal around this. Otherwise, I'll keep the proposal limited to the more conservative change. In addition, removing the one-off CLI/config options for reporting disable directives would be a nice simplification too. Personally, I'd likely be fine removing the options, but I'd be curious if others have a strong use case for disabling/silencing unused disable directive violations. |
This comment has been minimized.
This comment has been minimized.
@ljharb thats pretty tangential to the topic at hand. Feel free to open a separate issue if you’d like to discuss that. @bmish Lets see what other team members think. I’m happy to consider both subtle and aggressive changes. What I don’t want to do is a bunch of incremental changes if we ultimately want to rethink this functionality as a whole. |
This comment has been minimized.
This comment has been minimized.
A use case where reporting unused disable directives is undesirable is when different ESLint configurations are used at different stages of a workflow. In that case, a directive can be unused if the rule wasn't enabled in one config, but may become used with another config at a later stage. We already don't support this use case well, but enforcing the removal of unused disable directives without an opt-out could add more problems for users with such workflows. |
Fair enough. What do you think of the proposal as @bmish originally proposed?
I’m not sure I understand how we can change this in one spot but not the other. Can you explain more? |
I support changing the default value of the |
API option Lines 596 to 605 in 648fe1a
The CLI flag Line 120 in 648fe1a
All of this is a bit confusing, but in the end, for CLI users it normalizes to one of these three behaviors:
The current default behavior is 1. After this change, the default behavior will be 2. This change wouldn't affect:
|
@mdjermanovic thanks for the context, that's helpful. It sounds like there is support for my original proposal (i.e. warning on unused directives). But I haven't yet heard opinions on the more aggressive version which is erroring by default on unused directives. Is erroring still something we should consider now? Is anyone explicitly against erroring? Should we defer this question and just start with warning in ESLint v9 but consider erroring in a subsequent major version? Although as @nzakas said, it would be nice to figure out the endgame now. |
Since I can't make it error via my shared config, and I don't want to update 300+ projects to pass the command line flag, I'd love the default to be changed. |
@ljharb that's a great argument for erroring by default, which I support. I think the main question is just whether we're okay with the potential semver implications of erroring by default. I'm bringing this up because it sounded like in the past we were not okay with them, so I'm wondering if we're open to reconsidering this:
|
Just to make sure I’m understanding correctly, what would the CLI And that means you can’t opt-out of warning on disable directives from the CLI, only in config? (If true, not a fan.) @bmish regarding erroring, can you explain how that would interplay with the command line flag? |
@nzakas your questions have got me thinking that the current boolean design of these options is particularly confusing and unintuitive, making it hard to predict the interplay. What if I described a new "explicit severity" design:
Benefits of this design:
On top of this design, we can still debate what the default value should be:
|
That sounds like a good direction to head in. |
Oops! It looks like we lost track of this issue. What do we want to do here? This issue will auto-close in 7 days without an update. |
Assigning to @bmish to continue work |
I’d love to be able to set
I can try creating a PR that supports |
This will require an RFC. I can work on putting together an RFC based on what I proposed in my previous comment. |
I have opened an RFC for this: |
2023-11-30 tsc-meeting note: waiting on @bmish to finish PR (draft). |
Part 1 is done but I'm going to reopen this until part 2 of the RFC is done:
|
@bmish would you like to submit a PR for part 2 as well? This feature is planned for the first v9 alpha release, which will be next week. |
Yes I started part 2 in: |
2022-12 UPDATE
I have opened an RFC for this:
ESLint version
8.5.0
What problem do you want to solve?
Outdated/unused ESLint disable directive comments (e.g.
notConsole(); // eslint-disable-line no-console
) are currently a blind spot for most apps/repositories. These leftover comments cause clutter and confusion, and detecting them requires going out of one's way by using one of the following existing solutions:eslint --report-unused-disable-directives
(errors, but only applies during CLI)reportUnusedDisableDirectives: true
to .eslintrc.js config file (warns)These existing solutions are fine but they lack discoverability and require manual work to adopt in every single repository so are thus not commonly used.
What do you think is the correct solution?
As a breaking change, I propose changing the default value for the config option
reportUnusedDisableDirectives
from false to true. This will cause users to see additional warnings if they have unused disable directives.Displaying these warnings should help raise awareness about the problem of unused disable directives, and encourage many users to remove unused disable directives caused by their changes.
Note that since these are just warnings, users could also choose to ignore them and let them pile up, as a potential downside. But luckily, the new functionality from running
eslint --fix --report-unused-disable-directives
could be used to fix/remove all of them at any time.Another note is that we need to limit any default behavior to warnings and not errors for this due to our semver policy. More context in:
Participation
Additional comments
No response
The text was updated successfully, but these errors were encountered: