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

Update: support eslint-disable-* block comments (fixes #8781) #9745

Merged
merged 3 commits into from Feb 10, 2018

Conversation

@erindepew
Copy link
Contributor

@erindepew erindepew commented Dec 20, 2017

What is the purpose of this pull request? (put an "X" next to item)

[ ] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ X] Add something to the core
[ ] Other, please explain:

What changes did you make? (Give an overview)
Updated block comments to support eslint-disable-line and eslint-disable-next-line

Is there anything you'd like reviewers to focus on?
Nope!

Copy link
Member

@kaicataldo kaicataldo left a comment

Thanks for working on this! This all LGTM, but I'd love to see at least a few tests where multiple disable directives apply to the same line, just so we're sure what the expected behavior is around this. We should probably have tests both for mixed line and block eslint-disable-line and eslint-disable-next-line comments as well as multiple block comments.

lib/linter.js Outdated
@@ -329,6 +328,14 @@ function modifyConfigsFromComments(filename, ast, config, ruleMapper) {
[].push.apply(disableDirectives, createDisableDirectives("disable", comment.loc.start, value));
break;

case "eslint-disable-line":
[].push.apply(disableDirectives, createDisableDirectives("disable-line", comment.loc.start, value));
Copy link
Member

@mysticatea mysticatea Dec 22, 2017

Thank you for contributing!

If the block comment is multiline, I hope to ignore it and make a warning.

Copy link
Member

@kaicataldo kaicataldo Dec 24, 2017

Oh, right! I forgot about this. Do we want to only warn in debug mode or always? My only concern is is that if we start warning by default then we might generate more warnings for users that currently have eslint-disable block comments (that currently don't do anything).

I believe in the TSC meeting we discussed having multiline eslint-disable comments not do anything currently but also not warn for this case until our next major release. I think it would be fine to warn if we're in debug mode, though. Thoughts @eslint/eslint-team?

Copy link
Member

@kaicataldo kaicataldo Dec 24, 2017

Actually, looks like we did decide on having multiline comments not apply but also not warning at the meeting. See the notes from the meeting here.

Copy link
Contributor Author

@erindepew erindepew Dec 29, 2017

@kaicataldo so is this fine as is and we can create another PR with the warning about using multi-line comments for disabling a single line?

Copy link
Member

@kaicataldo kaicataldo Jan 4, 2018

I think the current behavior is incorrect because it applies multiline eslint-disable-* directives, which is confusing (which line should it be ignoring? The starting line, the ending line, or both?).

So I believe @mysticatea is saying that those directives should simply be ignored and not applied. We can't actually warn on them because that would be a breaking change. If anyone already mistakenly has multiline block eslint-disable-* directives (even though they don't do anything at the moment), this would create more warnings and potentially fail the linting job.

So I believe the correct behavior here is to not apply mutiline eslint-disable-* directives in this PR, but not warn on them. We can add a warning in another PR when we're getting ready for our next major release.

@mysticatea Please correct me if I'm wrong.

Copy link
Contributor Author

@erindepew erindepew Jan 31, 2018

@mysticatea @kaicataldo and would that also apply for disable-next-line as well? That it should only be a single line comment?

Copy link
Member

@not-an-aardvark not-an-aardvark Jan 31, 2018

Yes, it would apply to both eslint-disable-line and eslint-disable-next-line.

Copy link
Contributor Author

@erindepew erindepew Jan 31, 2018

Thanks for the clarification @not-an-aardvark

@erindepew
Copy link
Contributor Author

@erindepew erindepew commented Dec 29, 2017

@kaicataldo updated with tests to cover the combined rule use-case, let me know if you want me to change anything about how we're handling block comments for single lines!

@platinumazure
Copy link
Member

@platinumazure platinumazure commented Jan 20, 2018

@kaicataldo Is this ready for merge, or is there more that needs to be done here?

@kaicataldo
Copy link
Member

@kaicataldo kaicataldo commented Jan 24, 2018

I believe @mysticatea's comment and the subsequent discussion here still needs to be accounted for, but then we should be ready to go!

@erindepew erindepew force-pushed the 8781-block-comment-disable branch from 248359d to b802625 Jan 31, 2018
@erindepew
Copy link
Contributor Author

@erindepew erindepew commented Jan 31, 2018

Copy link
Member

@kaicataldo kaicataldo left a comment

If you wouldn't mind updating the PR title to follow our format, that would be great. As a suggestion: Update: support eslint-disable-* block comments (fixes #8781).

We ask that all PRs follow this format because it allows us to automate our changelogs and release notes.

Other than the PR title, this looks great to me. Thanks for contributing!

@erindepew erindepew changed the title Update: support block comment ersion of eslint-disable-line fixes #8781 Update: support eslint-disable-* block comments (fixes #8781) Feb 2, 2018
@erindepew erindepew force-pushed the 8781-block-comment-disable branch from b802625 to 3d7a574 Feb 2, 2018
Copy link
Member

@platinumazure platinumazure left a comment

Hi @erindepew, thanks for the PR! I've left some minor change requests, please ping me if you have questions. Thanks!

].join("\n");
const config = {
rules: {
"no-alert": 1
"no-alert": 1,
Copy link
Member

@platinumazure platinumazure Feb 2, 2018

Is there any reason to leave the no-alert rule in here?

@@ -1933,20 +1933,61 @@ describe("Linter", () => {

it("should report a violation", () => {
Copy link
Member

@platinumazure platinumazure Feb 2, 2018

Could this description be made more specific?

assert.strictEqual(messages[0].ruleId, "no-alert");
assert.strictEqual(messages[1].ruleId, "quotes");
Copy link
Member

@platinumazure platinumazure Feb 2, 2018

It's not clear which line in the fixture (quotes in alert or quotes in console.log) this message points to. Could we either add a line check, or change one of the examples so this is unambiguous? (My understanding is the quotes in alert should be warned, but the quotes in console.log should not.

Copy link
Contributor Author

@erindepew erindepew Feb 7, 2018

Sure, I just removed the console.log since it wasn't really necessary for this example anyway

it("should not report a violation", () => {
const code = [
"alert('test'); // eslint-disable-line no-alert",
"alert('test'); /*eslint-disable-line no-alert*/" // here
Copy link
Member

@platinumazure platinumazure Feb 2, 2018

I think the // here comment is used in other tests to indicate what line should have a warning reported even despite the inline config comment. So maybe it should be removed here?

rules: {
"no-alert": 1,
quotes: [1, "single"],
"no-console": 1
Copy link
Member

@platinumazure platinumazure Feb 2, 2018

Do we need no-console here?

@@ -2005,6 +2081,56 @@ describe("Linter", () => {
assert.strictEqual(messages[0].ruleId, "no-console");
});

it("should ignore violation of specified rule if comment is in block quotes", () => {
Copy link
Member

@platinumazure platinumazure Feb 2, 2018

Should this say something like, "if comment is a block comment"?

assert.strictEqual(messages.length, 1);
assert.strictEqual(messages[0].ruleId, "no-console");
});
it("should ignore violation of specified rule if comment is in block quotes", () => {
Copy link
Member

@platinumazure platinumazure Feb 2, 2018

Should this say something like, "if comment is a block comment"?

@@ -2103,7 +2248,7 @@ describe("Linter", () => {
assert.strictEqual(messages[0].ruleId, "no-console");
});

it("should not ignore violations if comment is in block quotes", () => {
it("should ignore violations if comment is in block quotes", () => {
Copy link
Member

@platinumazure platinumazure Feb 2, 2018

Should this say something like, "if comment is a block comment"?

@erindepew erindepew force-pushed the 8781-block-comment-disable branch from 3d7a574 to 7e14e53 Feb 7, 2018
@erindepew
Copy link
Contributor Author

@erindepew erindepew commented Feb 7, 2018

Thanks @platinumazure for the review! I updated the tests with your comments

Copy link
Member

@platinumazure platinumazure left a comment

LGTM, thanks!

@not-an-aardvark not-an-aardvark merged commit 4a6f22e into eslint:master Feb 10, 2018
5 checks passed
@not-an-aardvark
Copy link
Member

@not-an-aardvark not-an-aardvark commented Feb 10, 2018

Thanks for contributing!

@eslint-deprecated eslint-deprecated 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.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants