Skip to content
This repository has been archived by the owner on Mar 25, 2021. It is now read-only.

[New Rule] add noAsyncWithoutAwait rule #3945

Merged
merged 15 commits into from
Jun 16, 2019
Merged

[New Rule] add noAsyncWithoutAwait rule #3945

merged 15 commits into from
Jun 16, 2019

Conversation

eranshabi
Copy link
Contributor

@eranshabi eranshabi commented Jun 6, 2018 •
edited by JoshuaKGoldberg
Loading

PR checklist

Overview of change:

Add a new rule ("noAsyncWithoutAwait"). The rule makes sure that a function will not be an async function, unless it has an await;

Is there anything you'd like reviewers to focus on?

CHANGELOG.md entry:

[new-rule] no-async-without-await

giladgray
giladgray suggested changes Jun 14, 2018
View reviewed changes
src/rules/code-examples/noAsyncWithoutAwait.examples.ts Outdated Show resolved Hide resolved
src/rules/noAsyncWithoutAwaitRule.ts Outdated Show resolved Hide resolved
src/rules/noAsyncWithoutAwaitRule.ts Outdated Show resolved Hide resolved
src/rules/noAsyncWithoutAwaitRule.ts Outdated Show resolved Hide resolved
src/rules/noAsyncWithoutAwaitRule.ts Outdated Show resolved Hide resolved
src/runner.ts Outdated Show resolved Hide resolved
@giladgray
Copy link

@eranshabi please merge latest master, i believe we've resolved the test failures.

giladgray
giladgray suggested changes Jun 18, 2018
View reviewed changes
src/runner.ts Outdated Show resolved Hide resolved
@eranshabi
Copy link
Contributor Author

eranshabi commented Jul 6, 2018 •
edited
Loading

I fixed the rule's support of implicit promise return statements.
Thank you for your feedback.
@giladgray is there anything else needed for this PR to be merged?

@eranshabi
Copy link
Contributor Author

All builds are green now

@eranshabi
Copy link
Contributor Author

@giladgray @jkillian @suchanlee anything else needed for merge?
thanks

@adidahiya adidahiya closed this Aug 16, 2018
@adidahiya adidahiya reopened this Aug 16, 2018
@bjornstar
Copy link
Contributor

We'd really like to add this rule to our project, is there any chance someone can review this?

JoshuaKGoldberg
JoshuaKGoldberg suggested changes Nov 7, 2018
View reviewed changes
Copy link
Contributor

@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.

Functionality looks good! Waiting on a few touchups of the implementation details and some more rationale details.

src/rules/noAsyncWithoutAwaitRule.ts Outdated
optionExamples: [true],
options: null,
optionsDescription: "Not configurable.",
rationale: "Cleaner code, can possibly reduce transpiled output",
Copy link
Contributor

Choose a reason for hiding this comment

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

Please explain this in more detail. Someone reading this rule's page on palantir.github.io would leave with more questions than they started with.

