-
-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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
New: ignorePatterns in config files (refs eslint/rfcs#22) #12274
Conversation
Hmm, the test results are different from my local... 🤔 |
9a60f5b
to
cf5befc
Compare
cf5befc
to
869f96a
Compare
This PR is ready for review. |
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.
Just two small changes needed from my perspective.
Not sure if another person should also review. This is a pretty large commit.
Co-Authored-By: Kevin Partington <[email protected]>
Co-Authored-By: Kevin Partington <[email protected]>
# Conflicts: # lib/cli-engine/ignored-paths.js # tests/lib/cli-engine/ignored-paths.js
I have resolved conflicts. |
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.
LGTM, thanks!
# Conflicts: # lib/cli-engine/ignored-paths.js
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.
Still LGTM!
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.
Just one small nit (left a suggestion), otherwise looks great. Thanks!
Co-Authored-By: Kevin Partington <[email protected]>
Thanks! Sorry for the delay in getting this merged. |
Thank you for reviewing this big PR! |
* @param {string[]} sourcePaths The paths to calculate the common ancestor. | ||
* @returns {string} The path to the common ancestor directory. | ||
*/ | ||
function getCommonAncestorPath(sourcePaths) { |
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.
@mysticatea - getCommonAncestorPath introduces this issue - #12850
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)
This PR implements
ignorePatterns
top-level property into config files. This feature has been approved in RFC 22. People can useignorePatterns
property instead of.eslintignore
file. Also, people can provide theignorePatterns
setting with shareable configs. It will reduce the cost of setup of new repositories.Detailed Design
I removed
IgnoredPaths
class that had been handling.eslintignore
,--ignore-path
,--ignore-pattern
, and--no-ignore
. Instead, nowConfigArray
handles ignore patterns.Like above, config array contains the ignore pattern data additionally if existed. It will merge ignore patterns of all config array element in the order as-is then create a
node-ignore
instance. This means,ignorePatterns
properties will be merged in the same order as the other settings, and we can use the unignoring with!
.Classes:
IgnorePattern
class (new!)It has
basePath
andpatterns
as similar toOverrideTester
class that handlesoverrides[i].files
. TheIgnorePattern.createIgnore()
static method creates the predicate function from multipleIgnorePattern
instances.ConfigArray
classNow the array elements have
ignorePattern
property that is anIgnorePattern
instance or undefined. TheConfigArray#extractConfig()
method merges theIgnorePattern
instances byIgnorePattern.createIgnore()
static method.ConfigArrayFactory
classNow it recognizes
ignorePatterns
top-level property in config files then createsignorePattern
properties into config array elements.Also, now it has new two methods
factory.loadESLintIgnore(filePath)
andfactory.loadDefaultESLintIgnore()
to load.eslintignore
and something like.CascadingConfigArrayFactory
classNow it recognizes
--ignore-pattern
,--ignore-path
, and.eslintignore
then creates the corresponding config array elements by aConfigArrayFactory
instance.FileEnumerator
classNow it has the updated ignoring logic.
CLIEngine
classNow it has the updated ignoring logic in
executeOnFiles()
,executeOnText()
, andisPathIgnored()
.Tests:
ignored-paths.js
tocli-engine.js
(a630d9e). Now it tests public APIs instead of internal classes. (I'd like to test the public APIs primarily. Many tests that depend on internal structures make tough to refactor code.)cli-engine.js
to testisPathIgnored()
andexecuteOnFiles()
with the new feature. (executeOnText()
usesisPathIgnored()
internally)Is there anything you'd like reviewers to focus on?