tummychow / git-absorb

git commit --fixup, but automatic
https://crates.io/crates/git-absorb
BSD 3-Clause "New" or "Revised" License
3.35k stars 59 forks source link

refactor: use `derive` interface of `clap` #110

Closed ErichDonGubler closed 4 months ago

ErichDonGubler commented 4 months ago

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 commented 4 months ago

I've autosquash'd the feedback commits I've linked to in earlier conversations, and force-pushed the "clean" resulting history.

kiprasmel commented 4 months ago

@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 commented 4 months ago

@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 commented 4 months ago

@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 commented 4 months ago

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 (https://github.com/clap-rs/clap/issues/815) but i care more about having as little cli parsing code as possible

ErichDonGubler commented 4 months ago

@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?