src/rules/noAsyncWithoutAwaitRule.ts Outdated Show resolved Hide resolved
src/rules/noAsyncWithoutAwaitRule.ts Outdated Show resolved Hide resolved
src/rules/noAsyncWithoutAwaitRule.ts Outdated
if (this.isAsyncFunction(node)
&& !this.functionBlockHasAwait(node.body as ts.Node)
&& !this.functionBlockHasReturn(node.body as ts.Node)
&& !this.isShortArrowReturn(node)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Performance nit: please move isShortArrowReturn first. Seems like we could be slightly faster by performing that very fast check before doing deeper searches.

@eranshabi
Copy link
Contributor Author

Thanks for the feedback @JoshuaKGoldberg, fixed by latest commit.

@eranshabi
Copy link
Contributor Author

Hey @JoshuaKGoldberg is anything else needed before merging?

JoshuaKGoldberg
JoshuaKGoldberg suggested changes Dec 14, 2018
View reviewed changes
Copy link
Contributor

@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.

Took a deeper look - some things to fix up.

So sorry for the delay @eranshabi! I had a pending review started then dropped it ☹

src/rules/noAsyncWithoutAwaitRule.ts Outdated Show resolved Hide resolved
src/rules/noAsyncWithoutAwaitRule.ts Outdated Show resolved Hide resolved
src/rules/noAsyncWithoutAwaitRule.ts Outdated Show resolved Hide resolved
src/rules/noAsyncWithoutAwaitRule.ts Outdated Show resolved Hide resolved
src/rules/noAsyncWithoutAwaitRule.ts Outdated Show resolved Hide resolved
src/rules/noAsyncWithoutAwaitRule.ts Outdated Show resolved Hide resolved
src/rules/noAsyncWithoutAwaitRule.ts Outdated Show resolved Hide resolved
src/rules/noAsyncWithoutAwaitRule.ts Outdated Show resolved Hide resolved
test/rules/no-async-without-await/test.ts.lint Show resolved Hide resolved
@eranshabi
Copy link
Contributor Author

Thanks for the review @JoshuaKGoldberg, I pushed a fix for everything.

@eranshabi
Copy link
Contributor Author

anything else needed for merge? @JoshuaKGoldberg @giladgray

JoshuaKGoldberg
JoshuaKGoldberg suggested changes Jan 2, 2019
View reviewed changes
src/rules/code-examples/noAsyncWithoutAwait.examples.ts Outdated Show resolved Hide resolved
src/rules/code-examples/noAsyncWithoutAwait.examples.ts Show resolved Hide resolved
src/rules/noAsyncWithoutAwaitRule.ts Outdated Show resolved Hide resolved
src/rules/noAsyncWithoutAwaitRule.ts Outdated Show resolved Hide resolved
src/rules/noAsyncWithoutAwaitRule.ts Outdated Show resolved Hide resolved
src/rules/noAsyncWithoutAwaitRule.ts Outdated Show resolved Hide resolved
src/rules/noAsyncWithoutAwaitRule.ts Outdated Show resolved Hide resolved
@JoshuaKGoldberg
Copy link
Contributor

JoshuaKGoldberg commented Jan 2, 2019 •
edited
Loading

@eranshabi I took another structural look and posted some feedback. Once all mine are addressed I think we should still wait for a palantirtech maintainer to take a look, since this is a slightly tricky rule It's been long enough that I feel comfortable merging this in.

Edit: it's been a bit since the original review so apologies in advance if some of the feedback is duplicate 😊

@eranshabi eranshabi mentioned this pull request Mar 26, 2019
Closed
@JoshuaKGoldberg
Copy link
Contributor

All right, I cleaned up the merge conflicts and pending PR comments, then refactored this to be a walker function instead of the deprecated class-based approach. It's good to go!

FYI @eranshabi - this'll be in the next TSLint release. Thanks so much for sending this PR in... about a year ago! Sorry it took so long to merge. 😄😢

@JoshuaKGoldberg JoshuaKGoldberg merged commit 0807692 into palantir:master Jun 16, 2019
@adidahiya adidahiya mentioned this pull request Jun 21, 2019
Merged
@snyk-bot snyk-bot mentioned this pull request Oct 11, 2019
Merged
@snyk-bot snyk-bot mentioned this pull request Oct 26, 2019
Open
@snyk-bot snyk-bot mentioned this pull request Nov 15, 2019
Open
@snyk-bot snyk-bot mentioned this pull request Dec 15, 2019
Open
@snyk-bot snyk-bot mentioned this pull request Feb 19, 2020
Merged
This was referenced Mar 18, 2020
Closed
Open
Open
Open
Closed
@snyk-bot snyk-bot mentioned this pull request Mar 31, 2020
Open
@snyk-bot snyk-bot mentioned this pull request Apr 12, 2020
Closed
@snyk-bot snyk-bot mentioned this pull request Apr 22, 2020
Open
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@giladgray giladgray giladgray requested changes

@JoshuaKGoldberg JoshuaKGoldberg JoshuaKGoldberg approved these changes

Assignees
No one assigned
Labels
PR: waiting for reviewer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

New Rule: no async without await
5 participants
@eranshabi @giladgray @bjornstar @JoshuaKGoldberg @adidahiya