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

Allowing inline comments to disable eslint rules. #3471

Merged

Conversation

wbyoung
Copy link
Contributor

@wbyoung wbyoung commented Aug 20, 2015

I'd like to be able to use inline comments to disable eslint rules, but to disallow any other type of inline comment.

This change allows that configuration.

@BYK
Copy link
Member

BYK commented Aug 20, 2015

Validating Commit Message

So first you need to create an issue, explain the problem you have clearly, and then reference that issue from your commit :)

@wbyoung wbyoung force-pushed the no-inline-except-eslint-disable-line branch from c3f5e42 to a403b80 Compare August 20, 2015 22:37
@wbyoung
Copy link
Contributor Author

wbyoung commented Aug 20, 2015

@BYK issue created & referenced.

@@ -28,8 +28,12 @@ module.exports = function(context) {
// Also check after the comment
var postamble = endLine.slice(node.loc.end.column).trim();

// Check that this comment doesn't disable the line
var disablesLine = startLine.indexOf("eslint-disable-line") >= 0 ||
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this has to be a bit more specific, otherwise the following would also be ignored:

// someday use eslint-disable-line here

What you really want is:

var isDirective = node.value.trim().indexOf("eslint-") === 0;

@nzakas
Copy link
Member

nzakas commented Aug 26, 2015

@wbyoung are you still working on this?

@wbyoung
Copy link
Contributor Author

wbyoung commented Aug 26, 2015

@nzakas I can make the changes you suggested. I wasn't sure that this solution was acceptable since there was no consensus reached on #3472. I'll update shortly.

@nzakas
Copy link
Member

nzakas commented Aug 28, 2015

I think this is a good way to go for now.

@wbyoung wbyoung force-pushed the no-inline-except-eslint-disable-line branch from a403b80 to 5ec4e88 Compare August 28, 2015 20:52
@wbyoung
Copy link
Contributor Author

wbyoung commented Aug 28, 2015

@nzakas updated with your suggestion. Sorry for the delay.

@gyandeeps
Copy link
Member

@wbyoung Can you update the commit message to have "Update" and not "New".

@wbyoung wbyoung force-pushed the no-inline-except-eslint-disable-line branch from 5ec4e88 to 6179a9d Compare August 30, 2015 17:29
@wbyoung wbyoung force-pushed the no-inline-except-eslint-disable-line branch from 6179a9d to c1d91ea Compare August 30, 2015 19:01
@wbyoung
Copy link
Contributor Author

wbyoung commented Aug 30, 2015

@gyandeeps done.

ilyavolodin added a commit that referenced this pull request Aug 30, 2015
…-line

Allowing inline comments to disable eslint rules.
@ilyavolodin ilyavolodin merged commit deacbcc into eslint:master Aug 30, 2015
@IanVS
Copy link
Member

IanVS commented Sep 1, 2015

Should the docs be updated to mention this? If so I can include in my current cleanup work.

@gyandeeps
Copy link
Member

Good catch @IanVS . I think its good to add

@wbyoung wbyoung deleted the no-inline-except-eslint-disable-line branch October 1, 2015 19:52
@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Feb 7, 2018
@eslint-deprecated eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Feb 7, 2018
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants