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

feat: no-restricted-imports support casing #15439

Merged
merged 3 commits into from Jan 15, 2022

Conversation

gfyoung
Copy link
Contributor

@gfyoung gfyoung commented Dec 19, 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)
[x] Changes an existing rule (template)
[ ] Add autofix to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:

What rule do you want to change?

no-restricted-imports

Does this change cause the rule to produce more or fewer warnings?

By default, the amount of warnings is unchanged. Otherwise, it should be fewer warnings.

How will the change be implemented? (New option, new default behavior, etc.)?

A new option is added, with a default value that maintains backwards-compatibility.

Please provide some example code that this change will affect:

import 'food';
import 'fooBar';
"no-restricted-imports": ["error", {
    "patterns": [{
      "group": ["foo[A-Z]*"]
    }]
}]

What does the rule currently do for this code?

It will issue warnings for both imports.

What will the rule do after it's changed?

If we add the new option:

"no-restricted-imports": ["error", {
    "patterns": [{
      "group": ["foo[A-Z]*"],
      "caseSensitive": true
    }]
}]

The rule will issue a warning ONLY for the second import.

What changes did you make? (Give an overview)

Enhances no-restricted-imports to support more granular import restrictions via case-sensitive pattern matching. By default, this rule remains case-insensitive to maintain backwards compatibility with earlier versions. Only by explicitly specifying it in a custom pattern object (see the added docs) can it be triggered.

Path matching in .gitignore can be case-sensitive, and ignore@5.2.0 supports this as well as maintains support for relative paths, whose support was dropped after 4.0.6 but restored in 5.2.0 (this was a blocker for past upgrades).

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

N/A

@eslint-github-bot eslint-github-bot bot added the triage An ESLint team member will look at this issue soon label Dec 19, 2021
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Dec 19, 2021

CLA Signed

The committers are authorized under a signed CLA.

@eslint-github-bot
Copy link

Hi @gfyoung!, thanks for the Pull Request

The first commit message 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 wasn't recognized. Did you mean "docs", "fix", or "feat"?
  • There should be a space following the initial tag and colon, for example 'feat: Message'.
  • The first letter of the tag should be in lowercase

To Fix: You can fix this problem by running git commit --amend, editing your commit message, and then running git push -f to update this pull request.

Read more about contributing to ESLint here

@eslint-github-bot eslint-github-bot bot added the bug ESLint is working incorrectly label Dec 19, 2021
@gfyoung gfyoung changed the title Update: no-restricted-imports support casing feat: no-restricted-imports support casing Dec 19, 2021
@mdjermanovic mdjermanovic added enhancement This change enhances an existing feature of ESLint evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion feature This change adds a new feature to ESLint rule Relates to ESLint's core rules and removed bug ESLint is working incorrectly triage An ESLint team member will look at this issue soon labels Dec 23, 2021
@mdjermanovic
Copy link
Member

The option makes sense to me 👍, and it would be nice to update the ignore dependency.

Waiting for more opinions from team members, since this is an enhancement.

@mdjermanovic mdjermanovic added this to Feedback Needed in Triage Dec 23, 2021
lib/shared/ignore.js Outdated Show resolved Hide resolved
@gfyoung
Copy link
Contributor Author

gfyoung commented Dec 28, 2021

@mdjermanovic: PR has been updated with the requested changes, with tests still passing. 👍

@btmills
Copy link
Member

btmills commented Dec 29, 2021

This seems reasonable, but before adding new API, I find it helpful to understand the motivation. @gfyoung can you elaborate a bit on what use case you're trying to accomplish with case-sensitive path patterns?

@gfyoung
Copy link
Contributor Author

gfyoung commented Dec 29, 2021

@btmills: I believe the use case described in the PR description illustrates the motivation.

If not, let me know what further information or detail is required.

@btmills
Copy link
Member

btmills commented Dec 29, 2021

@gfyoung the PR description covered a lot of the how but not much of the why. What are you hoping to do with ESLint that you can't today?

@gfyoung
Copy link
Contributor Author

gfyoung commented Dec 30, 2021

What are you hoping to do with ESLint that you can't today?

I want to be able to do more granular import restrictions using case-sensitive regular expressions.

@btmills
Copy link
Member

btmills commented Dec 30, 2021

Okay, and what are you hoping to use case-sensitive import restrictions for?

@gfyoung
Copy link
Contributor Author

gfyoung commented Dec 30, 2021

The example provided in the PR description illustrates a case where I would want to use case-sensitive import restrictions.

@nzakas
Copy link
Member

nzakas commented Dec 31, 2021

@gfyoung the problem is that your description looks like a theoretical problem and not a real-life use case. What @btmills is asking for is something more concrete. What situation did you run into where this option would be helpful?

@gfyoung
Copy link
Contributor Author

gfyoung commented Dec 31, 2021

the problem is that your description looks like a theoretical problem and not a real-life use case.

