Skip to content

fix: consider promise.all() in await-async-utils #236

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

Merged
merged 1 commit into from
Oct 12, 2020

Conversation

gndelia
Copy link
Collaborator

@gndelia gndelia commented Oct 6, 2020

closes #227

After a while, trying to program a Lil' bit outside of my regular work again. Here I'm just fixing the referenced ticket. I also added a bunch of helpers to node-utils instead of checking the node's type directly in the rule

@gndelia gndelia added the bug Something isn't working label Oct 6, 2020
@gndelia gndelia self-assigned this Oct 6, 2020
Comment on lines +22 to +32
// verifies the CallExpression is Promise.all()
function isPromiseAll(node: TSESTree.CallExpression) {
return isMemberExpression(node.callee) && isIdentifier(node.callee.object) && node.callee.object.name === 'Promise' && isIdentifier(node.callee.property) && node.callee.property.name === 'all'
}

// verifies the node is part of an array used in a CallExpression
function isInPromiseAll(node: TSESTree.Node) {
const parent = node.parent
return isCallExpression(parent) && isArrayExpression(parent.parent) && isCallExpression(parent.parent.parent) && isPromiseAll(parent.parent.parent)
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I am not sure if these are worth moving elsewhere

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, they could be useful. We can move them when necessary tho.

@gndelia
Copy link
Collaborator Author

gndelia commented Oct 6, 2020

I thought the 100% coverage was for the rules, not for the utils 🤔 🧐

should I create a node-utils.test.ts? I don't think there's a realistic scenario to get coverage for isArrayExpression usage from the rule (I would have to call Promise.all() without args or without an array [?]).

Similar goes to isImportNamespaceSpecifier

Should we exclude files that are not under lib/rules of the 100% threshold?

@Belco90
Copy link
Member

Belco90 commented Oct 6, 2020

I thought the 100% coverage was for the rules, not for the utils 🤔 🧐

should I create a node-utils.test.ts? I don't think there's a realistic scenario to get coverage for isArrayExpression usage from the rule (I would have to call Promise.all() without args or without an array [?]).

Similar goes to isImportNamespaceSpecifier

Should we exclude files that are not under lib/rules of the 100% threshold?

I thought so! I was planning to add specific tests for those in v4, so I'd wait for add specific tests for them. I guess we can modify the threshold meanwhile.

@gndelia gndelia force-pushed the bug/await-async-utils-promise-all-false-positives branch from 8af7b73 to aca5271 Compare October 8, 2020 02:46
@gndelia
Copy link
Collaborator Author

gndelia commented Oct 8, 2020

threshold updated for node-utils

@gndelia gndelia requested a review from Belco90 October 8, 2020 03:00
@Belco90 Belco90 merged commit 6ce0140 into master Oct 12, 2020
@Belco90 Belco90 deleted the bug/await-async-utils-promise-all-false-positives branch October 12, 2020 18:12
@Belco90
Copy link
Member

Belco90 commented Oct 12, 2020

🎉 This PR is included in version 3.9.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[await-async-utils] false positive promise all
2 participants