-
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
Add compatible command group #917
Conversation
c7f4302
to
903a192
Compare
903a192
to
8e4b6f4
Compare
8bb861d
to
65486f9
Compare
65486f9
to
5b37288
Compare
CompatibleTransactionCmd cmd -> renderCompatibleTransactionCmd cmd | ||
CompatibleGovernanceCmds cmd -> renderCompatibleGovernanceCmds cmd | ||
|
||
pAnyCompatibleCommand :: EnvCli -> Parser AnyCompatibleCommand |
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.
Parsers should go to cardano-cli/src/Cardano/CLI/Compatible/Options/Common.hs
or something similar
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 want to mirror the previous EraBased
directory structure. A Common
module is fine but this parser is not common i.e it's called in one place.
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 should be named
cardano-cli/src/Cardano/CLI/Compatible/Options/Governance.hs
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 as above, I don't want to mirror the previous directory structure.
( CompatibleTransactionCmds (..) | ||
, CompatibleTransactionError (..) | ||
, pAllCompatibleTransactionCommands | ||
, renderCompatibleTransactionCmd | ||
, runCompatibleTransactionCmd |
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 module should be split into Run, Option and Command modules
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.
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.
What's the plan with Eons and compatible commands? Are we planning to keep necessary eons for those commands?
command related types
5b37288
to
70c7079
Compare
Currently those eons will be intended for use only with our compatibility commands and we should add this to the haddocks in |
Related: #926
Changelog
Context
How to trust this PR
Highlight important bits of the PR that will make the review faster. If there are commands the reviewer can run to observe the new behavior, describe them.
Checklist