Update: support eslint-disable-* block comments (fixes #8781) by erindepew · Pull Request #9745 · eslint/eslint

erindepew · 2017-12-20T21:14:10Z

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!

kaicataldo

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.

mysticatea · 2017-12-22T10:08:59Z

                        break;

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


Thank you for contributing!

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

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?

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.

@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?

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.

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

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

Thanks for the clarification @not-an-aardvark

erindepew · 2017-12-29T01:53:49Z

@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 · 2018-01-20T00:22:59Z

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

kaicataldo · 2018-01-24T05:50:32Z

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 · 2018-01-31T04:19:10Z

@kaicataldo @mysticatea updated!

kaicataldo

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!

platinumazure

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

platinumazure · 2018-02-02T18:54:33Z

                const config = {
                    rules: {
-                        "no-alert": 1
+                        "no-alert": 1,


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

platinumazure · 2018-02-02T18:55:00Z

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

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


Could this description be made more specific?

platinumazure · 2018-02-02T18:56:43Z

+                assert.strictEqual(messages.length, 2);
+
                assert.strictEqual(messages[0].ruleId, "no-alert");
+                assert.strictEqual(messages[1].ruleId, "quotes");


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.

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

platinumazure · 2018-02-02T18:57:31Z

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


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?

platinumazure · 2018-02-02T18:58:09Z

+                    rules: {
+                        "no-alert": 1,
+                        quotes: [1, "single"],
+                        "no-console": 1


Do we need no-console here?

platinumazure · 2018-02-02T18:59:02Z

                assert.strictEqual(messages[0].ruleId, "no-console");
            });

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


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

platinumazure · 2018-02-02T18:59:22Z

+                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", () => {


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

platinumazure · 2018-02-02T18:59:43Z

            });

-            it("should not ignore violations if comment is in block quotes", () => {
+            it("should ignore violations if comment is in block quotes", () => {


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

erindepew · 2018-02-07T04:03:50Z

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

platinumazure

LGTM, thanks!

not-an-aardvark · 2018-02-10T03:10:05Z

Thanks for contributing!

ilyavolodin approved these changes Dec 21, 2017

View reviewed changes

aladdin-add added core Relates to ESLint's core APIs and features enhancement This change enhances an existing feature of ESLint evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion labels Dec 21, 2017

kaicataldo suggested changes Dec 21, 2017

View reviewed changes

mysticatea reviewed Dec 22, 2017

View reviewed changes

ilyavolodin mentioned this pull request Dec 23, 2017

Update: Allow eslint-disable-line to be wrapper in BlockComment #8780

Closed

platinumazure mentioned this pull request Jan 20, 2018

Commit message check does not enforce "fixes"/"refs" in parentheses eslint/eslint-github-bot#56

Closed

erindepew force-pushed the 8781-block-comment-disable branch from 248359d to b802625 Compare January 31, 2018 04:18

kaicataldo approved these changes Feb 1, 2018

View reviewed changes

kaicataldo added accepted There is consensus among the team that this change meets the criteria for inclusion and removed evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion labels Feb 1, 2018

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 force-pushed the 8781-block-comment-disable branch from b802625 to 3d7a574 Compare February 2, 2018 13:24

platinumazure suggested changes Feb 2, 2018

View reviewed changes

erindepew and others added 2 commits February 6, 2018 22:44

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

dc22098

updating tests

7e14e53

erindepew force-pushed the 8781-block-comment-disable branch from 3d7a574 to 7e14e53 Compare February 7, 2018 03:49

more cleaning up tests

d5f435e

platinumazure approved these changes Feb 7, 2018

View reviewed changes

not-an-aardvark merged commit 4a6f22e into eslint:master Feb 10, 2018

renovate Bot mentioned this pull request Feb 16, 2018

Update dependency eslint to v4 Urigo/tortilla#62

Merged

This was referenced Mar 8, 2018

chore(deps): update dependency eslint to v4.19.0 mono-js/mono-mongodb#5

Closed

chore(deps): update dependency eslint to v4.19.0 mono-js/mono-redis#5

Closed

renovate Bot mentioned this pull request Mar 23, 2018

Update eslint to 4.19.1 sbardian/DebtTrackerAPI#33

Merged

not-an-aardvark mentioned this pull request May 10, 2018

Report an error for eslint-disable-line comments that span multiple lines #10334

Closed

eslint-deprecated Bot locked and limited conversation to collaborators Aug 10, 2018

eslint-deprecated Bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Aug 10, 2018

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

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

Uh oh!

Conversation

erindepew commented Dec 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kaicataldo left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kaicataldo Jan 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

erindepew commented Dec 29, 2017

Uh oh!

platinumazure commented Jan 20, 2018

Uh oh!

kaicataldo commented Jan 24, 2018

Uh oh!

erindepew commented Jan 31, 2018

Uh oh!

kaicataldo left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

platinumazure left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

erindepew commented Feb 7, 2018

Uh oh!

platinumazure left a comment

Choose a reason for hiding this comment

Uh oh!

not-an-aardvark commented Feb 10, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

erindepew commented Dec 20, 2017 •

edited

Loading

kaicataldo left a comment •

edited

Loading

kaicataldo Jan 4, 2018 •

edited

Loading

kaicataldo left a comment •

edited

Loading