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

Breaking: stricter rule config validating (fixes #9505) #11742

Merged
merged 3 commits into from May 25, 2019

Conversation

@aladdin-add
Copy link
Member

commented May 19, 2019

todo:

  • migration guide need to be updated.

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?

config before after
/*eslint-enable foo*/ no error linting error
/*eslint-disable(-line) foo*/ no error linting error
/*eslint foo: 0*/ no error linting error
{rules: {foo: 0}} no error no error
{rules: {foo: 1} linting warning linting error
  • please note 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:
image

Copy link
Member

left a comment

Hi. Thank you for your contribution!

I have some suggestions.

lib/config/config-validator.js Outdated Show resolved Hide resolved
lib/linter.js Outdated Show resolved Hide resolved
lib/config/config-validator.js Outdated Show resolved Hide resolved
lib/linter.js Outdated Show resolved Hide resolved
tests/lib/config/config-validator.js Outdated Show resolved Hide resolved
@mysticatea mysticatea added this to Implemented, pending review in v6.0.0 May 20, 2019
@aladdin-add aladdin-add force-pushed the aladdin-add:issue9505 branch from 7d930cb to f525bb8 May 21, 2019
Copy link
Member

left a comment

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!

lib/linter.js Outdated Show resolved Hide resolved
lib/linter.js Outdated Show resolved Hide resolved
lib/linter.js Outdated Show resolved Hide resolved
@@ -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);

This comment has been minimized.

Copy link
@mysticatea

mysticatea May 22, 2019

Member

I'm OK for this change, but we have to add it to the migration guide because it can newly fail CI builds.

@aladdin-add aladdin-add force-pushed the aladdin-add:issue9505 branch 3 times, most recently from 6ef06fa to 98bda3c May 22, 2019
@aladdin-add aladdin-add changed the title Breaking: throw an error if disables a non-existent rule in a configuration comment (fixes#9505) wip: Breaking: throw an error if disables a non-existent rule in a configuration comment (fixes#9505) May 22, 2019
@mysticatea mysticatea moved this from Implemented, pending review to Accepted, ready to implement in v6.0.0 May 24, 2019
@aladdin-add aladdin-add force-pushed the aladdin-add:issue9505 branch from 98bda3c to 0113795 May 24, 2019
@aladdin-add aladdin-add changed the title wip: Breaking: throw an error if disables a non-existent rule in a configuration comment (fixes#9505) Breaking: more stricter rule config validating (fixes#9505) May 24, 2019
@aladdin-add aladdin-add changed the title Breaking: more stricter rule config validating (fixes#9505) Breaking: stricter rule config validating (fixes#9505) May 24, 2019
@aladdin-add aladdin-add force-pushed the aladdin-add:issue9505 branch from 0113795 to dda77f5 May 24, 2019
@aladdin-add aladdin-add changed the title Breaking: stricter rule config validating (fixes#9505) Breaking: stricter rule config validating (fixes #9505) May 24, 2019
@aladdin-add aladdin-add force-pushed the aladdin-add:issue9505 branch from dda77f5 to 941b6e3 May 24, 2019
@aladdin-add aladdin-add requested a review from mysticatea May 24, 2019
@aladdin-add

This comment has been minimized.

Copy link
Member Author

commented May 24, 2019

I think it's ready to be reviewed! 😄 ptal. @mysticatea

@aladdin-add aladdin-add force-pushed the aladdin-add:issue9505 branch from 941b6e3 to 97c2808 May 24, 2019
to fix issue #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
@aladdin-add aladdin-add force-pushed the aladdin-add:issue9505 branch from 97c2808 to b8c24ef May 24, 2019
Copy link
Member

left a comment

Looks very good to me. Thank you!

@aladdin-add

This comment has been minimized.

Copy link
Member Author

commented May 24, 2019

and please note: this PR will break npm run perf, and should be fixed by #11756

@mysticatea mysticatea moved this from Accepted, ready to implement to Implemented, pending review in v6.0.0 May 24, 2019
@ilyavolodin

This comment has been minimized.

Copy link
Member

commented May 25, 2019

@aladdin-add Could you also add information into migration guide please?

@ilyavolodin

This comment has been minimized.

Copy link
Member

commented May 25, 2019

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

@ilyavolodin ilyavolodin merged commit 2d32a9e into eslint:master May 25, 2019
5 checks passed
5 checks passed
commit-message PR title follows commit message guidelines
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
licence/cla Contributor License Agreement is signed.
Details
release-monitor No patch release is pending
Details
v6.0.0 automation moved this from Implemented, pending review to Done May 25, 2019
aladdin-add added a commit that referenced this pull request May 26, 2019
it is no longer needed, as #11742 get merged. :)
@aladdin-add aladdin-add deleted the aladdin-add:issue9505 branch May 31, 2019
@mysticatea

This comment has been minimized.

Copy link
Member

commented Jun 24, 2019

Well, we need to update our migration guide.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
v6.0.0
  
Done
3 participants
You can’t perform that action at this time.