-
Notifications
You must be signed in to change notification settings - Fork 31
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
Improve CLAP/CLI #47
Improve CLAP/CLI #47
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.
Still needs the changes from clap-rs/clap#1617 to work properly. This somehow still compiles even though it shouldn't because we're using a yaml file instead of declaring the cli in Rust.
Also, there are help
and version
arguments that are generated implicitly generated by clap.
These were previously overridden by the arguments with the same name in the yaml file, but something should probably be done about them if they're supposed to be subcommands now.
Nevermind, they're still arguments. Would be better if the builtin version and help command were used though.
A guy from clap said me the it's possible to use In reality, I would like to remove the banner from clap (ASCII art + description) and display it in the front of each calls but only if the option |
@CephalonRho I see that you requested a change, but don't worry we will completely redesign the CLI :) |
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 be fine to merge now.
I ported most of the CLI changes from split-cli but kept it in a single binary.
The tokens
, store
and cli
subcommands have been removed since they didn't have any functionality attached to them.
I also added the -no-banner
flag to hide the banner on startup and removed the author duplicates in cli.yml using anchors. Clap doesn't seem to have a better way to do this, but this is good enough.
Hi @CephalonRho, I really enjoy your commits, especially moving to For the banner, I prefer to display it in the usage message.
Another point, maybe we can show a link to https://docs.lucid-kv.store/ in |
I don't think it's possible to display the banner like that everywhere while still keeping the |
Hmm yes, I understand but is it possible to set the banner as clap name? it's a little bit tricky but maybe it can works. |
I'm not sure if specifically setting the app name to the banner is a good idea (subcommand names include the name of the app), but showing the banner is definitely possible. It's just impossible to consider the |
I think it's a bad idea too, but for me the name need to be optional, I don't understand why they [clap] do that like this. For the banner, it could be good to have it in the usage and keep the parameter Why do you think? |
Okay, the help message always shows the banner now and everything else still respects |
Don't worry version is good like this, I eat and review & merge! |
Hi,
Like @Slals mentionned, the CLAP integration is not really clean, and this PR plan to fix this situation.
cli.yml
PS: opened issue here: clap-rs/clap#1617
Thank you