-
Notifications
You must be signed in to change notification settings - Fork 5
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
Some CLI improvements #54
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.
Looks good, thanks. Just had a small suggestion for code style.
clorinde/src/cli.rs
Outdated
cfg.sync = sync; | ||
cfg.r#async = r#async || !sync; | ||
cfg.serialize = serialize; | ||
if let Some(podman) = podman { |
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.
suggestion: instead of if let Some(...)
you could do cfg.podman = podman.unwrap_or(cfg.podman);
. But async might need something like cfg.r#async = r#async.map_or(cfg.r#async, |a| a || !cfg.sync);
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.
done! thank you for the quick feedback!
} | ||
|
||
#[derive(Debug, Subcommand)] | ||
enum Action { | ||
/// Generate your modules against your own db | ||
Live { | ||
#[clap(env = "DATABASE_URL")] |
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, didn't know about using env
with clap. This will be useful in the future.
The main problem I was facing with the CLI was that CLI default values were overwriting values from the config file. This happened because
cfg
variable was set by the config file, and the CLI values overwrote the values with default values.For example,
queries
path anddestination
path were keeping being overwritten to default CLI values even though I specified otherwise in the config file. This confused me for a long time.I solved this by making all CLI args optional and only overwriting the config values if they exist.
Second annoyance that I had was that all the cli args were required before the subcommand; this went against the usual CLI experience where I would write
command subcommand --arg1 val1 --arg2 val2
; instead, I have to docommand --arg1 val1 --arg2 val2 subcommand
.Solution for this is just feeding the args to the subcommand. This has added benefit of
clorinde live -h
showing all args descriptions.Also, I made the database url be fetchable by env var
DATABASE_URL
so I don't have to feed it to the cli everytime.