Fix: Handle nested disable directive correctly (fixes #9318) #9322

gyandeeps · 2017-09-17T16:03:37Z

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

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

What changes did you make? (Give an overview)

Fixed a bug where nested eslint disable directive created some issues. Please refer to the linked issue for examples.

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

Not very clean (I kinda think - not sure why) but it looks to solve the problem.
Column calculation threw a 🔧 . 😄

mention-bot · 2017-09-17T16:03:40Z

@gyandeeps, thanks for your PR! By analyzing the history of the files in this pull request, we identified @not-an-aardvark, @aladdin-add and @i-ron-y to be potential reviewers.

not-an-aardvark

Thanks for working on this.

To me, it seems like this solution is more complex than necessary. I'll take a look at it to see if I can make it simpler.

not-an-aardvark · 2017-09-17T17:45:35Z

lib/util/apply-disable-directives.js


            switch (directive.type) {
+                case "disable-line":
+                    tempDisabledByLine.add(directive.ruleId === null ? "all" : directive.ruleId);


This will fail if someone creates a rule with the id "all".

not-an-aardvark · 2017-09-17T17:48:20Z

lib/util/apply-disable-directives.js

        ) {
            problems.push(problem);
        }
+
+        lineDisabledRules.set(problem.line, {
+            column: directive.column || 0,


Why does the column matter here? eslint-disable-line comments should apply to the entire line, regardless of where they're placed on the line.

not-an-aardvark · 2017-09-17T17:54:35Z

lib/util/apply-disable-directives.js


    for (const problem of options.problems) {
+        let tempDisabledByLine = new Set();


I don't understand the name of this comment -- why is it temp?

not-an-aardvark · 2017-09-17T17:57:00Z

lib/util/apply-disable-directives.js


    for (const problem of options.problems) {
+        let tempDisabledByLine = new Set();
+        let directive = {};
+        const lineDisabledRule = lineDisabledRules.get(problem.line);


Why are enable-line directives needed if we are filtering by line here anyway?

Because sometimes there would be no code on a disabled line and then next line would have code with violations. That would only for some unique scenarios like this one

eslint/tests/lib/util/apply-disable-directives.js

Lines 352 to 360 in 22bf943

it("keeps problems after the next line", () => {

assert.deepEqual(

applyDisableDirectives({

directives: [{ type: "disable-next-line", line: 1, column: 1, ruleId: null }],

problems: [{ line: 3, column: 3, ruleId: "foo" }]

}),

[{ line: 3, column: 3, ruleId: "foo" }]

);

});

eslintbot · 2017-09-17T18:56:51Z

LGTM

not-an-aardvark · 2017-09-17T20:21:05Z

@gyandeeps I made a few modifications to your implementation in #9323, which makes the implementation a bit simpler. Feel free to merge that if you're okay with those changes (the base branch of #9323 is your issue9318 branch, which is on this PR).

eslintbot · 2017-09-17T20:33:32Z

LGTM

not-an-aardvark

Hmm, I might have made a mistake -- I'm still getting an error when linting the code from #9318 (comment) on this branch. Requesting changes while I figure out what's going on.

not-an-aardvark · 2017-09-17T21:15:11Z

@gyandeeps I made another PR onto this branch: #9324

eslintbot · 2017-09-17T23:24:10Z

LGTM

gyandeeps · 2017-09-18T01:22:45Z

tests/lib/util/apply-disable-directives.js

@@ -343,19 +388,6 @@ describe("apply-disable-directives", () => {
                []
            );
        });
-
-        it("keeps problems on the next line if there is an eslint-enable comment before the problem on the next line", () => {


Sorry for the confusion... I dont think we should remove this test...
This behavior in this test does make sense....

Really? I think the behavior is sort of strange -- this is the only place where the "block" directives and the "line" directives interact with each other. I think it makes more sense for them to be separate, and people can just use /* eslint-disable */ rather than // eslint-disable-line if they only want to disable part of a line.

We should probably decide about the correct behavior here. I think the version that's currently on this PR is the most reasonable choice (and at very least it's better than the version on master). If there is going to be a patch release tomorrow, then I think we should either decide this quickly or merge the PR as-is and decide it later.

gyandeeps added bug ESLint is working incorrectly core Relates to ESLint's core APIs and features patch candidate This issue may necessitate a patch release in the next few days labels Sep 17, 2017

Fix: Handle nested disable directive correctly (fixes #9318)

6648bb0

gyandeeps force-pushed the issue9318 branch from c145215 to 6648bb0 Compare September 17, 2017 16:35

eslint deleted a comment from eslintbot Sep 17, 2017

not-an-aardvark self-requested a review September 17, 2017 16:44

add unit test for enabled scenarios

8093c02

eslint deleted a comment from eslintbot Sep 17, 2017

not-an-aardvark suggested changes Sep 17, 2017

View reviewed changes

add more test and identifier for all rules

22bf943

gyandeeps force-pushed the issue9318 branch from 001ac4f to 22bf943 Compare September 17, 2017 18:56

eslint deleted a comment from eslintbot Sep 17, 2017

not-an-aardvark mentioned this pull request Sep 17, 2017

Chore: Simply line-comment handling in fix for #9318 #9323

Merged

Chore: Simply line-comment handling in fix for #9318 (#9323)

bdf4c7a

gyandeeps closed this Sep 17, 2017

gyandeeps reopened this Sep 17, 2017

not-an-aardvark approved these changes Sep 17, 2017

View reviewed changes

not-an-aardvark changed the title ~~Fix: Handle nested disable directive correctly (fixes 9318)~~ Fix: Handle nested disable directive correctly (fixes #9318) Sep 17, 2017

not-an-aardvark suggested changes Sep 17, 2017

View reviewed changes

not-an-aardvark mentioned this pull request Sep 17, 2017

Chore: handle disable-line comments in a separate pass #9324

Merged

Chore: handle disable-line comments in a separate pass (#9324)

d7e227e

not-an-aardvark approved these changes Sep 17, 2017

View reviewed changes

not-an-aardvark added accepted There is consensus among the team that this change meets the criteria for inclusion regression Something broke labels Sep 17, 2017

gyandeeps commented Sep 18, 2017

View reviewed changes

ilyavolodin approved these changes Sep 18, 2017

View reviewed changes

btmills approved these changes Sep 18, 2017

View reviewed changes

btmills merged commit 08656db into master Sep 18, 2017

gyandeeps deleted the issue9318 branch September 18, 2017 21:00

eslint-deprecated bot locked and limited conversation to collaborators Mar 18, 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 Mar 18, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: Handle nested disable directive correctly (fixes #9318) #9322

Fix: Handle nested disable directive correctly (fixes #9318) #9322

gyandeeps commented Sep 17, 2017 •

edited

mention-bot commented Sep 17, 2017

not-an-aardvark left a comment

not-an-aardvark Sep 17, 2017

not-an-aardvark Sep 17, 2017

not-an-aardvark Sep 17, 2017

not-an-aardvark Sep 17, 2017

gyandeeps Sep 17, 2017 •

edited

eslintbot commented Sep 17, 2017

not-an-aardvark commented Sep 17, 2017 •

edited

eslintbot commented Sep 17, 2017

not-an-aardvark left a comment

not-an-aardvark commented Sep 17, 2017

eslintbot commented Sep 17, 2017

gyandeeps Sep 18, 2017

not-an-aardvark Sep 18, 2017

not-an-aardvark Sep 18, 2017


		for (const problem of options.problems) {
		let tempDisabledByLine = new Set();

	it("keeps problems after the next line", () => {
	assert.deepEqual(
	applyDisableDirectives({
	directives: [{ type: "disable-next-line", line: 1, column: 1, ruleId: null }],
	problems: [{ line: 3, column: 3, ruleId: "foo" }]
	}),
	[{ line: 3, column: 3, ruleId: "foo" }]
	);
	});

Fix: Handle nested disable directive correctly (fixes #9318) #9322

Fix: Handle nested disable directive correctly (fixes #9318) #9322

Conversation

gyandeeps commented Sep 17, 2017 • edited

mention-bot commented Sep 17, 2017

not-an-aardvark left a comment

Choose a reason for hiding this comment

not-an-aardvark Sep 17, 2017

Choose a reason for hiding this comment

not-an-aardvark Sep 17, 2017

Choose a reason for hiding this comment

not-an-aardvark Sep 17, 2017

Choose a reason for hiding this comment

not-an-aardvark Sep 17, 2017

Choose a reason for hiding this comment

gyandeeps Sep 17, 2017 • edited

Choose a reason for hiding this comment

eslintbot commented Sep 17, 2017

not-an-aardvark commented Sep 17, 2017 • edited

eslintbot commented Sep 17, 2017

not-an-aardvark left a comment

Choose a reason for hiding this comment

not-an-aardvark commented Sep 17, 2017

eslintbot commented Sep 17, 2017

gyandeeps Sep 18, 2017

Choose a reason for hiding this comment

not-an-aardvark Sep 18, 2017

Choose a reason for hiding this comment

not-an-aardvark Sep 18, 2017

Choose a reason for hiding this comment

gyandeeps commented Sep 17, 2017 •

edited

gyandeeps Sep 17, 2017 •

edited

not-an-aardvark commented Sep 17, 2017 •

edited