-
Notifications
You must be signed in to change notification settings - Fork 14
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
governance vote view: use --output-format
, like other commands, instead of --yaml
#521
Conversation
[ Opt.long "output-format" | ||
, Opt.metavar "STRING" | ||
, Opt.help $ mconcat | ||
[ "Optional transaction view output format. Accepted output formats are \"json\" " | ||
[ "Optional " <> kind <> " view output format. Accepted output formats are \"json\" " |
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.
you can use a comma instead of <>
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.
Ah true that, because of the leading mconcat
. Good catch 👍
Change done
s <- Opt.str @String | ||
case s of | ||
"json" -> pure ViewOutputFormatJson | ||
"yaml" -> pure ViewOutputFormatYaml | ||
_ -> | ||
fail $ mconcat | ||
[ "Invalid transaction view output format: " <> show s | ||
[ "Invalid " <> kind <> " output format: " <> show s |
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.
Same, you can use a comma.
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.
Change done 👍
@@ -1,11 +1,13 @@ | |||
Usage: cardano-cli conway governance vote view [--yaml] | |||
Usage: cardano-cli conway governance vote view [--output-format STRING] |
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.
A flag here would be nicer imo: --yaml | --json
. We can add flags if we ever want to include more format options. What does the parsing error look like for --output-format STRING
vs specifying a misspelled flag?
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.
Sounds good, I will do that in a follow-up PR rn, because it's a bigger change, because it affects multiple commands.
s <- Opt.str @String | ||
case s of | ||
"json" -> pure GovernanceActionViewOutputFormatJson | ||
"yaml" -> pure GovernanceActionViewOutputFormatYaml | ||
"json" -> pure ViewOutputFormatJson |
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.
If we use flags, we can avoid _
completely
cde6c71
to
23d330e
Compare
Note
I will squash before merging but wanted to keep the commits separate in case there are comments, to better control history for myself
Changelog
Context
Fixes #516
On the way, I shared some code in the parsers; because code duplication is what brought this issue in the first place I think.
How to trust this PR
Inspect the diff in golden files
Checklist