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

Support block-comment version of eslint-disable-line #8781

Closed

Comments

@qfox
Copy link
Member

@qfox qfox commented Jun 22, 2017

Tell us about your environment

  • ESLint Version: master
  • Node Version: any
  • npm Version: any

What parser (default, Babel-ESLint, etc.) are you using?
default

Please show your full configuration:

{ "rules": { "no-alert": [1] } }

What did you do? Please include the actual source code causing the issue.

/* eslint-disable-line */ alert(1);

What did you expect to happen?

It should work like in JSCS:

/* jscs:ignore */ alert(1)

What actually happened? Please include the actual, raw output from ESLint.

Error reported.

@kaicataldo

This comment has been minimized.

Copy link
Member

@kaicataldo kaicataldo commented Jun 22, 2017

Would it make sense to also make eslint-disable-next-line also available in block comments to fix this issue?

@qfox

This comment has been minimized.

Copy link
Member Author

@qfox qfox commented Jun 27, 2017

Yeah, absolutely.

Waiting for accept then ;-)

@qfox

This comment has been minimized.

Copy link
Member Author

@qfox qfox commented Jun 27, 2017

Oops. Am I right that this issue already has 3 ok's?

@platinumazure

This comment has been minimized.

Copy link
Member

@platinumazure platinumazure commented Jun 27, 2017

Previous discussions on this have been "resolved" with the notion that we shouldn't have two ways to do the same thing. I would add that if we support line/block comments for some of our configuration comments, then users will be confused why we don't do so for all configuration comments.

Are we sure we want/need to do this?

@ilyavolodin

This comment has been minimized.

Copy link
Member

@ilyavolodin ilyavolodin commented Jun 27, 2017

@zxqfox Need one more 👍 to get this accepted. Since you proposed it, you count as champion. We need champion + 3 +1s

@not-an-aardvark

This comment has been minimized.

Copy link
Member

@not-an-aardvark not-an-aardvark commented Jun 27, 2017

This is a core change, so it would need to be approved by the TSC.

@ilyavolodin

This comment has been minimized.

Copy link
Member

@ilyavolodin ilyavolodin commented Jun 28, 2017

Oh, right, sorry about that. For some reason I was thinking that this would be a rule change.

@jeffvandyke

This comment has been minimized.

Copy link

@jeffvandyke jeffvandyke commented Jun 28, 2017

I'd like to just throw out there that I think that both line and block comments should be allowed for all configuration comments, in regards to @platinumazure's comment. I remember being confused why I had to have specific types for certain configuration options, and in the case of JSX, requiring line comments are very limiting and make for messy code, which is why we have this issue. That's my vote.

@mysticatea

This comment has been minimized.

Copy link
Member

@mysticatea mysticatea commented Jun 28, 2017

How does it behave in this case?

/* eslint-disable-line 


*/ alert(1);
@qfox

This comment has been minimized.

Copy link
Member Author

@qfox qfox commented Jun 28, 2017

@mysticatea Looks like it should disable both lines or does not work at all and throw an error or warning (the second is better for me).
Definitely need to add few spec for this.

@platinumazure There are cases in wild that can't be achieved with line-form. Also JSCS supported block-form of line-disabling directive. Actually, that's my case.

@not-an-aardvark

This comment has been minimized.

Copy link
Member

@not-an-aardvark not-an-aardvark commented Jun 29, 2017

I'm not sure about this.

I don't think we need to support the use case described in #8779 (using a block comment rather than a line comment to make sure line numbers in the file are preserved). Automatically inserting eslint-disable-line comments into a file with a tool is not the intended use case for disable comments, and in my opinion being able to do so while preserving line numbers seems like an overly strict requirement. (Allowing block disable comments wouldn't fully solve this problem either. For example, it still wouldn't be possible to insert a comment to disable a line in the middle of a long multiline template literal.)

