Skip to content
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

refactor: use derive interface of clap #110

Merged
merged 3 commits into from
Apr 6, 2024

Conversation

ErichDonGubler
Copy link
Contributor

@ErichDonGubler ErichDonGubler commented Apr 4, 2024

Hey there! 👋🏻 I figured this would be a nice way to make editing the CLI more approachable, since this only takes on 1 new dependency (heck 0.5). LMK what you think!

@ErichDonGubler ErichDonGubler force-pushed the derived-clap-cli branch 2 times, most recently from dd28e6f to a4d7c46 Compare April 4, 2024 02:00
src/main.rs Outdated Show resolved Hide resolved
src/main.rs Outdated Show resolved Hide resolved
#[macro_use]
extern crate slog;

use clap::ArgAction;
use clap::{CommandFactory, Parser as _};
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is commandfactory necessary here? most of the flag-only examples don't seem to require it

Copy link
Contributor Author

@ErichDonGubler ErichDonGubler Apr 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's so we can call Cli::command to feed a Command structure to clap_completion::generate below. If one removes the import, they get the following compiler error (assuming a compilation based on a4d7c46):

error[E0599]: no function or associated item named `command` found for struct `Cli` in the current scope
  --> src/main.rs:53:35
   |
12 | struct Cli {
   | ---------- function or associated item `command` not found for this struct
...
53 |         let mut args_clone = Cli::command();
   |                                   ^^^^^^^ function or associated item not found in `Cli`
   |
   = help: items from traits can only be used if the trait is in scope
help: the following trait is implemented but not in scope; perhaps add a `use` for it:
   |
1  + use clap::CommandFactory;
   |

For more information about this error, try `rustc --explain E0599`.
error: could not compile `git-absorb` (bin "git-absorb") due to 1 previous error

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I consider this resolved, but I'll leave this open for visibility.

@ErichDonGubler

This comment was marked as resolved.

@tummychow tummychow merged commit 3da0e7f into tummychow:master Apr 6, 2024
3 checks passed
@ErichDonGubler ErichDonGubler deleted the derived-clap-cli branch April 6, 2024 20:31
@kiprasmel
Copy link
Contributor

@ErichDonGubler i want to implement --foo and --no-foo. previously before this PR, i could simply get the last index of both args (if any index), and whichever index was bigger, that value of either --foo or --no-foo would prevail.

how would I implement this now?

@ErichDonGubler
Copy link
Contributor Author

@kiprasmel: It's still possible, though it might look a bit odd! I didn't realize I had affected you here. 😅

If you're determined to use the --{,no-}option scheme, you could something like clap's tests for specification on an overrides_with option, like in clap's tests upstream or this playground I made to demonstrate; in short:

#[derive(Debug, Eq, clap::Parser, PartialEq)]
struct Cli {
    #[clap(long)]
    foo: bool,
    #[arg(long, action = clap::ArgAction::SetTrue, overrides_with = "foo")]
    no_foo: (),
}

In case it's interesting, I've often found that I prefer using a doubly-optional ValueEnum with a variant that disables the flag, for forwards compatibility and expressiveness. I don't know if the suggestion is relevant to you (you're probably dealing with an actually boolean solution space), but you might find this an overall more clear alternative, depending on what you're implementing (playground link):

#[derive(Debug, Eq, clap::Parser, PartialEq)]
struct Cli {
    #[clap(long)]
    // One can remove the outer `Option` to force specification of the value on
    // the CLI.
    foo: Option<Option<Foo>>,
}

#[derive(Clone, Debug, Eq, PartialEq, clap::ValueEnum)]
enum Foo {
    Yes,
    No,
    Whatever,
}

@kiprasmel
Copy link
Contributor

kiprasmel commented Apr 11, 2024

@ErichDonGubler yes I thought about having the foo and no_foo as cli args, and resolve them into final value when passing in the config to run absorb.

but again, i want a new --foo or --no-foo to overwrite the previous one, thus need to access the index.

is there a way to simply access the argv strings and simply do it myself?
i.e. argv.filter(x includes foo, no foo).last == foo || == no foo

@tummychow
Copy link
Owner

for the record, having graceful precedence of --foo/--not-foo is low importance to me. definitely not important enough to write custom cli parsing code beyond what clap already does - in fact, i would even merge a change that only added --foo without implementing --no-foo at all. it's unfortunate that clap doesn't already support this (clap-rs/clap#815) but i care more about having as little cli parsing code as possible

@ErichDonGubler
Copy link
Contributor Author

@kiprasmel:

@ErichDonGubler yes I thought about having the foo and no_foo as cli args, and resolve them into final value when passing in the config to run absorb.

If you examine the cases in both the tests and the playground link, I believe even the question of precedence is handled for you; AFAICT, the last flag specified "wins". Am I not understanding your intent?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants