Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

feat(eslint-plugin): [no-duplicate-type-constituents] prevent unnecessary| undefined for optional parameters#9479

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

Conversation

@abrahamguo
Copy link
Contributor

PR Checklist

@typescript-eslint
Copy link
Contributor

Thanks for the PR,@abrahamguo!

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 onhttps://opencollective.com/typescript-eslint.

@netlify
Copy link

netlifybot commentedJul 2, 2024
edited
Loading

Deploy Preview fortypescript-eslint ready!

NameLink
🔨 Latest commit63d6400
🔍 Latest deploy loghttps://app.netlify.com/sites/typescript-eslint/deploys/66cdc4456c44300008d82dd2
😎 Deploy Previewhttps://deploy-preview-9479--typescript-eslint.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
Lighthouse
Lighthouse
1 paths audited
Performance: 99 (no change from production)
Accessibility: 100 (no change from production)
Best Practices: 92 (no change from production)
SEO: 90 (no change from production)
PWA: 80 (no change from production)
View the detailed breakdown and full score reports

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

@nx-cloud
Copy link

nx-cloudbot commentedJul 2, 2024
edited
Loading

☁️ Nx Cloud Report

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

📂 See all runs for this CI Pipeline Execution


✅ Successfully ran 30 targets

Sent with 💌 fromNxCloud.

@codecov
Copy link

codecovbot commentedJul 2, 2024
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.06%. Comparing base(ef2eab1) to head(63d6400).
Report is 19 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@##             main    #9479      +/-   ##==========================================+ Coverage   88.05%   88.06%   +0.01%==========================================  Files         407      407                Lines       13947    13961      +14       Branches     4071     4077       +6     ==========================================+ Hits        12281    12295      +14  Misses       1354     1354                Partials      312      312
FlagCoverage Δ
unittest88.06% <100.00%> (+0.01%)⬆️

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

Files with missing linesCoverage Δ
...plugin/src/rules/no-duplicate-type-constituents.ts100.00% <100.00%> (ø)
packages/eslint-plugin/src/rules/prefer-find.ts100.00% <ø> (ø)

Comment on lines 180 to 206
TSUnionType(node):void{
checkDuplicate(node);
constmaybeTypeAnnotation=node.parent;
if(maybeTypeAnnotation.type===AST_NODE_TYPES.TSTypeAnnotation){
constmaybeIdentifier=maybeTypeAnnotation.parent;
if(
maybeIdentifier.type===AST_NODE_TYPES.Identifier&&
maybeIdentifier.optional
){
constmaybeFunction=maybeIdentifier.parent;
const{ type}=maybeFunction;
if(
(type===AST_NODE_TYPES.ArrowFunctionExpression||
type===AST_NODE_TYPES.FunctionDeclaration||
type===AST_NODE_TYPES.FunctionExpression)&&
maybeFunction.params.includes(maybeIdentifier)
){
constexplicitUndefined=node.types.find(
({ type})=>type===AST_NODE_TYPES.TSUndefinedKeyword,
);
if(explicitUndefined){
report('unnecessary',explicitUndefined);
}
}
}
}
},
Copy link
Member

Choose a reason for hiding this comment

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

[Bug] This case isn't reported

functiona(arg?:number|(string|undefined)){}

playground

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

This is not currently handled even in the existing rule functionality — we'd need to implement some recursion handling for this. I've gone ahead and opened#9496 for further discussion

@auvredauvred added the awaiting responseIssues waiting for a reply from the OP or another party labelJul 4, 2024
@abrahamguoabrahamguo requested a review fromauvredJuly 5, 2024 04:26
@github-actionsgithub-actionsbot removed the awaiting responseIssues waiting for a reply from the OP or another party labelJul 5, 2024
@abrahamguo
Copy link
ContributorAuthor

Work done:

  • MovedreportDuplicate into a more inner scope so that it can readconstituentNode from scope
  • Rename it toreport because it can now accept a message ID
  • Update autofixing logic to now support removing a& or|after the duplicate node — this is necessary because a duplicate node will never be the first member of an intersection or union, but a uselessundefined can be the first member of an intersection or union
  • Add parameter tocheckDuplicate to allow additional checks on just unions or just intersections
  • Update docs and add new lint message

Ready for review!

type===AST_NODE_TYPES.FunctionDeclaration||
type===AST_NODE_TYPES.FunctionExpression)&&
(isFunctionOrFunctionType(maybeFunction)||
maybeFunction.type===AST_NODE_TYPES.TSDeclareFunction)&&
Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Created#9788 to addTSDeclareFunction inutils.

@auvred
Copy link
Member

Let's wait for#9788 to get merged

abrahamguo reacted with heart emoji

