typescript-eslint/typescript-eslintPublic

NotificationsYou must be signed in to change notification settings
Fork2.8k
Star15.7k

fix(type-utils): allow`FileSpecifier` to exclude paths#6860

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Closed

RebeccaStevens wants to merge1 commit intotypescript-eslint:mainfromRebeccaStevens:issue/6819

Closed

fix(type-utils): allow`FileSpecifier` to exclude paths#6860

RebeccaStevens wants to merge1 commit intotypescript-eslint:mainfromRebeccaStevens:issue/6819

Conversation

Copy link

Contributor

RebeccaStevens commentedApr 7, 2023

PR Checklist

Addresses an existing open issue:fixesBug:typeMatchesSpecifierFileSpecifier checks in node modules #6819
That issue was marked asaccepting prs
Steps inContributing were taken

Overview

AddsexcludePaths option toFileSpecifier (has no effect if apath is given).
This new option defaults to["node_modules"].

fix(type-utils): allow FileSpecifier to excludePaths. defaults to exc…

db94a99

…luding node_modules

Copy link

Contributor

typescript-eslintbot commentedApr 7, 2023

Thanks for the PR,@RebeccaStevens!

typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community.

The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately.

Thanks again!

🙏Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint.

Copy link

netlifybot commentedApr 7, 2023•
edited
Loading

✅ Deploy Preview fortypescript-eslint ready!

Name	Link
🔨 Latest commit	`db94a99`
🔍 Latest deploy log	https://app.netlify.com/sites/typescript-eslint/deploys/643014f5acb7b7000869114a
😎 Deploy Preview	https://deploy-preview-6860--typescript-eslint.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to yourNetlify site settings.

Copy link

nx-cloudbot commentedApr 7, 2023•
edited
Loading

☁️ Nx Cloud Report

CI is running/has finished running commands for commitdb94a99. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch

✅ Successfully ran 30 targets

Sent with 💌 fromNxCloud.

Copy link

codecovbot commentedApr 7, 2023

Codecov Report

Merging#6860 (db94a99) intov6 (a35a2bb) willdecrease coverage by0.02%.
The diff coverage is75.00%.

Additional details and impacted files

@@            Coverage Diff             @@##               v6    #6860      +/-   ##==========================================- Coverage   87.46%   87.45%   -0.02%==========================================  Files         374      374                Lines       12879    12884       +5       Branches     3811     3814       +3     ==========================================+ Hits        11265    11268       +3- Misses       1229     1230       +1- Partials      385      386       +1

Flag	Coverage Δ
unittest	`87.45% <75.00%> (-0.02%)`	⬇️

Flags with carried forward coverage won't be shown.Click here to find out more.

Impacted Files	Coverage Δ
packages/type-utils/src/TypeOrValueSpecifier.ts	`84.84% <75.00%> (-4.44%)`	⬇️

marekdedic suggested changes

Apr 7, 2023

View reviewed changes

Copy link

Contributor

marekdedic left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, you truly beat me to it by just a few minutes :D

As I wrote in the comments, I'm not a fan of the paths being configurable and would just hard-codenode_modules there.

However, if you do decide to go ahead with theexcludePaths option,typeOrValueSpecifierSchema needs to be modified as well :)

packages/type-utils/src/TypeOrValueSpecifier.tsShow resolvedHide resolved

RebeccaStevens mentioned this pull request

Apr 8, 2023

fix(type-utils): ensure external sources don't match for theFileSpecifier#6866

Closed

3 tasks

bradzacher added the bugSomething isn't working label

Apr 10, 2023

Copy link

Member

JoshuaKGoldberg commentedApr 16, 2023

I'm going to wait on discussion in#6839 - it might impact the config option here.

JoshuaKGoldberg added the blocked by another PRPRs which are ready to go but waiting on another PR label

Apr 16, 2023

JoshuaKGoldberg deleted the branchtypescript-eslint:main

July 10, 2023 17:52

JoshuaKGoldberg closed this

Jul 10, 2023

Copy link

Member

JoshuaKGoldberg commentedJul 10, 2023•
edited
Loading

This was unintentionally auto-closed when we merged thev6 branch 🙃 it'll be~~recreated~~ reopened.

JoshuaKGoldberg reopened this

Jul 10, 2023

JoshuaKGoldberg changed the base branch fromv6 tomain

July 10, 2023 21:11

Copy link

Member

JoshuaKGoldberg commentedOct 17, 2023

Sorry for the delay -#6839 is accepting PRs so this is unblocked!

JoshuaKGoldberg removed the blocked by another PRPRs which are ready to go but waiting on another PR label

Oct 17, 2023

JoshuaKGoldberg requested changes

Oct 17, 2023

View reviewed changes

Copy link

Member

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

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

Sorry for the delay here! I think the changes generally make sense, but I'm leaning towards keeping it less complex by not makingexcludePaths andpath mutually exclusive. What do you think?

packages/type-utils/src/TypeOrValueSpecifier.ts

		path?: undefined;
		excludePaths?: string \| string[]; // defaults to `["node_modules"]`.
		}
		);

Copy link

Member

JoshuaKGoldbergOct 17, 2023

Choose a reason for hiding this comment

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

Starting a new conversation here: why makeexcludePaths andpath mutually exclusive? What if someone wants to provide both? E.g.:

{path:"examples",excludePaths:["examples/*/node_modules"]}

Proposal: can we switch this back to a straightforward interface?

interfaceFileSpecifier{from:'file';name:string|string[];path?:string;excludePaths?:string|string[];// defaults to `["node_modules"]`.}

packages/type-utils/src/TypeOrValueSpecifier.ts

		}
		\| {
		path?: undefined;
		excludePaths?: string \| string[]; // defaults to `["node_modules"]`.

Copy link

Member

JoshuaKGoldbergOct 17, 2023

Choose a reason for hiding this comment

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

I also wonder if we should call thispathExcludes, to visually lump it in withpath... 🤔

JoshuaKGoldberg added the awaiting responseIssues waiting for a reply from the OP or another party label

Oct 17, 2023

Copy link

Member

JoshuaKGoldberg commentedNov 13, 2023

👋 ping@RebeccaStevens, just checking in - is this still something you have time for? No worries if not! I just don't want to leave it hanging.

JoshuaKGoldberg added the stalePRs or Issues that are at risk of being or have been closed due to inactivity for a prolonged period label

Nov 13, 2023

Copy link

Member

JoshuaKGoldberg commentedJan 8, 2024

Closing this PR as it's been stale for a while without activity. Feel free to reopen@RebeccaStevens if you have time - but no worries if not!

If anybody wants to drive it forward, please do post your own PR - and if you use this as a start, consider adding a co-author at the end of your PR description. Thanks! 😊