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-misused-promises] check subtype methods against heritage type methods#8765

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

alythobani
Copy link
Contributor

@alythobanialythobani commentedMar 25, 2024•
edited
Loading

PR Checklist

Overview

This PR adds a newheritageTypes option (underchecksVoidReturn) to theno-misused-promises rule, defaulted totrue. If enabled, it checksClassDeclaration /ClassExpression /InterfaceDeclaration nodes for any methods that returnPromises, and checks those methods against any matching methods in their declaredextends ... /implements ... heritage types. Error outputted when a heritage type has the given method declared/implemented as returningvoid instead of aPromise.

(The issue#8739 I filed only mentioned abstract classes/methods, but I realized the topic is more generalizable to heritage types in general.)

Note that this is my first PR here and first time diving into this repo / the Typescript source code. So hopefully I didn't miss any cases to test / handle, but it's possible I did! There did end up being small edge cases I realized weren't handled as I iterated and had to tweak things. E.g., callingchecker.getTypeAtLocation(heritageTypeExpression) ended up being necessary in order to properly check against aClassExpression heritage type's members;checker.getTypeFromTypeNode(heritageTypeExpression) worked for all other heritage types, just not class expressions.

@typescript-eslint
Copy link
Contributor

Thanks for the PR,@alythobani!

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.

@netlifyNetlify
Copy link

netlifybot commentedMar 25, 2024•
edited
Loading

✅ Deploy Preview fortypescript-eslint ready!

NameLink
🔨 Latest commit8304cef
🔍 Latest deploy loghttps://app.netlify.com/sites/typescript-eslint/deploys/66ba352e6d1ab700084b289a
😎 Deploy Previewhttps://deploy-preview-8765--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 (🟢 up 4 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-cloudNx Cloud
Copy link

nx-cloudbot commentedMar 25, 2024•
edited
Loading

☁️ Nx Cloud Report

CI is running/has finished running commands for commit8304cef. 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.

@alythobanialythobani changed the titlefeat(eslint-plugin):checksVoidReturn.subtypes option added tono-misused-promises rule (checks methods against heritage type methods)feat(eslint-plugin): Check against heritage type methods (no-misused-promises / checksVoidReturn.subtypes)Mar 25, 2024
@alythobanialythobani changed the titlefeat(eslint-plugin): Check against heritage type methods (no-misused-promises / checksVoidReturn.subtypes)feat(eslint-plugin): check subtype methods against heritage type methods (no-misused-promises)Mar 25, 2024
@bradzacherbradzacher changed the titlefeat(eslint-plugin): check subtype methods against heritage type methods (no-misused-promises)feat(eslint-plugin): [no-misused-promises] check subtype methods against heritage type methodsMar 25, 2024
@codecovCodecov
Copy link

codecovbot commentedMar 25, 2024•
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.91%. Comparing base(3d9ae44) to head(8304cef).
Report is 2 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@##             main    #8765   +/-   ##=======================================  Coverage   87.91%   87.91%           =======================================  Files         402      402             Lines       13725    13775   +50       Branches     3998     4019   +21     =======================================+ Hits        12066    12110   +44- Misses       1352     1358    +6  Partials      307      307
FlagCoverage Δ
unittest87.91% <100.00%> (+<0.01%)⬆️

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

FilesCoverage Δ
...ges/eslint-plugin/src/rules/no-misused-promises.ts97.51% <100.00%> (+0.32%)⬆️

Copy link
Member

@JoshuaKGoldbergJoshuaKGoldberg left a comment•
edited
Loading

Choose a reason for hiding this comment

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

Looks like a great start, nice! I haven't reviewed to deeply yet, just waiting on the missing test coverage in the file in case it needs some big changes (you never know). LMK if I'm off base though! ✨

alythobani reacted with heart emoji
@JoshuaKGoldbergJoshuaKGoldberg added the awaiting responseIssues waiting for a reply from the OP or another party labelMar 25, 2024
@JoshuaKGoldberg
Copy link
Member

I realized the topic is more generalizable to heritage types in general

+1, this is a nice spot!

alythobani reacted with heart emoji

- checksVoidReturn sub-options each get their own subsection- option descriptions go under Options subsection instead of Examples
@github-actionsgithub-actionsbot removed the awaiting responseIssues waiting for a reply from the OP or another party labelJun 7, 2024
Copy link
Member

@JoshuaKGoldbergJoshuaKGoldberg 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! I commented on the last remaining thread.

@github-actionsgithub-actionsbot removed the awaiting responseIssues waiting for a reply from the OP or another party labelJul 17, 2024
JoshuaKGoldberg
JoshuaKGoldberg previously approved these changesJul 20, 2024
Copy link
Member

@JoshuaKGoldbergJoshuaKGoldberg left a comment

Choose a reason for hiding this comment

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

I have been mollified. 😄

This really is a lovely PR with a super interesting bug case. Thanks for all your work on it@alythobani! 🙌

alythobani and kirkwaiblinger reacted with rocket emoji
@JoshuaKGoldbergJoshuaKGoldberg added the 1 approval>=1 team member has approved this PR; we're now leaving it open for more reviews before we merge labelJul 20, 2024
@alythobani
Copy link
ContributorAuthor

😁 Sweet, and thanks for such a detailed review@JoshuaKGoldberg! Really appreciate all the catches and suggestions and all the time you put into it! And for putting up with my overly long explanations 😇

JoshuaKGoldberg and kirkwaiblinger reacted with heart emoji

JoshuaKGoldberg
JoshuaKGoldberg previously approved these changesJul 29, 2024
Copy link
Member

@JoshuaKGoldbergJoshuaKGoldberg left a comment

Choose a reason for hiding this comment

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

Looks great to me, thanks! Since@kirkwaiblinger was also reviewing, will wait a bit for Kirk to review.

alythobani reacted with rocket emoji
kirkwaiblinger
kirkwaiblinger previously approved these changesAug 1, 2024
Copy link
Member

@kirkwaiblingerkirkwaiblinger left a comment

Choose a reason for hiding this comment

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

🚀 😁

if (memberName === undefined) {
// Call/construct/index signatures don't have names. TS allows call signatures to mismatch,
// and construct signatures can't be async.
// TODO - Once we're able to use `checker.isTypeAssignableTo` (v8), we can check an index

Choose a reason for hiding this comment

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

v8 is upon us! Though of course this PR has been in the works since long before that.

@alythobani, let's merge as-is, but would you mind filing a followup (that we now know will not be blocked 🙂)

JoshuaKGoldberg reacted with thumbs up emoji
Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Sweet, yeah definitely! Do you mean filing an issue for this? And I can start looking into a proper implementation too if you'd like

@JoshuaKGoldbergJoshuaKGoldberg dismissed stale reviews fromkirkwaiblinger and themself via8304cefAugust 12, 2024 16:15
Copy link
Member

@JoshuaKGoldbergJoshuaKGoldberg left a comment•
edited
Loading

Choose a reason for hiding this comment

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

All right!~ This is looking great! Thanks so much for working with us a bunch on it@alythobani. Your explanations were really helpful and I'm super happy where this went. Awesome job.

Merging now to be released later today in our weekly release! 🙌

Homer Simpson raising his fists up and yelling "Whoo-hoo!"

alythobani reacted with hooray emojialythobani reacted with heart emojialythobani reacted with rocket emoji
@JoshuaKGoldbergJoshuaKGoldberg merged commit6a1c177 intotypescript-eslint:mainAug 12, 2024
59 checks passed
@alythobani
Copy link
ContributorAuthor

Sorry again for the late replies here, life is a little hectic these days! But amaaazing, so good to see this merged! Thanks again so much for all the collaboration here@JoshuaKGoldberg and@kirkwaiblinger !!

Obviously please keep me updated on any issues that arise, but hopefully there won't be any :)

kirkwaiblinger and JoshuaKGoldberg reacted with heart emoji

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

@JoshuaKGoldbergJoshuaKGoldbergJoshuaKGoldberg approved these changes

@kirkwaiblingerkirkwaiblingerkirkwaiblinger left review comments

@auvredauvredAwaiting requested review from auvred

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 mergeenhancementNew feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Enhancement: [no-misused-promises] Check when an abstract method implementation returns a promise instead of void
5 participants
@alythobani@JoshuaKGoldberg@kirkwaiblinger@auvred@bradzacher
[8]ページ先頭
©2009-2025 Movatter.jp