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
Breaking: stricter rule config validating (fixes #9505) #11742
Conversation
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.
Hi. Thank you for your contribution!
I have some suggestions.
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.
Great. The direction looks good to me.
I have some suggestions.
(And I guess the "commit-message" check fails due to the length of the PR title.)
Thank you!
tests/lib/cli-engine.js
Outdated
@@ -1370,8 +1370,8 @@ describe("CLIEngine", () => { | |||
assert.strictEqual(report.results.length, 1); | |||
assert.strictEqual(report.results[0].messages.length, 1); | |||
assert.strictEqual(report.results[0].messages[0].ruleId, "missing-rule"); | |||
assert.strictEqual(report.results[0].messages[0].severity, 1); | |||
assert.strictEqual(report.results[0].messages[0].message, "Definition for rule 'missing-rule' was not found"); | |||
assert.strictEqual(report.results[0].messages[0].severity, 2); |
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'm OK for this change, but we have to add it to the migration guide because it can newly fail CI builds.
6ef06fa
to
98bda3c
Compare
I think it's ready to be reviewed! 😄 ptal. @mysticatea |
to fix issue eslint#9505, it made a refactor and introduced a few changes to make validating more consistent: * will report a linting error when enable/disable a non-existent in inline comment. * will throw an error, if config a non-existent rule to non-zero value. * fixes problem loc in some cases
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.
Looks very good to me. Thank you!
and please note: this PR will break |
@aladdin-add Could you also add information into migration guide please? |
@aladdin-add Since this is the last breaking issue left in this release, I'm going to merge it as is. Could you open a new PR with updated migration guide please? We will merge it with the next release. |
it is no longer needed, as #11742 get merged. :)
Well, we need to update our migration guide. |
todo:
What is the purpose of this pull request? (put an "X" next to item)
[x] Add something to the core
What changes did you make? (Give an overview)
fixes #9505
Is there anything you'd like reviewers to focus on?
/*eslint-enable foo*/
/*eslint-disable(-line) foo*/
/*eslint foo: 0*/
{rules: {foo: 0}}
{rules: {foo: 1}
getRule()
will not return a stub rule if the rule cannot be loaded.Previously, the
ruleMapper.getRule()
will return a stub rule, when the rule is not defined. it was really counter-intuitive and confusing.After this change, it will return
null
in this case. As a result it will report a linting error, and has no effect to the final config: