-
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
style(golang): Align CLI command fields with upstream recommendations #8678
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 doing this! As usual, my approval is based on addressing comments to your satisfaction.
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.
This file is copied directly from the upstream genaccounts.go. Would you please also make a PR against that file so that your changes benefit the Cosmos as a whole?
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.
This is a copy of another upstream file, testnet.go.
func NewCmdSubmitCoreEvalProposal() *cobra.Command { | ||
cmd := &cobra.Command{ | ||
Use: "swingset-core-eval [[permit.json] [code.js]]...", | ||
Use: "swingset-core-eval <permit.json code.js>...", |
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'm not sure of the correct placement of angle brackets to indicate that these are pairs of filenames. Your rendition seems okay, but I wish there was something even clearer.
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.
Approving - but only to avoid needing to come back to me. I second mfig's requests and have a self-contained request that I trust you to respond to.
Use: "deliver [json string]", | ||
Short: "deliver inbound messages", | ||
Args: cobra.ExactArgs(1), | ||
Use: "deliver {<messages JSON> | @- | @<file path>}", |
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 don't know what a "messages JSON" is, as opposed to just a "JSON". Explain what @-
does in the long description. Each command needs to either be self-contained or have an explicit reference to conventions that can be found somewhere else.
cf. https://pkg.go.dev/github.com/spf13/cobra#Command and https://github.com/spf13/cobra/blob/v1.8.0/site/content/user_guide.md#example Only optional arguments should be wrapped in square brackets, alternatives should be wrapped in curly braces, and `Long` strings should generally start similar to `Use`.
f7b96ae
to
0887645
Compare
style(golang): Align CLI command fields with upstream recommendations
Description
cf. https://pkg.go.dev/github.com/spf13/cobra#Command and https://github.com/spf13/cobra/blob/v1.8.0/site/content/user_guide.md#example
Only optional arguments should be wrapped in square brackets, alternatives should be wrapped in curly braces, and
Long
strings should generally start similar toUse
.Security Considerations
n/a
Scaling Considerations
n/a
Documentation Considerations
This improves self-documentation of CLI commands, reducing the need for external digging.
Testing Considerations
n/a
Upgrade Considerations
Deprecation of
agd testnet --v
may result in some noise, but no change in actual behavior.