Skip to content
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

refactor: pulling up more await weeds #6744

Merged
merged 2 commits into from
Jan 3, 2023
Merged

Conversation

erights
Copy link
Member

@erights erights commented Jan 3, 2023

Inspired by #6739 to return the favor.

If we keep pulling up the await weeds, we'll eventually be able to turn these warnings into errors.

Of the many PRs spawned off from #6219 and the not-yet-written ugly await safety rules at endojs/Jessie#93 , if we just fix 'em, we drop at least some of these kludgy and still-to-be-done workarounds. But this PR is only further incremental progress towards that goal. There are still way too many warnings.

I also refactored the asyncGenerate changes from #6739. None of them were interested in providing a value. All were only interested in the stopping condition. So instead @agoric/internal exports three convenience wrappers around asyncGenerate: forever, whileTrue, and untilTrue.

I suggest reviewing with "Hide whitespace" on.

@erights erights self-assigned this Jan 3, 2023
@@ -630,7 +637,7 @@ export default async function startMain(progname, rawArgs, powers, opts) {
SOLO_COINS,
],
];
for (const cmd of provCmds) {
for (/* await */ const cmd of provCmds) {
Copy link
Member Author

Choose a reason for hiding this comment

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

The await is commented out because
* turning it into a for-await-of loop would legitimize some await in its body
* both a for-await-of loop is not allowed here. This needs repair first.

packages/cosmic-swingset/src/sim-chain.js Outdated Show resolved Hide resolved
packages/vats/src/vat-bank.js Outdated Show resolved Hide resolved
packages/agoric-cli/src/install.js Outdated Show resolved Hide resolved
packages/agoric-cli/src/install.js Outdated Show resolved Hide resolved
packages/agoric-cli/src/install.js Outdated Show resolved Hide resolved
Copy link
Member

@michaelfig michaelfig left a comment

Choose a reason for hiding this comment

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

The following typing changes allow your caller to supply a function that returns an unspecified type, since you explicitly coerce it to a boolean with !boolFunc() or !!boolFunc().

Then your caller can relax and just use truthy/falsy values as expected.

packages/internal/src/utils.js Outdated Show resolved Hide resolved
packages/internal/src/utils.js Outdated Show resolved Hide resolved
Copy link
Member

@michaelfig michaelfig left a comment

Choose a reason for hiding this comment

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

LGTM! I'm happy to see this land as soon as the CI bots give the final approval.

@erights
Copy link
Member Author

erights commented Jan 3, 2023

This PR only reduces await warning from 259 to 230. Lots more weeding needed.

@erights erights added automerge:squash Automatically squash merge and removed automerge:squash Automatically squash merge labels Jan 3, 2023
@erights erights force-pushed the markm-driveby-await-cleanups branch from c52a653 to 453db21 Compare January 3, 2023 05:29
@erights erights added the automerge:squash Automatically squash merge label Jan 3, 2023
@mergify mergify bot merged commit 3ff8f54 into master Jan 3, 2023
@mergify mergify bot deleted the markm-driveby-await-cleanups branch January 3, 2023 05:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge:squash Automatically squash merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants