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.

Sign up for GitHub

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

Jump to bottom

fix(eslint-plugin): [return-await] correct handling of ternaries #2168

Merged
merged 6 commits into from Jun 5, 2020
Merged

fix(eslint-plugin): [return-await] correct handling of ternaries #2168

merged 6 commits into from Jun 5, 2020

Conversation

yeonjuan
Copy link
Contributor

@yeonjuan yeonjuan commented Jun 3, 2020

Fixes #1838

@typescript-eslint
Copy link
Contributor

Thanks for the PR, @yeonjuan!

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.

@bradzacher bradzacher added the bug Something isn't working label Jun 3, 2020
@yeonjuan
Copy link
Contributor Author

yeonjuan commented Jun 3, 2020 •
edited

@bradzacher
Please don't merge it. it needs a check for arrow function also. I forgot that

@bradzacher bradzacher added the awaiting response Issues waiting for a reply from the OP or another party label Jun 3, 2020
@codecov
Copy link

codecov bot commented Jun 3, 2020 •
edited

Codecov Report

Merging #2168 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #2168   +/-   ##
=======================================
  Coverage   93.45%   93.45%           
=======================================
  Files         171      171           
  Lines        7810     7813    +3     
  Branches     2228     2227    -1     
=======================================
+ Hits         7299     7302    +3     
  Misses        244      244           
  Partials      267      267
Flag Coverage Δ
#unittest 93.45% <100.00%> (+<0.01%) ⬆️
Impacted Files Coverage Δ
packages/eslint-plugin/src/rules/return-await.ts 95.77% <100.00%> (-0.18%) ⬇️
...plugin/src/rules/explicit-module-boundary-types.ts 88.96% <0.00%> (-0.19%) ⬇️
.../eslint-plugin/src/util/explicitReturnTypeUtils.ts 100.00% <0.00%> (+2.32%) ⬆️

@bradzacher bradzacher removed the awaiting response Issues waiting for a reply from the OP or another party label Jun 3, 2020
@yeonjuan
Copy link
Contributor Author

yeonjuan commented Jun 3, 2020

@bradzacher It seems ready to be reviewed :) sorry

bradzacher
bradzacher requested changes Jun 4, 2020
View reviewed changes
Copy link
Member

@bradzacher bradzacher left a comment

Choose a reason for hiding this comment

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

one nitpick, otherwise LGTM - thanks for this!

packages/eslint-plugin/src/rules/return-await.ts Outdated
Comment on lines 111 to 114
// There should always be a target node; but let's be safe.
/* istanbul ignore if */ if (!targetNode) {
/* istanbul ignore if */ if (!node) {
return null;
}
Copy link
Member

@bradzacher bradzacher Jun 4, 2020 •
edited

Choose a reason for hiding this comment

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

you don't need this if any more, because the argument is never undefined

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bradzacher Sure!

@bradzacher bradzacher added the awaiting response Issues waiting for a reply from the OP or another party label Jun 4, 2020
@bradzacher bradzacher removed the awaiting response Issues waiting for a reply from the OP or another party label Jun 5, 2020
bradzacher
bradzacher approved these changes Jun 5, 2020
View reviewed changes
Copy link
Member

@bradzacher bradzacher 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 your work!

@bradzacher bradzacher changed the title fix(eslint-plugin): [return-await] fix bug in ternaries fix(eslint-plugin): [return-await] correct handling of ternaries Jun 5, 2020
@bradzacher bradzacher merged commit fe4c0bf into typescript-eslint:master Jun 5, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 6, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@bradzacher bradzacher bradzacher approved these changes

Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[return-await] --fix (amusingly) confused by ternaries
2 participants
@yeonjuan @bradzacher