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(eslint-plugin): [no-misused-promises] check more places for checksVoidReturn #4541

Merged
merged 22 commits into from Feb 24, 2022

Conversation

uhyo
Copy link
Contributor

@uhyo uhyo commented Feb 12, 2022

PR Checklist

Overview

The checksVoidReturn: true option only checks function arguments now. This PR extends the rule so that misused Promises at other places are checked too. Namely, variable assignments, object properties, return statements and JSX props are newly checked.

See the added tests for examples of newly-incorrect code.

@nx-cloud
Copy link

nx-cloud bot commented Feb 12, 2022

☁️ Nx Cloud Report

CI ran the following commands for commit 42701f5. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch


✅ Successfully ran 48 targets

Sent with 💌 from NxCloud.

@typescript-eslint
Copy link
Contributor

Thanks for the PR, @uhyo!

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. As a thank you, your profile/company logo will be added to our main README which receives thousands of unique visitors per day.

@netlify
Copy link

netlify bot commented Feb 12, 2022

❌ Deploy Preview for typescript-eslint failed.

🔨 Explore the source changes: 42701f5

🔍 Inspect the deploy log: https://app.netlify.com/sites/typescript-eslint/deploys/6217b76f95b0d100079e1af4

@codecov
Copy link

codecov bot commented Feb 12, 2022

Codecov Report

Merging #4541 (42701f5) into main (823b945) will increase coverage by 0.03%.
The diff coverage is 98.11%.

@@            Coverage Diff             @@
##             main    #4541      +/-   ##
==========================================
+ Coverage   94.62%   94.65%   +0.03%     
==========================================
  Files         149      150       +1     
  Lines        8072     8162      +90     
  Branches     2581     2617      +36     
==========================================
+ Hits         7638     7726      +88     
- Misses        239      241       +2     
  Partials      195      195              
Flag Coverage Δ
unittest 94.65% <98.11%> (+0.03%) ⬆️

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

Impacted Files Coverage Δ
packages/eslint-plugin/src/configs/all.ts 100.00% <ø> (ø)
...ckages/eslint-plugin/src/util/getESLintCoreRule.ts 75.00% <ø> (ø)
...ges/eslint-plugin/src/rules/no-misused-promises.ts 98.63% <97.18%> (-1.37%) ⬇️
...ackages/eslint-plugin/src/rules/member-ordering.ts 95.68% <100.00%> (+0.12%) ⬆️
.../src/rules/sort-type-union-intersection-members.ts 92.53% <100.00%> (+0.11%) ⬆️
...ges/eslint-plugin/src/rules/space-before-blocks.ts 100.00% <100.00%> (ø)

@JoshuaKGoldberg
Copy link
Member

JoshuaKGoldberg commented Feb 12, 2022

Screenshot of "11 hours ago"

Screenshot of 9 thumbs up reactions

...is there a collective group project or hackathon going on or something? A micro viral tweet? I did not expect this PR to be so popular so quick after posting 😄

Screenshot of commit message: "worship code coverage"

🙏 ✨

@bradzacher bradzacher added the enhancement New feature or request label Feb 14, 2022
Copy link
Member

@JoshuaKGoldberg JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

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

Generally looks great to me so far, thanks for tackling all these cases @uhyo!

Requesting changes on the error message I commented on. Also, codecov (now that it's done changing its mind) is correct, there are some untested lines of code. Please do fill those in.

packages/eslint-plugin/src/rules/no-misused-promises.ts Outdated Show resolved Hide resolved
packages/eslint-plugin/src/rules/no-misused-promises.ts Outdated Show resolved Hide resolved
packages/eslint-plugin/src/rules/no-misused-promises.ts Outdated Show resolved Hide resolved
packages/eslint-plugin/src/rules/no-misused-promises.ts Outdated Show resolved Hide resolved
packages/eslint-plugin/src/rules/no-misused-promises.ts Outdated Show resolved Hide resolved
@JoshuaKGoldberg JoshuaKGoldberg added the awaiting response Issues waiting for a reply from the OP or another party label Feb 23, 2022
@uhyo
Copy link
Contributor Author

uhyo commented Feb 24, 2022

@JoshuaKGoldberg Thank you for review!
Coverage is now 100% and I addressed your points. 🙏

Copy link
Member

@JoshuaKGoldberg 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, one more bug? 😄

packages/eslint-plugin/src/rules/no-misused-promises.ts Outdated Show resolved Hide resolved
Copy link
Member

@JoshuaKGoldberg JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

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

🔥

@JoshuaKGoldberg JoshuaKGoldberg enabled auto-merge (squash) February 24, 2022 17:51
@JoshuaKGoldberg JoshuaKGoldberg removed the awaiting response Issues waiting for a reply from the OP or another party label Feb 24, 2022
@JoshuaKGoldberg JoshuaKGoldberg merged commit 052cf51 into typescript-eslint:main Feb 24, 2022
@uhyo uhyo deleted the fix/4523 branch February 25, 2022 03:44
@lgenzelis
Copy link
Contributor

thanks for the improvement @uhyo ! I'm wondering, is there a way to disable only the Promise-returning function provided to attribute where a void return was expected error? I find it too strict for my React codebase. I have code like this in many places:

function MyButton() {
   const onClick = async () => {
      const b = await doSomething();
      doSomethingElse(b);
   }

  return <button onClick={onClick}>Click me!</button>;
}

and now that error is being (correctly!) detected on those places.

I know I could do '@typescript-eslint/no-misused-promises': ['error', { checksVoidReturn: false }],, but I don't want to disable all the other error cases. I'm wondering if something more like '@typescript-eslint/no-misused-promises': ['error', { checksVoidReturningArgument: false }], exists / is on the roadmap.

@bradzacher
Copy link
Member

As per our contributing guide - please don't comment on closed PRs.

Instead please use issues to start new discussions and find existing ones like #4619 or #4609

@typescript-eslint typescript-eslint locked as resolved and limited conversation to collaborators Mar 1, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[no-misused-promises] Suggestion: check more AST positions
4 participants