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

Update: Implement FlatConfigArray (refs #13481) #14321

Merged
merged 35 commits into from Jun 26, 2021
Merged

Conversation

@nzakas
Copy link
Member

@nzakas nzakas commented Apr 13, 2021

Prerequisites checklist

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

[ ] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[x] Add something to the core
[ ] Other, please explain:

Implements FlatConfigArray (refs #13481)

What changes did you make? (Give an overview)

I added FlatConfigArray class and associated tests to make sure everything is working okay. FlatConfigArray is not referenced anywhere while running ESLint currently, as implementing FlatConfigArray is meant to be a standalone task. In the next task, I'll work on using it when eslint.config.js is found.

Merging this does not result in any end-user-facing changes. It will result in FlatConfigArray tests being run with npm test

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

  1. Are there any cases I missed?
  2. Is the code easy enough to follow? (The old config system was so complicated that the code was hard to follow, I want this version to be easier for people to look at and figure out what's going on.)
@eslint-github-bot
Copy link

@eslint-github-bot eslint-github-bot bot commented Apr 13, 2021

Hi @nzakas!, thanks for the Pull Request

The pull request title isn't properly formatted. We ask that you update the message to match this format, as we use it to generate changelogs and automate releases.

  • The commit message tag must be one of the following:

    The Tag is one of the following:

    • Fix - for a bug fix.
    • Update - either for a backwards-compatible enhancement or for a rule change that adds reported problems.
    • New - implements a new feature.
    • Breaking - for a backwards-incompatible enhancement or feature.
    • Docs - changes to documentation only.
    • Build - changes to build process only.
    • Upgrade - for a dependency upgrade.
    • Chore - for anything that isn't user-facing (for example, refactoring, adding tests, etc.).

    You can use the labels of the issue you are working on to determine the best tag.

  • There should be a space following the initial tag and colon, for example 'New: Message'.

Read more about contributing to ESLint here

@nzakas nzakas changed the title Flat config array Update: Implement FlatConfigArray (refs #13481) Apr 13, 2021
@eslint-github-bot
Copy link

@eslint-github-bot eslint-github-bot bot commented Apr 13, 2021

Hi @nzakas!, thanks for the Pull Request

The pull request title isn't properly formatted. We ask that you update the message to match this format, as we use it to generate changelogs and automate releases.

  • The commit message tag must be one of the following:

    The Tag is one of the following:

    • Fix - for a bug fix.
    • Update - either for a backwards-compatible enhancement or for a rule change that adds reported problems.
    • New - implements a new feature.
    • Breaking - for a backwards-incompatible enhancement or feature.
    • Docs - changes to documentation only.
    • Build - changes to build process only.
    • Upgrade - for a dependency upgrade.
    • Chore - for anything that isn't user-facing (for example, refactoring, adding tests, etc.).

    You can use the labels of the issue you are working on to determine the best tag.

  • There should be a space following the initial tag and colon, for example 'New: Message'.

  • The first letter of the tag should be in uppercase

Read more about contributing to ESLint here

@nzakas nzakas changed the title Update: Implement FlatConfigArray (refs #13481) Update: Implement FlatConfigArray (refs #13481) Apr 13, 2021
@nzakas nzakas requested a review from Apr 13, 2021
Copy link
Member

@btmills btmills left a comment

I am pleasantly surprised by how much less is going on here than in the existing config system. Outsourcing all of the file path matching to @humanwhocodes/config-array nicely separates the "what needs to be merged" logic from the "what are the rules for merging each field" logic. It's great to see the vision of a simpler config coming together 🎉

tests/lib/config/flat-config-array.js Show resolved Hide resolved
tests/lib/config/flat-config-array.js Show resolved Hide resolved
tests/lib/config/flat-config-array.js Show resolved Hide resolved
tests/lib/config/flat-config-array.js Show resolved Hide resolved
tests/lib/config/flat-config-array.js Show resolved Hide resolved
lib/config/flat-config-schema.js Outdated Show resolved Hide resolved
lib/config/flat-config-schema.js Show resolved Hide resolved
lib/config/default-config.js Show resolved Hide resolved
lib/config/flat-config-schema.js Show resolved Hide resolved
@nzakas
Copy link
Member Author

@nzakas nzakas commented Apr 19, 2021

I believe everything has been addressed, but let me know if I missed anything.

@nzakas nzakas requested a review from mdjermanovic Apr 22, 2021
Copy link
Member

@btmills btmills left a comment

I just did a thorough pass through the schema to make sure everything matches up. I'm excited that we'll be able to alert users to invalid configurations as close to the source as possible with some of the checks you've added. There were a few particularly helpful ones that aren't covered by a test, so I pointed those out in case you'd like to add those. LGTM either way.

lib/config/flat-config-schema.js Show resolved Hide resolved
lib/config/flat-config-schema.js Show resolved Hide resolved
lib/config/flat-config-schema.js Show resolved Hide resolved
lib/config/flat-config-array.js Show resolved Hide resolved
@nzakas
Copy link
Member Author

@nzakas nzakas commented Apr 23, 2021

Nice! I’ll plan on adding tests for those.

Copy link
Member

@btmills btmills left a comment

Nice! Sorry about the false positive on a couple of those.

rules: defaultRulesConfig
}
},
ignores: ["node_modules/**"],
Copy link
Member

@mdjermanovic mdjermanovic Apr 26, 2021

Is this now a glob pattern, or a .gitignore-style pattern as it used to be with ignorePatterns?

If it's a glob, should it be **/node_modules/*, as an equivalent to the actual default ignore pattern /**/node_modules/*?

Also, ignoring files in the RFC specifies .git directories. Should we add a pattern for them here?

Copy link
Member Author

@nzakas nzakas Apr 27, 2021

These are all glob patterns now. I’ll update them to match the current defaults.

Copy link
Member

@mdjermanovic mdjermanovic Apr 27, 2021

Does it also mean that we'll interpret the content of .eslintignore file differently, depending on the format of the found eslint configuration file:

  • as a list of .gitignore patterns (actual behavior) if the config is .eslintrc.*.
  • as a list of glob patterns if the config is eslint.config.js

Copy link
Member Author

@nzakas nzakas Apr 28, 2021

No. There is no change to how .eslintignore will be interpreted. It will be read and compiled into a function that is then included in a flat config ignores key. (Can be either a glob pattern or a function.)

lib/config/flat-config-array.js Show resolved Hide resolved
lib/config/flat-config-schema.js Outdated Show resolved Hide resolved
lib/config/flat-config-schema.js Outdated Show resolved Hide resolved
@nzakas
Copy link
Member Author

@nzakas nzakas commented Apr 28, 2021

Comments addressed.

@mdjermanovic
Copy link
Member

@mdjermanovic mdjermanovic commented Apr 29, 2021

@nzakas regarding #14321 (comment), isn't it the opposite (writable - correct, writeable - misspelling)?

Our documentation uses "writable", and says that "writeable" can be used for historical reasons.

lib/config/default-config.js Outdated Show resolved Hide resolved
lib/config/flat-config-schema.js Outdated Show resolved Hide resolved
lib/config/flat-config-schema.js Show resolved Hide resolved
lib/config/rule-validator.js Outdated Show resolved Hide resolved
lib/config/rule-validator.js Outdated Show resolved Hide resolved
lib/config/flat-config-schema.js Outdated Show resolved Hide resolved
lib/config/flat-config-schema.js Outdated Show resolved Hide resolved
lib/config/rule-validator.js Outdated Show resolved Hide resolved
lib/config/flat-config-schema.js Outdated Show resolved Hide resolved
lib/config/default-config.js Outdated Show resolved Hide resolved
lib/config/flat-config-array.js Show resolved Hide resolved
lib/config/rule-validator.js Show resolved Hide resolved
@nzakas
Copy link
Member Author

@nzakas nzakas commented May 3, 2021

I think I've fixed everything. @mdjermanovic thanks for the amazing level of detail in your comments.

lib/config/flat-config-schema.js Outdated Show resolved Hide resolved
lib/config/flat-config-schema.js Show resolved Hide resolved
@nzakas
Copy link
Member Author

@nzakas nzakas commented May 7, 2021

Should be set now (hopefully).

@stephenwade stephenwade mentioned this pull request May 11, 2021
1 task
lib/config/default-config.js Show resolved Hide resolved
lib/config/flat-config-array.js Show resolved Hide resolved
@nzakas nzakas force-pushed the flat-config-array branch from 5986e58 to 7b66408 Jun 25, 2021
Copy link
Member

@mdjermanovic mdjermanovic left a comment

LGTM!

I'm not sure if the feature of unignoring files that are ignored in the base config (or a shared config) is supported, but we can discuss that when we start integrating.

@mdjermanovic mdjermanovic merged commit b08170b into master Jun 26, 2021
12 checks passed
@mdjermanovic mdjermanovic deleted the flat-config-array branch Jun 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants