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

Fix: refactor eslint-disable comment processing (fixes #9215) #9216

Merged
merged 1 commit into from Sep 6, 2017

Conversation

not-an-aardvark
Copy link
Member

@not-an-aardvark not-an-aardvark commented Sep 3, 2017

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

[x] Bug fix (#9215)

What changes did you make? (Give an overview)

This extracts the processing logic for eslint-disable comment processing into a different module file, rather than storing state on Linter throughout the comment traversal. This fixes bugs such as #9215, and adds a bunch of tests for edge cases to avoid issues like #9195 in the future.

This also improves the asymptotic complexity of disable comment checking logic -- it was previously O(#problems * #disable comments), and now it's O(#problems + #disable comments * log(#disable comments)). In most cases this probably won't be noticeable because most files don't have very many disable comments.

edit: This also fixes #6592. I had forgotten that issue existed, but it seems like my resulting design is pretty similar to what was proposed there.

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

Nothing in particular

@not-an-aardvark not-an-aardvark added accepted There is consensus among the team that this change meets the criteria for inclusion bug ESLint is working incorrectly chore This change is not user-facing core Relates to ESLint's core APIs and features labels Sep 3, 2017
@eslintbot
Copy link

LGTM

@mention-bot
Copy link

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

@@ -1664,7 +1664,7 @@ describe("Linter", () => {
assert.equal(messages[1].column, 19);
});

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

@gyandeeps gyandeeps Sep 3, 2017

Choose a reason for hiding this comment

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

These unit test (all of them) changes makes me little uncomfortable.... Ideally refactor should not change unit test...
I am questioning to better understand the intent...

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree that in general, refactors shouldn't change unit tests.

However, I think these tests were asserting incorrect behavior. The first test was asserting that no error should be reported in cases like this:

/* eslint no-alert: error */
/* eslint-disable */
/* eslint-disable */

/* eslint-enable */

alert('foo');

I don't think this makes sense because eslint-enable is supposed to enable all disabled rules, regardless of how the rules were disabled previously.

The second test was asserting that no error should be reported in cases like this:

/* eslint no-alert: error */

/* eslint-disable no-alert */

/* eslint-disable no-console */

/* eslint-enable */

alert('foo');

I don't think this makes sense because /* eslint-enable */ is supposed to enable all disabled rules, regardless of how many rules have been disabled before. (Previously, the user would need to use a second /* eslint-enable */ comment to enable no-alert again.)

@gyandeeps
Copy link
Member

I like the overall intent of this change... we should try to breakdown cli-engine also..
thanks @not-an-aardvark for dng this n other changes...

@eslintbot
Copy link

LGTM

@eslintbot
Copy link

LGTM

@eslintbot
Copy link

LGTM

@not-an-aardvark not-an-aardvark merged commit a32ec36 into master Sep 6, 2017
@not-an-aardvark not-an-aardvark deleted the disable-comment-refactor branch September 6, 2017 20:22
@platinumazure
Copy link
Member

Thanks for knocking this out, @not-an-aardvark!

aladdin-add pushed a commit to aladdin-add/eslint that referenced this pull request Sep 8, 2017
@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Mar 6, 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 Mar 6, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion bug ESLint is working incorrectly chore This change is not user-facing core Relates to ESLint's core APIs and features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactor inline config comment processing out of lib/eslint.js
6 participants