-
Notifications
You must be signed in to change notification settings - Fork 13
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
Era-sensitive command structure #98
Conversation
fbd2250
to
73af3e4
Compare
73af3e4
to
eef24cf
Compare
| babbage | ||
| conway | ||
| latest | ||
| experimental |
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.
These are the new command groups.
Note that byron
is intentionally missing because there already is a legacy byron
command group.
latest
currently set to byron
and experimental
set to conway
.
runEraBasedGovernancePostConwayCmd | ||
:: ConwayEraOnwards era | ||
-> ExceptT () IO () | ||
runEraBasedGovernancePostConwayCmd _w = pure () |
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.
The pre and post conway run functions receive a witness which constrains which era the run function applies to.
|
||
pPostConwayCmd :: EnvCli -> CardanoEra era -> Maybe (Parser (EraBasedGovernanceCmd era)) | ||
pPostConwayCmd envCli = | ||
featureInEra Nothing $ \feature -> |
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.
Era witnesses are summoned from the era like this. feature
is the witness.
@@ -69,8 +69,8 @@ import Text.Read (readEither, readMaybe) | |||
parseLegacyCommands :: EnvCli -> Parser LegacyCommand | |||
parseLegacyCommands envCli = | |||
Opt.hsubparser $ mconcat | |||
[ Opt.metavar "Era based commands" | |||
, Opt.commandGroup "Era based commands" | |||
[ Opt.metavar "Legacy commands" |
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.
Perhaps instead of "Legacy commands" something like "Common commands"
36f33bb
to
1fbf7be
Compare
pPostConwayCmd envCli = | ||
featureInEra Nothing $ \feature -> | ||
Just | ||
$ subParser "post-conway" |
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.
So rather than all encompassing "pre/post-conway"
commands we should pattern match on the relevant ShelleyToBabbage era
/ ConwayOnwards era
values and render the appropriate command name.
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.
Yeah, post-conway
is a place holder example. It should be replaced with a command.
, subParser "latest" | ||
$ Opt.info (AnyEraCommandOf BabbageEra <$> pEraBasedCommand envCli BabbageEra) | ||
$ Opt.progDesc "Latest era commands (Babbage)" | ||
, subParser "experimental" |
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 skeptical of having an experimental set of commands, it will add more churn for not that much benefit. We should just print something to stdout
if a command or group of commands is experimental.
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.
experimental
isn't strictly necessary because users wanting to test Conway can still use conway
. The cost of having experimental
isn't that high though since it is effectively equivalent to conway
.
1fbf7be
to
7281f4d
Compare
Changelog
Context
Adding top-level era command groups avoids some of the problems posed by era flags. That is
optparse-applicative
does not allow for any dependencies between two command line options, so it is not possible for the era flag to control the validity of other flags. To allow for the chosen era to control the validity (and presence) of flags so that flags that are not relevant for the era don't appear and are invalid, the eras are promoted to top-level command groups.This is what the help looks like now:
And inside
governance
sub-command forbabbage
:And inside
governance
sub-command forconway
:The latter two examples show how command availability can switch on era.
Checklist
See Runnings tests for more details
.cabal
files are updatedhlint
. See.github/workflows/check-hlint.yml
to get thehlint
versionstylish-haskell
. See.github/workflows/stylish-haskell.yml
to get thestylish-haskell
versionghc-8.10.7
andghc-9.2.7
Note on CI
If your PR is from a fork, the necessary CI jobs won't trigger automatically for security reasons.
You will need to get someone with write privileges. Please contact IOG node developers to do this
for you.