lucid-kv / lucid

High performance and distributed KV store w/ REST API. 🦀
https://clintnetwork.gitbook.io/lucid/
MIT License
373 stars 31 forks source link

Improve CLAP/CLI #47

Closed imclint21 closed 4 years ago

imclint21 commented 4 years ago

Hi,

Like @Slals mentionned, the CLAP integration is not really clean, and this PR plan to fix this situation.

PS: opened issue here: https://github.com/clap-rs/clap/issues/1617

Thank you

imclint21 commented 4 years ago

A guy from clap said me the it's possible to use template!

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 show_banner is set to true

imclint21 commented 4 years ago

@CephalonRho I see that you requested a change, but don't worry we will completely redesign the CLI :)

imclint21 commented 4 years ago

Hi @CephalonRho,

I really enjoy your commits, especially moving to main.rs and also --config as global parameters!

For the banner, I prefer to display it in the usage message.

PS C:\Users\me\Projets\lucid> lucid
<< HERE
Lucid 0.1.2 << and remove this if it's possible
High performance and distributed KV store w/ REST API.

USAGE:
    lucid.exe [FLAGS] [OPTIONS] <SUBCOMMAND>

Another point, maybe we can show a link to https://docs.lucid-kv.store/ in help command.

shuni64 commented 4 years ago

I don't think it's possible to display the banner like that everywhere while still keeping the no-banner flag. The problem is that clap requires you to first define the whole CLI, including the template/name/version, and then matches the arguments. This means that the program can't know if the no-banner flag is present until any help or error message has already been written to the terminal.

imclint21 commented 4 years ago

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.

shuni64 commented 4 years ago

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 no-banner flag, so it will always be shown.

imclint21 commented 4 years ago

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 --no-banner for all commands, by this way, it could be possible to run the server without the banner, etc

Why do you think?

shuni64 commented 4 years ago

Okay, the help message always shows the banner now and everything else still respects --no-banner. The only place that doesn't contain the banner in some way is the version subcommand/argument, but it might get harder to parse the version if it contains a banner that can't be disabled so that's fine I think.

imclint21 commented 4 years ago

Don't worry version is good like this, I eat and review & merge!

imclint21 commented 4 years ago