-
Notifications
You must be signed in to change notification settings - Fork 16
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
ESLint rule for non-nested first await #104
Conversation
f9cce72
to
8fa98d6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't fully step through the detection logic. I'm confident in its correctness from the test cases but I don't know if there's a more performant implementation.
Will approve after test cases are well organized.
ccb1895
to
27b817d
Compare
@@ -90,17 +94,15 @@ module.exports = { | |||
if (!isAwaitAllowedInNode(parent)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The part above this looks unnecessarily opaque to me. Can it be simplified and/or explained (e.g., "we allow await …;
expression statements and identifierName = await …;
assignments, but not update-assignment like identifierName += await …;
or nesting in other structures such as array/object literals or argument lists")?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added a comment explaining this function's allowlist.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find the logic suggestions Richard made easier to follow. I'll approve after those are resolved.
valid: clearlyBalanced, | ||
// `no-nested-await` is not smart enough to ignore subtle code that is | ||
// actually balanced. We test that it fails these cases. | ||
invalid: [...clearlyUnbalanced, ...subtlyBalanced].map(example => ({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I appreciate the comment. Especially on subtlyBalanced
in the other file. What the comment here says may be obvious if the names were mapped to this "smart enough" aspect:
valid: clearlyBalanced, | |
// `no-nested-await` is not smart enough to ignore subtle code that is | |
// actually balanced. We test that it fails these cases. | |
invalid: [...clearlyUnbalanced, ...subtlyBalanced].map(example => ({ | |
valid: trueValid, | |
invalid: [...trueInvalid, ...falseInvalid].map(example => ({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like clearly*
vs subtly*
, but I agree that *Balanced
vs. *Unbalanced
should be *Valid
vs. *Invalid
.
6f83ac2
to
b85546b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉
Excited to see the overzealous warnings in agoric-sdk go away :)
This relaxes the restrictions imposed by
@jessie.js/no-nested-await
so that only the first await needs to be non-nested.Also add some unit tests to better track the rule in the future.