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: fix config loading (fixes #11510, fixes #11559, fixes #11586) #11546

Merged
merged 53 commits into from May 10, 2019

Conversation

Projects
3 participants
@mysticatea
Copy link
Member

commented Mar 24, 2019

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

[X] Bug fix: fixes #11510, fixes #11559, fixes #11586
[X] Other, please explain: the refactoring of configurations

Closes #10928 because I realized that this PR does the same thing as #10928 (comment).

What changes did you make? (Give an overview)

This PR fixes three bugs along with the refactoring of configurations.
I'm sorry that this diff is huge.

All new files are in lib/cli-engine/ directory because only CLIEngine uses those.

The refactoring does the following items:

I describe file mapping of before/after the refactoring. There are details for each new file on the top of the source code.

Old New
lib/config-ops.js
(merge method)
lib/cli-engine/config-array/config-array.js
(n/a) lib/cli-engine/config-array/config-dependency.js
(n/a) lib/cli-engine/config-array/extracted-config.js
lib/config-ops.js
(pathMatchesGlobs method)
lib/cli-engine/config-array/override-tester.js
lib/config.js lib/cli-engine/cascading-config-array-factory.js
lib/config/config-file.js
lib/util/file-finder.js
lib/cli-engine/config-array-factory.js
lib/util/glob-utils.js
lib/util/glob.js
lib/cli-engine/file-enumerator.js

Therefore, appropriate tests to the moved logics are moved to new files.

And I added some @typedef comments for VSCode IntelliSense.

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

  • Are there unintentional breaking changes?

I found an additional breaking change (as a bug fix) about plugin loading. Currently, even if a configuration has no plugins field, CLIEngine#executeOnFiles() and CLIEngine#executeOnText() can use plugin rules that the previous calls loaded. After this PR, CLIEngine#executeOnFiles() and CLIEngine#executeOnText() will report error messages consistently if plugins were not found in configuration.

Related to that, Linter#getRules() and CLIEngine#getRules() will change:

  • Linter#getRules() will return the rules, including built-in rules, user-defined rules, and only the plugin rules that the last Linter#verify() loaded.
  • CLIEngine#getRules() will return the rules, including built-in rules and only the plugin rules that the last CLIEngine#executeOnFiles() or CLIEngine#executeOnText() loaded.

I think that this change is good because formatters can access to unrelated rules (via RFC 10) before this PR and we already have a getter to get information of the last Linter#verify() call; Linter#getSourceCode().

@mysticatea mysticatea changed the title Breaking: fix `overrides` settings in shareable configs overwrite extender's setting (fixes #11510) Breaking: fix `overrides` setting to not affect extendee files (fixes #11510) Mar 24, 2019

@mysticatea mysticatea changed the title Breaking: fix `overrides` setting to not affect extendee files (fixes #11510) Breaking: fix overrides setting to not affect extendee (fixes #11510) Mar 24, 2019

@mysticatea mysticatea added this to Implemented, pending review in v6.0.0 Mar 26, 2019

@mysticatea mysticatea force-pushed the issue11510 branch from 447f701 to d0a3a19 Mar 27, 2019

@mysticatea mysticatea changed the title Breaking: fix overrides setting to not affect extendee (fixes #11510) Breaking: fix bugs in config loading (fixes #11510, fixes #11559) Mar 29, 2019

@mysticatea mysticatea force-pushed the issue11510 branch from 904eff8 to 5ebeed3 Apr 4, 2019

@mysticatea mysticatea force-pushed the issue11510 branch 2 times, most recently from a864ae1 to 89b3292 Apr 4, 2019

@mysticatea mysticatea changed the title Breaking: fix bugs in config loading (fixes #11510, fixes #11559) Breaking: fix config loading (fixes #11510, fixes #11559, fixes #11586) Apr 4, 2019

@platinumazure

This comment has been minimized.

Copy link
Member

commented Apr 4, 2019

@mysticatea PR #11388 has been merged. It also looks like this needs a rebase/merge. Once that's done, feel free to request my review or ping me and I'll review this. Thanks!

@mysticatea mysticatea force-pushed the issue11510 branch 3 times, most recently from 88e1649 to 2fc4464 Apr 5, 2019

@mysticatea mysticatea removed the do not merge label Apr 5, 2019

@mysticatea

This comment has been minimized.

Copy link
Member Author

commented Apr 5, 2019

Hi. I updated this PR and I think ready for review!

@mysticatea mysticatea moved this from Implemented, pending review to Ready to merge in v6.0.0 Apr 8, 2019

@mysticatea mysticatea moved this from Ready to merge to Implemented, pending review in v6.0.0 Apr 8, 2019

@mysticatea

This comment has been minimized.

Copy link
Member Author

commented Apr 9, 2019

I updated this PR to resolve conflicts.

@mysticatea

This comment has been minimized.

Copy link
Member Author

commented Apr 9, 2019

I will revert moved files because I have found that Git didn't recognize cli-engine.js as moved.

mysticatea and others added some commits May 9, 2019

Update lib/config/config-initializer.js
Co-Authored-By: Teddy Katz <teddy.katz@gmail.com>
@mysticatea

This comment has been minimized.

Copy link
Member Author

commented May 9, 2019

Thank you very much for making time to review this large PR!

I think that I fixed all the suggested changes.

@not-an-aardvark

This comment has been minimized.

Copy link
Member

commented May 9, 2019

This looks good to me after #11546 (comment) is addressed.

Also, I think this PR should remove these lines from the migration guide.

mysticatea added some commits May 9, 2019

@not-an-aardvark
Copy link
Member

left a comment

LGTM, thanks!

@mysticatea

This comment has been minimized.

Copy link
Member Author

commented May 9, 2019

Thank you very much for the review of this huge PR!

@mysticatea mysticatea merged commit 6ae21a4 into master May 10, 2019

3 checks passed

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

v6.0.0 automation moved this from Implemented, pending review to Done May 10, 2019

@mysticatea mysticatea deleted the issue11510 branch May 10, 2019

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