fix(eslint-plugin): [return-await] don't error for `in-try-catch` if the return is in a `catch` without a `finally` #2356

soobing · 2020-08-04T13:19:05Z

Added below two TC, and fixed.

if you return a promise within a catch, and there is no finally, then it must not be awaited.
if you return a promise within a finally, then it must not be awaited.

typescript-eslint · 2020-08-04T13:19:12Z

Thanks for the PR, @soobing!

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.

codecov · 2020-08-04T13:29:15Z

Codecov Report

Merging #2356 into master will increase coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #2356      +/-   ##
==========================================
+ Coverage   92.81%   92.82%   +0.01%     
==========================================
  Files         290      290              
  Lines        9453     9481      +28     
  Branches     2647     2657      +10     
==========================================
+ Hits         8774     8801      +27     
- Misses        322      323       +1     
  Partials      357      357

Flag	Coverage Δ
#unittest	`92.82% <ø> (+0.01%)`	⬆️

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

Impacted Files	Coverage Δ
packages/eslint-plugin/src/rules/return-await.ts	`95.69% <ø> (-0.08%)`	⬇️
packages/experimental-utils/src/ts-eslint/Scope.ts	`100.00% <0.00%> (ø)`
...es/eslint-plugin/src/rules/no-use-before-define.ts	`94.36% <0.00%> (ø)`
packages/eslint-plugin/src/rules/no-unused-vars.ts	`93.06% <0.00%> (+0.21%)`	⬆️

bradzacher · 2020-08-04T17:22:18Z

So the issue in question isn't that the code was erroring - it's that it shouldn't error.

The code should work in the following way for in-try-catch mode:

if you return a promise within a try, then it must be awaited.
if you return a promise within a catch, and there is no finally, then it must not be awaited.
if you return a promise within a catch, and there is a finally, then it must be awaited.
if you return a promise within a finally, then it must not be awaited.

soobing · 2020-08-05T13:50:56Z

Aha.. thanks for the kind explanation.
I will try again.

This reverts commit aba2f89.

soobing · 2020-08-19T15:15:11Z

@bradzacher
Hi, I tried again. Please review my code 😋.
I also change this PR's description #2356 (comment)

soobing · 2020-08-25T15:03:17Z

@bradzacher hi, could you review again? I don't know How to ask review again so I am mentioning 🦊

bradzacher

The code LGTM.
Thanks for your work!

Could you please update the docs for the in-try-catch option?

Specifically, could you make it very clear how it works, doing something like this will be good.

`in-try-catch`

Requires that a returned promise must be awaited in try-catch-finally blocks, and disallows it elsewhere. Specifically:

if you return a promise within a try, then it must be awaited.
if you return a promise within a catch, and there is no finally, then it must not be awaited.
if you return a promise within a catch, and there is a finally, then it must be awaited.
if you return a promise within a finally, then it must not be awaited.

soobing · 2020-09-07T12:42:58Z

@bradzacher Thanks for review!
I updated the docs and also added some examples.

bradzacher

LGTM - thanks for your work

test: throw in try block

aba2f89

bradzacher added awaiting response Issues waiting for a reply from the OP or another party bug Something isn't working labels Aug 4, 2020

bradzacher changed the title ~~[return-await] Added test including 'throw' in try block~~ fix(eslint-plugin): [return-await] don't error for in-try-catch if the return is in a catch without a finally Aug 4, 2020

soobing added 7 commits August 6, 2020 00:12

Revert "test: throw in try block"

befef9a

This reverts commit aba2f89.

test: in-try-catch return at catch without finally

fe32cd3

refactor: isFinallyBlock 구현

81601c0

refactor: inTryCatch -> inTry, inCatch

7392d56

refactor: 오타 수정

1dedc46

refactor: catch with no finally, then must not be awaited

6d4883d

test: finally

76ff37f

bradzacher removed the awaiting response Issues waiting for a reply from the OP or another party label Aug 25, 2020

soobing added 2 commits August 29, 2020 12:57

Merge branch 'master' into test/return-await

f555920

Merge branch 'master' into test/return-await

422a934

bradzacher requested changes Sep 6, 2020

View reviewed changes

bradzacher added the awaiting response Issues waiting for a reply from the OP or another party label Sep 6, 2020

soobing added 2 commits September 7, 2020 20:58

Merge branch 'master' into test/return-await

abec413

docs: update return-await

adfefbc

Merge branch 'master' into test/return-await

7724500

bradzacher removed the awaiting response Issues waiting for a reply from the OP or another party label Sep 8, 2020

Merge branch 'master' into test/return-await

c174b38

This comment has been minimized.

Sign in to view

Merge branch 'master' into test/return-await

ea4ef05

typo in docs

f848443

bradzacher approved these changes Sep 14, 2020

View reviewed changes

bradzacher merged commit efdd521 into typescript-eslint:master Sep 14, 2020

github-actions bot locked as resolved and limited conversation to collaborators Oct 15, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(eslint-plugin): [return-await] don't error for `in-try-catch` if the return is in a `catch` without a `finally` #2356

fix(eslint-plugin): [return-await] don't error for `in-try-catch` if the return is in a `catch` without a `finally` #2356

soobing commented Aug 4, 2020 •

edited by bradzacher

Loading

typescript-eslint bot commented Aug 4, 2020

codecov bot commented Aug 4, 2020 •

edited

Loading

bradzacher commented Aug 4, 2020

soobing commented Aug 5, 2020

soobing commented Aug 19, 2020 •

edited

Loading

soobing commented Aug 25, 2020

bradzacher left a comment

soobing commented Sep 7, 2020 •

edited

Loading

This comment has been minimized.

bradzacher left a comment

fix(eslint-plugin): [return-await] don't error for in-try-catch if the return is in a catch without a finally #2356

fix(eslint-plugin): [return-await] don't error for in-try-catch if the return is in a catch without a finally #2356

Conversation

soobing commented Aug 4, 2020 • edited by bradzacher Loading

typescript-eslint bot commented Aug 4, 2020

codecov bot commented Aug 4, 2020 • edited Loading

Codecov Report

bradzacher commented Aug 4, 2020

soobing commented Aug 5, 2020

soobing commented Aug 19, 2020 • edited Loading

soobing commented Aug 25, 2020

bradzacher left a comment

Choose a reason for hiding this comment

in-try-catch

soobing commented Sep 7, 2020 • edited Loading

This comment has been minimized.

bradzacher left a comment

Choose a reason for hiding this comment

fix(eslint-plugin): [return-await] don't error for `in-try-catch` if the return is in a `catch` without a `finally` #2356

fix(eslint-plugin): [return-await] don't error for `in-try-catch` if the return is in a `catch` without a `finally` #2356

soobing commented Aug 4, 2020 •

edited by bradzacher

Loading

codecov bot commented Aug 4, 2020 •

edited

Loading

soobing commented Aug 19, 2020 •

edited

Loading

`in-try-catch`

soobing commented Sep 7, 2020 •

edited

Loading