@nzakas: First off, expanding the API for the sake of a theoretical problem would be a waste of time for all of us. I have been waiting to submit this PR for quite some time, as I had to get commits to ignore merged and released first.

Without sharing actual code, I thought the example using foo would be minimal and sufficient to explain the situation where this option would be helpful. However, if you would like to see more realistic import names, here is an example, without sharing actual code, that should help to provide something more concrete:

import * as bots from bots;
import * as botUtils from botUtils;   // TODO: all imports should be from `bots`

In this case, we want all developers to importing from the bots file and not any of its constituent bot[A-Z]* files, which are in the process of being refactored and moved, but this will take quite some time to complete.

@nzakas
Copy link
Member

nzakas commented Jan 5, 2022

@gfyoung please understand, we review a lot of PRs, and not all of them are well thought out. We need to ask followup questions and for more examples in order to figure out which ones are worth pursuing and which don’t provide much value to the project.

Answering our questions helps a lot, and if the PR description was enough, we wouldn’t ask for more info.

Copy link
Member

@nzakas nzakas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, just a couple of suggestions.

docs/rules/no-restricted-imports.md Outdated Show resolved Hide resolved
lib/rules/no-restricted-imports.js Show resolved Hide resolved
Path matching in `.gitignore` can be case-sensitive, and
ignore@5.2.0 supports this as well as maintains support for
relative paths, whose support was dropped after 4.0.6 but
restored in 5.2.0 (this was a blocker for past upgrades).

By default, this rule remains case-insensitive to maintain
backwards compatibility with earlier versions. Only by explicitly
specifying it in a custom pattern object can it be triggered.
Per member feedback, the allowing of relative paths
was not entirely intentional for rules using the
`ignore` library. This commit also reverts the
utility for creating `ignore` instances.
@gfyoung
Copy link
Contributor Author

gfyoung commented Jan 5, 2022

@nzakas: I understand the need to request clarification. As a maintainer myself, I can relate to the experience of working through PRs that have not been as well thought out as we would like. That said, suggesting that someone's PR description is a "theoretical problem" before knowing more (e.g., not wanting to divulge actual code) is where some umbrage was taken. IMO prefer to give contributors benefit of the doubt to prove themselves before coming to a conclusion about the PR.

Anyhow, I'll leave it at that. Appreciate the feedback so far. 👍

I was able to address one of your feedback points but ran into issues with the other. See my comments in the conversations.

@mdjermanovic
Copy link
Member

Path matching in .gitignore can be case-sensitive, and ignore@5.2.0 supports this as well as maintains support for relative paths, whose support was dropped after 4.0.6 but restored in 5.2.0 (this was a blocker for past upgrades)

Question: The ignorecase option we need for this PR was already available in 4.0.6, but we must upgrade to 5.2.0 because of the caching bug that was fixed by kaelzhang/node-ignore@f2f67cb? (I noticed that the invalid test fails with 4.0.6, but works when the valid test is removed).

@gfyoung
Copy link
Contributor Author

gfyoung commented Jan 5, 2022

Yes, that is in part why (it's one of several patches I submitted to the library to make this upgrade here possible).

Copy link
Member

@mdjermanovic mdjermanovic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM if accepted, thanks!

@nzakas
Copy link
Member

nzakas commented Jan 6, 2022

@gfyoung Im sorry that I didn’t explain well enough. We get a lot of people suggesting rules or changes to rules that are, in fact, theoretical or unlikely enough that it doesn’t make sense to accept. I did not mean to imply that your use case was theoretical, only that we needed more justification. Maintaining all of our rules, and ensuring things like rules don’t contradict each other, takes a lot work, so while we always assume positive intent from contributors (as we did here), we can’t assume that every case should be merged, which is why we asked for more details.

@nzakas
Copy link
Member

nzakas commented Jan 6, 2022

@btmills do you have the info you need here?

Copy link
Member

@aladdin-add aladdin-add left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks! 💯

Copy link
Member

@btmills btmills left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks for the additional explanation.

@btmills btmills merged commit 19ad061 into eslint:main Jan 15, 2022
Triage automation moved this from Feedback Needed to Complete Jan 15, 2022
@gfyoung
Copy link
Contributor Author

gfyoung commented Jan 15, 2022

Fantastic! Thank you for all the feedback @btmills @mdjermanovic @nzakas 🙏

@gfyoung gfyoung deleted the case-sensitive-patterns branch January 15, 2022 23:06
@eslint-github-bot eslint-github-bot bot locked and limited conversation to collaborators Jul 15, 2022
@eslint-github-bot eslint-github-bot bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Jul 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
archived due to age This issue has been archived; please open a new issue for any further discussion enhancement This change enhances an existing feature of ESLint evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion feature This change adds a new feature to ESLint rule Relates to ESLint's core rules
Projects
Archived in project
Triage
Complete
Development

Successfully merging this pull request may close these issues.

None yet

5 participants