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

New: support specifying extensions in the config (fixes #10828) #12555

Closed
wants to merge 4 commits into from

Conversation

g-plane
Copy link
Member

@g-plane g-plane commented Nov 11, 2019

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

Add something to the core

What changes did you make? (Give an overview)
Support specifying extensions in configuration files.

The extensionRegExp property on FileEnumerator instance is removed. Instead, getExtensionRegExp method is introduced. (This isn't a breaking change because it isn't part of public APIs.)
This enables that ESLint reads configuration file and retrieves extensions on demand, instead of generating extensionRegExp in the constructor of FileEnumerator.

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

  • Does this change need more tests?
  • Even specifying any extensions in configuration file, .js is always included. Is this behavior correct? Or should it be overrided?

@eslint-deprecated eslint-deprecated bot added the triage An ESLint team member will look at this issue soon label Nov 11, 2019
@g-plane g-plane added this to Implemented, pending review in v7.0.0 Nov 11, 2019
@g-plane g-plane added accepted There is consensus among the team that this change meets the criteria for inclusion core Relates to ESLint's core APIs and features enhancement This change enhances an existing feature of ESLint and removed triage An ESLint team member will look at this issue soon labels Nov 11, 2019
@g-plane g-plane added the do not merge This pull request should not be merged yet label Nov 11, 2019
@mysticatea
Copy link
Member

Hi, @g-plane. Thank you for your work!

However, this direction is different a bit from the current consensus. As mentioned in #10828 (comment), we have RFC20 and RFC22 as approved approach. If you want to support the extentions top-level property, would you send an RFC similar to RFC22?

@g-plane
Copy link
Member Author

g-plane commented Nov 11, 2019

It's still in progress so it may be different with RFC, however, this PR is modeled after RFC20. Is this direction wrong?

@mysticatea
Copy link
Member

mysticatea commented Nov 11, 2019

It's no problem if the add of the extentions top-level property is temporary. I wanted to clarify because the issue description doesn't mention about WIP.

To make sure, RFC20 doesn't add any property to config files.

@mysticatea mysticatea added the breaking This change is backwards-incompatible label Nov 11, 2019
@g-plane
Copy link
Member Author

g-plane commented Nov 11, 2019

So should I close this if it's too early?

@mysticatea
Copy link
Member

No, feel free to continue to work on this. Just I wanted to clarify.

@g-plane
Copy link
Member Author

g-plane commented Nov 11, 2019

Should I send an RFC about adding this property before continuing?

@mysticatea
Copy link
Member

However, this direction is different a bit from the current consensus. As mentioned in #10828 (comment), we have RFC20 and RFC22 as approved approach. If you want to support the extentions top-level property, would you send an RFC similar to RFC22?

#12555 (comment)

@g-plane g-plane removed this from Implemented, pending review in v7.0.0 Nov 12, 2019
@g-plane g-plane closed this Nov 12, 2019
@g-plane g-plane deleted the issue-10828 branch November 12, 2019 11:04
@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators May 12, 2020
@eslint-deprecated eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label May 12, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion breaking This change is backwards-incompatible core Relates to ESLint's core APIs and features do not merge This pull request should not be merged yet enhancement This change enhances an existing feature of ESLint
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants