-
Notifications
You must be signed in to change notification settings - Fork 55
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
CLI upgrade for more control and less duplication #691
Conversation
e531b70
to
d79e6bc
Compare
Update Cleaner code Making progress Fixing parsing Fixing up options Update More sane Better header/footer Much cleaner Fixing address Cleaner parsing Factor out Cleaner Fixing build Fixing capitalization Updating to fix assertions parsing Improve text Cleanup
Fixing cli tests
c970f6a
to
1e54ddf
Compare
Fixing errors in cabal file Update authors, too Much cleaner test code Cleaner Update Run tests, update 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.
Looks pretty cool!
I have not spotted any problem. I will try to play with the CLI as well :)
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.
Looks good! Can you check if these new package dependencies should really be dependencies for the library target?
Description
We are now using
Options.Applicative
for CLI. This allows us to have a lot more control, e.g. separate footers for each subcommand. It also allows us to skip having repeated code that was becoming pretty unwieldy. The common commands are now much more visible/reasonable to edit and improve.I also added some very simple test cases for the CLI, just to make sure I didn't break some basic CLI functionality. We probably should extend this test more for some basic checks to make sure new options work.
Notes:
solver
inUnitTestOptions
was completely unused (oops!). Removed.functionAbi f
function is actually not very user-friendly. When I give a signature of--sig "stuff"
it gives aninternalError
, because it's missing the()
. It's now a little bit better, but it should be rewritten to return anEither
and give better feedback to the user.cli-test
default-language
everywhere in the cabal file. This seems to be needed now, with the newcli-test
depending onhevm
. Weird, but otherwisecabal check
complains. This will be needed anyway in the future when we upgrade to newer cabal.Checklist