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: allowing eslint-disable-next-line directives to have extra lines only for descriptions #15567

Closed
1 task
thernstig opened this issue Feb 2, 2022 · 7 comments
Labels
archived due to age This issue has been archived; please open a new issue for any further discussion core Relates to ESLint's core APIs and features enhancement This change enhances an existing feature of ESLint
Projects

Comments

@thernstig
Copy link

thernstig commented Feb 2, 2022

ESLint version

v8.8.0

What problem do you want to solve?

It is possible to allow eslint-disable-next-line directives to span multiple lines like this:

/* eslint-disable-next-line no-console --
 * Here's a very long description about why this configuration is necessary
 * along with some additional information
**/

But some of us prefer reserving // for plain comments using multiline-comment-style with the option "separate-lines".

We would like to support this:

// eslint-disable-next-line global-require -- Some text here as well
// description on why this exception is seen as appropriate but past a 
// comfortable reading line length

What do you think is the correct solution?

For ESLint to support it.

Participation

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

Additional comments

Original PR for the currently allowed multi-line eslint-disable-next-line directives seen here #15436

@thernstig thernstig 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 Feb 2, 2022
@eslint-github-bot eslint-github-bot bot added this to Needs Triage in Triage Feb 2, 2022
@mdjermanovic mdjermanovic moved this from Needs Triage to Triaging in Triage Feb 3, 2022
@mdjermanovic mdjermanovic removed the triage An ESLint team member will look at this issue soon label Feb 3, 2022
@mdjermanovic
Copy link
Member

Hi @thernstig, thanks for the issue!

// eslint-disable-next-line global-require -- Some text here as well
// description on why this exception is seen as appropriate but past a 
// comfortable reading line length

A problem with this is that it becomes unclear what's a part of the directive, and where the directive applies.

Consider the following example:

/* eslint spaced-comment: "error" */

// eslint-disable-next-line spaced-comment -- some description
//this comment isn't spaced

Directive on line 3 disables error on line 4, and the code is lint-free as in this demo.

If we change the logic and interpret consecutive comments as parts of the same directive, the error on line 4 would not be disabled and the directive would become unused since it would apply to line 5.

But some of us prefer reserving // for plain comments using multiline-comment-style with the option "separate-lines".

This rule ignores all eslint directive comments, so it allows use of multiline /* eslint-disable-line */ block comments. Here's a demo example.

@mdjermanovic mdjermanovic moved this from Triaging to Feedback Needed in Triage Feb 3, 2022
@thernstig
Copy link
Author

/* eslint spaced-comment: "error" */

// eslint-disable-next-line spaced-comment -- some description
//this comment isn't spaced

Does the same problem not occur with /* */ with multiple lines, when using that directive?

@mdjermanovic
Copy link
Member

The current behavior seems consistent and intuitive to me: 1 JS comment = 1 directive that applies to the line after the comment.

/* eslint-disable-next-line -- 
comment ends on this line */
// and applies to this line

// eslint-disable-next-line -- comment ends on this line
// and applies to this line

Consecutive line comments are separate comments, so I think it would become confusing if we treat them like parts of the directive that applies after the last comment.

@thernstig
Copy link
Author

You are probably right. I just read too much into multiline-comment-style which indicates if "separate-lines" is used, one should consider consecutive "line" comments as the same comment. I am ok to close this then.

It kind of makes multiline-comment-style with "separate-lines" impossible to use then if one wants eslint-disable-next-line to allow to wrap multiple lines.

I wonder if there should be a doc update somewhere to let users be aware of this.

@mdjermanovic
Copy link
Member

It kind of makes multiline-comment-style with "separate-lines" impossible to use then if one wants eslint-disable-next-line to allow to wrap multiple lines.

It should be possible because multiline-comment-style rule ignores eslint directive comments.

From the documentation:

The rule always ignores directive comments such as /* eslint-disable */. Additionally, unless the mode is "starred-block", the rule ignores JSDoc comments.

Here's an example where it doesn't report error on a multiline eslint-disable-next-line comment:

/* eslint no-undef: ["error"] */
/* eslint multiline-comment-style: ["error", "separate-lines"] */

/* eslint-disable-next-line no-undef
   --  some description */
foo();

Online Demo

@thernstig
Copy link
Author

@mdjermanovic very true, should we close this then? I assume from all this there is no good way to support the original request?

@mdjermanovic
Copy link
Member

I agree with closing as it seems there is no good way to support the requested feature.

Triage automation moved this from Feedback Needed to Complete Feb 4, 2022
@eslint-github-bot eslint-github-bot bot locked and limited conversation to collaborators Aug 4, 2022
@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 Aug 4, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
archived due to age This issue has been archived; please open a new issue for any further discussion core Relates to ESLint's core APIs and features enhancement This change enhances an existing feature of ESLint
Projects
Archived in project
Triage
Complete
Development

No branches or pull requests

2 participants