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

Allow explanation of eslint-disable #11806

Open
bfred-it opened this issue Jun 4, 2019 · 3 comments

Comments

Projects
None yet
2 participants
@bfred-it
Copy link

commented Jun 4, 2019

The version of ESLint you are using.
eslint@5.16.0

The problem you want to solve.
I want to disable a feature but I need to explain why.

This can be done on a separate line when you use block-level eslint-disable but it becomes awkward when using one-line disablers like eslint-disable-next-line.

Your take on the correct solution to problem.

// eslint-disable-next-line your-rule because "explanation"
// eslint-disable-next-line your-rule, "explanation"
// eslint-disable-next-line your-rule // "explanation"
// eslint-disable-next-line your-rule - "explanation"

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

Way out of my comfort zone

@kaicataldo

This comment has been minimized.

Copy link
Member

commented Jun 4, 2019

Thanks for the proposal.

Do you mind expanding on why a separate explanation line comment either on the offending line or above the line that contains the disable comment won’t suffice in your case?

I personally am not not convinced we should make this change, as allowing for arbitrary characters in disable comments adds a lot of complexity and surface area for bugs for what seems like very little gain. Adding the ability to do this feels like a mixing of responsibilities of comments that are part of ESLint’s API and comments that are intended for other humans as well as creating a more complex/muddied API.

Let’s see what others have to say.

@kaicataldo kaicataldo added evaluating and removed triage labels Jun 4, 2019

@kaicataldo

This comment has been minimized.

Copy link
Member

commented Jun 4, 2019

Additionally, while you’re more than welcome to discuss and get feedback here first, our policy is that any change to core requires creating an RFC. Thanks!

@bfred-it

This comment has been minimized.

Copy link
Author

commented Jun 4, 2019

I'll use a real-life example.

Ideal (same line)

The comment is exactly where it needs to be: explaining a line, on the line itself. A comment on the comment

// eslint-disable-next-line no-extraneous-class // Because it follows `@types/chrome`'s style
export class RequestContentScript {
    constructor(contentScript: ContentScript);
}

Good (next line, not allowed with next-line)

The comment follows the thought "This is disabled \n This is why"
This would actually be more readable in some cases, but it requires changing what eslint-disable-next-line considers "the next line"

// eslint-disable-next-line no-extraneous-class
// Because it follows `@types/chrome`'s style
export class RequestContentScript {
    constructor(contentScript: ContentScript);
}

Currently

"This is why, I do this" seems backwards. "This" what? The code or eslint?

// This follows `@types/chrome`'s style
// eslint-disable-next-line no-extraneous-class
export class RequestContentScript {
    constructor(contentScript: ContentScript);
}

Making it more clear requires making it wordy

// The rule can be disabled because the class follows `@types/chrome`'s style
// eslint-disable-next-line no-extraneous-class
export class RequestContentScript {
    constructor(contentScript: ContentScript);
}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.