I do think it would be nice to find a solution for the JSX case (#7030). But eslint-disable-line directives are supposed to unambiguously refer to a single line, and requiring line comments for them is a nice way of enforcing that. I'm not sure it's a good idea to allow people to use multiline eslint-disable-line comments, since that seems like it would just cause confusion about what line is being referred to.

@qfox

This comment has been minimized.

Copy link
Member Author

@qfox qfox commented Jun 29, 2017

Guys guys, if we said we support JSCS — there is nothing to discuss, JSCS supported this and ESLint should support this too.
If no — then no. But then why we deprecated JSCS and closed? We should reopen it as a fork of ESLint or something like that.

The main point we can't migrate from JSCS because of this and there are no workaround.

@platinumazure

This comment has been minimized.

Copy link
Member

@platinumazure platinumazure commented Jun 29, 2017

@qfox

This comment has been minimized.

Copy link
Member Author

@qfox qfox commented Jun 29, 2017

Well, what should I do with my case?

I literally can't do the same with ESLint. It means there are no compatibility.

@platinumazure

This comment has been minimized.

Copy link
Member

@platinumazure platinumazure commented Jun 29, 2017

Again: I would love to see if we can solve this some other way.

This is feeling like an XY problem. I don't know what actual problem you're trying to solve, but this feels like a clunky way to do it. I'm hoping we can come up with something better.

@qfox

This comment has been minimized.

Copy link
Member Author

@qfox qfox commented Jun 29, 2017

Okay, sorry. I'll pray 🙏

@kaicataldo

This comment has been minimized.

Copy link
Member

@kaicataldo kaicataldo commented Jun 29, 2017

I don't know what actual problem you're trying to solve, but this feels like a clunky way to do it. I'm hoping we can come up with something better.

@platinumazure Mind explaining what it is that you don't like about this solution? It's clear that you're not satisfied with this solution, but I don't know what about it you're against.

My 2c:
The most compelling reason for me to support this is the JSX use case (which only allows block comments), and I would also argue that it would be even more confusing to only allow those style comments in JSX but not allow them elsewhere. So it seems to me like supporting this style comment everywhere makes the most sense.

The multiline comment is an interesting case. Seems like it should treat lines that contain the beginning or end line of the comment as "the line" in question.

I agree that a line comment would probably be clearer most of the time, but this feels like a case where we can decide on behavior, document it, and trust our users rather than limit them. And for what it's worth, this seems like a pretty reasonable style for a eslint-disable-next-line comment:

/*
  eslint-disable-next-line
 */
var foo;
@alberto

This comment has been minimized.

Copy link
Member

@alberto alberto commented Jul 4, 2017

The main point we can't migrate from JSCS because of this and there are no workaround.

Can you explain why the current directives don't work for you?

@kaicataldo

This comment has been minimized.

Copy link
Member

@kaicataldo kaicataldo commented Jul 4, 2017

Can you explain why the current directives don't work for you?

I'm also not entirely clear on this. Yes, you'd have to move the disable-line comment to the end of the line and make it a line comment, but that's an implementation detail. As far as I can tell, we have feature parity (but do correct me if I'm wrong, @zxqfox!), even though the implementation itself is slightly different.

@not-an-aardvark

This comment has been minimized.

Copy link
Member

@not-an-aardvark not-an-aardvark commented Jul 6, 2017

It looks like the discussion here is still ongoing. Should we still discuss it at the TSC meeting, or should we remove it from the agenda until we have a concrete proposal or we're unable to reach consensus?

@qfox

This comment has been minimized.

Copy link
Member Author

@qfox qfox commented Jul 6, 2017

Can you explain why the current directives don't work for you?

I'm also not entirely clear on this.

I have a couple of files that should be prepared before prepublishing and I can't use // eslint-ignore-line because expression can be on the to several lines like:

myfunc(`a
b
c
d`);
//
(something && myfunc(`a
b
c
d`));

Also we can't use // eslint-ignore-next-line because it adds new line each time.

Again, it worked well with jscs and even jshint.

This was referenced Mar 22, 2018
@eslint eslint bot locked and limited conversation to collaborators Aug 10, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.