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

disable-enable-pair option allowWholeFile should be the default #59

Open
Zamiell opened this issue Jun 14, 2021 · 8 comments
Open

disable-enable-pair option allowWholeFile should be the default #59

Zamiell opened this issue Jun 14, 2021 · 8 comments

Comments

@Zamiell
Copy link

Zamiell commented Jun 14, 2021

I propose that the allowWholeFile should be set to true by default if it is not specified by the end-user.

Setting up this plugin for the first time was frustrating, because I got a lot of false positives. I then assumed that this plugin was bugged, and eventually found out that this option existed.

I think that it is pretty standard in the JavaScript/TypeScript ecosystem to disable a while file like this, so I don't see any compelling reasons for the current behavior of allowWholeFile being set to false by default.

@bschlenk
Copy link

bschlenk commented Jun 18, 2021

I came here to propose this feature, but glad to see it already exists! I agree it would be nice if this were the default...

@bmish
Copy link

bmish commented Jun 18, 2021

As a counterpoint to changing the default, if you want to disable a rule for an entire file, it's actually better practice to use this comment style instead:

/* eslint no-console:"off" */

// rest of file

This actually disables the rule for the entire file, whereas using /* eslint-disable no-console */ only disables the rule starting after the comment, which means the rule can technically still flag a violation, see this ticket for why: eslint/eslint#12245

@bschlenk
Copy link

Interesting! I didn't realize there was a "correct" way to do this.

In that case, what would be really cool would be if the disable-enable-pair rule came with a suggestion to covert it to the /* eslint <rule-name>:"off" */ format if it detected the eslint-disable comment at the top of the file without a corresponding eslint-enable.

@Zamiell
Copy link
Author

Zamiell commented Jun 18, 2021

Screw suggesting - I would go further and say that the linter should either
a) not complain about it at all, or
b) automatically change it to the "off" style with the --fix flag, so that the end-user doesn't have to do anything

(In this context, I am assuming that the end-user has their IDE configured to do eslint --fix on save, which seems to be fairly common with the advent of e.g. eslint-plugin-prettier.)

@bschlenk
Copy link

The issue with autofixing would be not knowing the intent of the developer. This plugin doesn't know if you forgot a corresponding eslint-enable comment, or if you really want the rule disabled for the whole file. A suggestion not only lets you quickly fix it, but also makes you aware of what the proper syntax is.

I guess that's why the allowWholeFile setting exists because you can set it up to "not complain about it at all". But given the above discussion, I'm going to start using the "correct" syntax and probably keep the allowWholeFile setting false.

@Zamiell
Copy link
Author

Zamiell commented Jun 18, 2021

This plugin doesn't know if you forgot a corresponding eslint-enable comment, or if you really want the rule disabled for the whole file.

It's somewhat safe (but not completely safe) to assume that if the eslint-disable statement is on the very first line of the file, then the developer intended for it to apply to the whole file. Seems like a reasonable middle ground though.

@mansona
Copy link

mansona commented Jun 23, 2022

I think we might be past the point of making the /* eslint <rule-name>:"off" */ syntax required by default, every single team I have worked with and every single open source project I have worked with are using the /* eslint-disable <rule-name> */ format for file-based ignores.

Sure we can try to turn that ship but I strongly feel like that will be a failing battle. I understand that there may be some edge cases for rules that could report before the start of the comment but that must be a very very small portion of rules or teams that are affected by that 🤔

@MichaelDeBoey
Copy link

Hi @Zamiell!

Since this repo is unmaintained, you might want to re-open this issue in the @eslint-community fork https://github.com/eslint-community/eslint-plugin-eslint-comments

For more info about why we created this organization, you can read https://eslint.org/blog/2023/03/announcing-eslint-community-org

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants