-
Notifications
You must be signed in to change notification settings - Fork 12
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
Updated CLI Argument Parsing (New) #13
base: master
Are you sure you want to change the base?
Conversation
Transitioned from crate `argh` to `clap`. Created identical argument-parsing framework, with some additional elements, such as short and long flags for all items, instead of the previous only short flags. Currently `cargo check` passes with no warnings and the help page that is printed is fairly pretty.
Implemented the derive-attributes pattern for the config struct, moving away from the builder pattern. Almost identical to the previous version except the unkillable list of globs is not split by the tilde delimeter character. That will likely have to be implemented via the validator attribute, I just haven't done it yet.
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 awesome to me, what do you think @vrmiguel and @marcospb19 ?
Also, not sure where else to post this but I wanted to let you know I created an arch linux package for |
Awesome! Thanks for shipping The only drawback of using
All binaries compiled in release profile, and stripped. New size (with this pr) is 210% of the previous one. I am not sure if the gains in ergonomy are more important, as I don't expect people to be invoking Note: I also tried compiling with
Here the it drops from 210% to 186%, which is a bit better. By running the following script:
For both binaries, I compared the memory consumption. Current: 3040K With Current: 3036K Final decision of merging is up to @vrmiguel, but I know that he used |
Well I agree it does increase a lot if we look at percentage wise, however if you look at UX I believe it's the choice for this project to focus on. Sorry, but the final binary still under 1 MB, that's not a problem at all. Could you give any update here @vrmiguel ? |
Another thought too, I also added the |
Hey @JamesDevlin5! I'm very sorry for the catastrophic response to this PR 😢 I'm very glad you took time out to contribute to this project but a multitude of reasons made me take wayyy too long to consider if In any case, I'd argue that |
Re-implemented the argument parsing module, now using the nightly version of clap, and a derive-attribute pattern instead of the previous builder pattern, as suggested by @Xunjin in the previous pull request. The help page is colorized and very clean looking. Some small changes have yet to be fully implemented:
Other than that, everything looks pretty good!