-
Notifications
You must be signed in to change notification settings - Fork 15
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
Options: avoid code duplication #890
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.
Nice work! There are some HLint warnings though.
<$> File | ||
<$> parseFilePath | ||
"calculate-plutus-script-cost" | ||
"(File () Out) filepath of the script cost information." |
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 know that it was copied from before, but I'm not sure about this haskell bit in the documentation: (File () Out)
. Seems odd, and not really informative for people not familiar with cardano-cli codebase.
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.
Good point, I changed the message 👍
, Opt.help help' | ||
, Opt.completer (Opt.bashCompleter "file") | ||
] | ||
fmap File $ parseFilePath name help' |
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, I didn't know we had this function. I guess we should move it from cardano-api to cardano-cli and don't depend on optparse-applicative in cardano-api.
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.
Good point, I'm looking whether we can do that easily 👍
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.
Follow-up PRs: IntersectMBO/cardano-api#635 and #894
dad25dc
to
108c1c2
Compare
Changelog
Context
Improvement found when starting to write https://github.com/input-output-hk/cardano-node-wiki/pull/52/files
How to trust this PR
You can review the diff in Haskell files manually.
This question is more tricky for the 700+ golden files that have changed 😄 What I did is that I ran the golden tests against a modified version of cardano-api, at revision https://github.com/IntersectMBO/cardano-api/tree/bf84187ae986f9a2621c55b690638fe8bb2cb156. I changed this line of parseFilePath so that it used
FILE
as the metavariable name. This way the diff is greatly reduced, becauseFILE
was the existing metavariable used so far most of the time. I then reviewed the golden files changes manually and everything was good.Checklist