-
Notifications
You must be signed in to change notification settings - Fork 212
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
chore(deploy-script-support): Sync code patterns across files #8712
Conversation
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.
Thanks for making this code clearer! My only comments are around the (granted, preexisting) use of IIFEs where assigning names may make the intent easier to understand.
I'll leave it to your discretion whether or how to address that.
// Make an enactCoreProposals function and "export" it by way of script completion value. | ||
(( | ||
makeCoreProposalBehavior = ${makeCoreProposalBehavior}, | ||
makeEnactCoreProposals = ${makeEnactCoreProposals}, | ||
) => makeEnactCoreProposals({ metadataRecords, E }))(); |
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.
Wow! I found this really tricky to understand, but eventually puzzled through it... I take it the IFFE default arguments are used just to create local bindings?
I'm not sure that's as clear as creating them at the top level. Would you be able to write a very clear explanation of what these lines are trying to do?
If it's too hard to render in English, I'd prefer something like:
// Make an enactCoreProposals function and "export" it by way of script completion value. | |
(( | |
makeCoreProposalBehavior = ${makeCoreProposalBehavior}, | |
makeEnactCoreProposals = ${makeEnactCoreProposals}, | |
) => makeEnactCoreProposals({ metadataRecords, E }))(); | |
const makeCoreProposalBehavior = ${makeCoreProposalBehavior}; | |
const makeEnactCoreProposals = ${makeEnactCoreProposals}; | |
// Make an enactCoreProposals function and "export" it by way of script completion value. | |
const enactCoreProposals = makeEnactCoreProposals({ metadataRecords, E }); | |
enactCoreProposals; |
// Make a behavior function and "export" it by way of script completion value. | ||
const behavior = (${makeCoreProposalBehavior})({ manifestBundleRef, getManifestCall, customManifest, E }); | ||
behavior; |
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.
Consider this for clarity, as well:
// Make a behavior function and "export" it by way of script completion value. | |
const behavior = (${makeCoreProposalBehavior})({ manifestBundleRef, getManifestCall, customManifest, E }); | |
behavior; | |
const makeCoreProposalBehavior = ${makeCoreProposalBehavior}; | |
// Make a behavior function and "export" it by way of script completion value. | |
const behavior = makeCoreProposalBehavior({ manifestBundleRef, getManifestCall, customManifest, E }); | |
behavior; |
aab6e09
to
1965715
Compare
…rs to "custom" (the former can be read like a verb)
…in only when necessary
1965715
to
2992afa
Compare
…salbehavior chore(deploy-script-support): Sync code patterns across files
…salbehavior chore(deploy-script-support): Sync code patterns across files
…salbehavior chore(deploy-script-support): Sync code patterns across files
…salbehavior chore(deploy-script-support): Sync code patterns across files
…salbehavior chore(deploy-script-support): Sync code patterns across files
…salbehavior chore(deploy-script-support): Sync code patterns across files
Description
assert(cond, X`...`)
calls tocond || Fail`...`
best reviewed commit-by-commit
Security Considerations
This should be a pure refactor, and even preserves lack of a
makeEnactCoreProposals
binding in the script code generated byextractCoreProposalBundles
.Scaling Considerations
n/a
Documentation Considerations
n/a
Testing Considerations
No new tests should be required.
Upgrade Considerations
I don't think anything outside of agoric-sdk relies upon the parameter field names (which should be self-contained in generated script), but if so then we'll need to update or accommodate them.