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
Conversation
LGTM |
@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", () => { |
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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.)
I like the overall intent of this change... we should try to breakdown cli-engine also.. |
4164ca4
to
fdd9e39
Compare
LGTM |
fdd9e39
to
4a98a29
Compare
LGTM |
4a98a29
to
ad97d22
Compare
LGTM |
Thanks for knocking this out, @not-an-aardvark! |
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 onLinter
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'sO(#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