Skip to content

feat(require-returns): add forceReturnsWithAsync #277

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
Merged
merged 7 commits into from
Jun 16, 2019
Merged

feat(require-returns): add forceReturnsWithAsync #277

merged 7 commits into from
Jun 16, 2019

Conversation

@AndrewLeedham
Copy link
Contributor

@AndrewLeedham AndrewLeedham commented Jun 12, 2019

Async functions implicitly returning Promise<void> is now behind the new forceReturnsWithAsync (or forceRequireReturn) setting.

fixes: #276
@AndrewLeedham
feat(require-returns): add forceReturnsWithAsync
bc184a4 
Async functions implicitly returning Promise<void> is now behind the forceReturnsWithAsync (or forceRequireReturn) setting.
@AndrewLeedham AndrewLeedham mentioned this pull request Jun 12, 2019
Closed
@AndrewLeedham
Copy link
Contributor Author

AndrewLeedham commented Jun 13, 2019

@brettz9 Thoughts?
@brettz9
Copy link
Collaborator

brettz9 commented Jun 13, 2019

It looked good at my bird's eye view. When I may have a chance, I would like to have a closer look at the tests and the tests that were changed. Just those nits for the moment...
@brettz9 brettz9 force-pushed the master branch 2 times, most recently from 2eba18e to 46bc0c5 Compare June 14, 2019 10:56
@AndrewLeedham @brettz9
feat(require-returns): add forceReturnsWithAsync
7e4369b 
Async functions implicitly returning Promise<void> are now behind the forceReturnsWithAsync (or forceRequireReturn) setting.
@brettz9
Copy link
Collaborator

brettz9 commented Jun 14, 2019

I went ahead and fixed the nits and added back the tests that were removed. While some functionality may cover similar ground, I think it is better to be explicit with different tests, with forceRequireReturn and forceReturnsWithAsync being of course somewhat different. (And in the case of async arrow functions, we do want to cover that is added back along with the few others.)
@brettz9 brettz9 self-requested a review June 14, 2019 12:28
@chiawendt
Copy link
Collaborator

chiawendt commented Jun 15, 2019

Since forceReturnsWithAsync only affects this rule, it should be moved to options?
@AndrewLeedham
Copy link
Contributor Author

AndrewLeedham commented Jun 15, 2019

Since forceReturnsWithAsync only affects this rule, it should be moved to options?

Makes sense to me. Implemented.
brettz9
brettz9 approved these changes Jun 16, 2019
View reviewed changes
Copy link
Collaborator

@brettz9 brettz9 left a comment

Choose a reason for hiding this comment

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

Good idea re: adding as an option and all looks good except that I added back one test. I also added an additional test.
@brettz9 brettz9 requested a review from gajus June 16, 2019 07:02
chiawendt
chiawendt requested changes Jun 16, 2019
View reviewed changes
@brettz9
test: add back async function quux test with forceRequireReturn (…
71ba9f2 
…keeping the one with `forceReturnsWithAsync` as well) test: add passing test for `forceReturnsWithAsync` on non-async function without requires docs: grammar nit
@brettz9 brettz9 merged commit 71ba9f2 into gajus:master Jun 16, 2019
@gajus
Copy link
Owner

gajus commented Jun 16, 2019

🎉 This PR is included in version 8.1.0 🎉

The release is available on:

Your semantic-release bot 📦🚀
@gajus gajus added the released label Jun 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

released

4 participants

@AndrewLeedham @brettz9 @chiawendt @gajus