-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
feat(eslint-plugin): [no-floating-promises] add 'allowForKnownSafeCalls' option #9234
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(eslint-plugin): [no-floating-promises] add 'allowForKnownSafeCalls' option #9234
Conversation
…miseReturns' option
Thanks for the PR, @JoshuaKGoldberg! 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. |
✅ Deploy Preview for typescript-eslint ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
</TabItem> | ||
<TabItem value="✅ Correct"> | ||
|
||
```ts option='{"allowForKnownSafePromiseReturns":[{"from":"file","name":"safe","path":"input.ts"}]}' skipValidation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
skipValidation
This was a fun one to discover:
- The playground uses
input.ts
docs.test.ts
usesfile.ts
I'm stepping out, but separate from this PR we should fix the two to be in sync.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
packages/eslint-plugin/tests/rules/no-floating-promises.test.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
couple questions, no significant concerns.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just requesting a second look at https://github.com/typescript-eslint/typescript-eslint/pull/9234/files#r1626760319 before it's good to go 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## v8 #9234 +/- ##
==========================================
- Coverage 87.52% 87.49% -0.04%
==========================================
Files 387 386 -1
Lines 13197 13162 -35
Branches 3822 3814 -8
==========================================
- Hits 11551 11516 -35
Misses 1349 1349
Partials 297 297
Flags with carried forward coverage won't be shown. Click here to find out more.
|
PR Checklist
Overview
Uses the same format and plumbing as the existing
allowForKnownSafePromises
(#7008 -> #9186). The actual changes are pretty straightforward as a result.Note that this doesn't yet add
node:test
support, as that's blocked by #6861. I plan on sending a followup PR after this one goes in to fix that - usingProgram.sourceFileToPackageName
if we really need.Sending to the
v8
branch instead ofmain
because there are some project service fixes in there that this relies on. Example errors from targeting themain
branch: https://github.com/typescript-eslint/typescript-eslint/actions/runs/9355798709/job/25751928593?pr=9234#logs. Update: or maybe those are also inv8
. More investigation required.💖