@auvredauvred added the blocked by another PRPRs which are ready to go but waiting on another PR labelAug 24, 2024
@bradzacherbradzacher removed the blocked by another PRPRs which are ready to go but waiting on another PR labelAug 25, 2024
@abrahamguo
Copy link
ContributorAuthor

Updated now that#9788 has been merged ✅

Copy link
Member

@auvredauvred left a comment

Choose a reason for hiding this comment

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

💯

@auvredauvred added the 1 approval>=1 team member has approved this PR; we're now leaving it open for more reviews before we merge labelAug 28, 2024
@JoshuaKGoldbergJoshuaKGoldberg merged commitf44da95 intotypescript-eslint:mainSep 2, 2024
@jswalden
Copy link

Hi, maybe I'm missing something here, but...why does this lint make sense to have for&?

For|, if you're intending to concatenate type sets rather than combine them, you might well be presuming their disjointedness.

But for&, if the type sets don't contain a duplication...aren't you just going to vacuously get the empty type set (which I presume meansnever)?

Maybe I'm just too TypeScript n00b. But it seems like if you're intersecting type sets not with intention of asserting non-overlap, the only way you get any value from doing so is if thereis duplication. (This was, in fact, exactly what I was intentionally doing when I just ran into this in my own code.)

@abrahamguo
Copy link
ContributorAuthor

abrahamguo commentedSep 6, 2024
edited
Loading

@jswalden& can be useful!

One place I like to use it is to combine two object types, like so:

type Animal = { name: string };type Bear = Animal & { honey: boolean };

Here, we're using& to combine theAnimal type and add additional properties — this is an example of using& without duplication. TheBear type will end up including both the property fromAnimaland the additional propertyhoney.

@jswalden
Copy link

@abrahamguo Interesting! I probably should have realized that possibility. 🙂

I guess I don't have good ideas about& where it is not intended to build up an intersection type that way -- other than the explicit disabling comment I eventually resorted to. I'd suggest at least keep an eye out to see how many other people are in my boat, tho. If other people run into this, in "sufficient" quantity (you'd be a far better judge of what that is than me!), it might be worth thinking about if there's anything that could be done to treat the two use cases separately "somehow".

@abrahamguo
Copy link
ContributorAuthor

Can you show the example from your code, where you ended up needing to use a disable comment?

@jswalden
Copy link

@abrahamguo The lines are here:https://github.com/bitfocus/companion-module-allenheath-sq/blob/50255dd03c4d2c2efc9a1e84e2a97483570c29f2/src/presets.ts#L17

I'll try to briefly summarize the enclosing use, as the line alone is probably obscure.

Companion is software you can use to define connections to control devices (or more-abstract stuff like a Slack connection or a Google spreadsheet), by programming buttons on various hardware devices (such as theStream Deck). My code defines a module implementing a connection to control an Allen & Heath sound mixer.

Companion modules in part define a set of "actions" Companion users can invoke to control the underlying device, and a set of "feedbacks" that expose device status changes. Users can assign series of actions to a hardware button, to be performed when it's pressed/released. A modulealso can define "presets": predefined buttons for simple, common use cases.

Actions and feedbacks are identified using separate string ID sets. In this module, I represent "mute this input or output" action and feedback IDs using theMuteActionId andMuteFeedbackId enums.

This module has presets that show whether a particular mixer input/output is muted, that will toggle muting when pressed. When I define these presets, I use the keys of those enums to specify the particular kind of mute button being defined. Iintersect those keys to get a key that identifies both a mute action and a mute feedback. I can then useMuteActionId[key] orMuteFeedbackId[key] to get an ID of the desired type, without risking a typo.

I hope that's reasonably clear without being excessively detailed!

@abrahamguo
Copy link
ContributorAuthor

In this case, shouldn'tMuteFeedbackId andMuteActionId be merged, since they are identical to each other?
That is whyno-duplicate-type-constituents is reporting — because the twoenums are identical to each other, and therefore it's unnecessary to combine them with&.

If the twoenums were different from each other, with only some properties in common, then the lint rule would not report.

@jswalden
Copy link

@abrahamguo They're type-identical, but they are notsemantically identical. Feedback IDs aren't the same as action IDs.

@github-actionsgithub-actionsbot locked asresolvedand limited conversation to collaboratorsSep 16, 2024
Sign up for freeto subscribe to this conversation on GitHub. Already have an account?Sign in.

Reviewers

@auvredauvredauvred approved these changes

Assignees

No one assigned

Labels

1 approval>=1 team member has approved this PR; we're now leaving it open for more reviews before we merge

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Enhancement: [no-duplicate-type-constituents] Prevent unnecessary| undefined for optional parameters

5 participants

@abrahamguo@auvred@jswalden@JoshuaKGoldberg@bradzacher

[8]ページ先頭

©2009-2025 Movatter.jp