-
Notifications
You must be signed in to change notification settings - Fork 721
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
JSON format for leadership schedule #3687
JSON format for leadership schedule #3687
Conversation
7410f0a
to
243ebf1
Compare
Resolves #3678 |
68c8620
to
7631d02
Compare
Example output:
The testnet for the above test was created via: IntersectMBO/plutus-apps#364 |
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.
Nice. A couple comments.
import Data.Eq | ||
import Text.Show | ||
|
||
data OutputAs = OutputAsJson | OutputAsText deriving (Eq, Show) |
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 should probably be in Cardano.CLI.Shelley.Output
. However I'm also wondering if this is necessary as we use Maybe OutputFile
to determine if we are rendering as JSON to disk or if we are printing to stdout. Are there instances where we don't render as JSON to disk?
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 just had this idea that we could have --json-output=FILE
and if users really want it in stdout
, they can do this: --json-output=/dev/stdout
.
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.
Fair enough. How about modifying OutputFile
to encompass this?
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 thought about this and I think it's better to follow the existing convention.
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 have pushed the update.
readerOutputAs = do | ||
v <- Opt.str @Text | ||
case v of | ||
"json" -> return OutputAsJson |
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 would be better as a flag as we can take advantage of the Alternative
instance of Parser
((<|>)
) which would eliminate ambiguity (therefore eliminating any error case).
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 went with your suggestion after all.
7631d02
to
16e0db3
Compare
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.
LGTM! Just squash the commits 👍
bors merge |
Build succeeded: |
No description provided.