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

Feature: Custom Derive for ArgEnum #1659

Closed
Tracked by #1661
kbknapp opened this issue Nov 12, 2017 · 15 comments · Fixed by #1849
Closed
Tracked by #1661

Feature: Custom Derive for ArgEnum #1659

kbknapp opened this issue Nov 12, 2017 · 15 comments · Fixed by #1849
Labels
A-derive Area: #[derive]` macro API
Milestone

Comments

@kbknapp
Copy link
Member

kbknapp commented Nov 12, 2017

From @Hoverbear on November 12, 2017 18:11

Hey!

I had a bit of time, and a hankering to write a custom derive, so I got working on a custom derive for ArgEnum.

I've structured the crate so if we want to expand it, it should be easy. I think this is related to #835 but I didn't look too deeply into what the progress is with that.

The crate is https://github.com/Hoverbear/clap-derives and unpublished, and I'd be more than happy to transfer you (@kbknapp) ownership. It's a first draft and could probably use refinement.

You can see an example here:

#[macro_use]
extern crate clap;
#[macro_use]
extern crate clap_derive;

use clap::{App, Arg};

#[derive(ArgEnum, Debug)]
enum ArgChoice {
    Foo,
    Bar,
    Baz,
}

fn main() {
    let matches = App::new(env!("CARGO_PKG_NAME"))
            .arg(Arg::with_name("arg")
                .required(true)
                .takes_value(true)
                .possible_values(&ArgChoice::variants())
            ).get_matches();
    
    let t = value_t!(matches.value_of("arg"), ArgChoice)
        .unwrap_or_else(|e| e.exit());

    println!("{:?}", t);
}

Will derive this:

impl ::std::str::FromStr for ArgChoice {
    type Err = String;
    fn from_str(input: &str) -> ::std::result::Result<Self, Self::Err> {
        match input {
            val if ::std::ascii::AsciiExt::eq_ignore_ascii_case(val, "Foo") => Ok(ArgChoice::Foo),
            val if ::std::ascii::AsciiExt::eq_ignore_ascii_case(val, "Bar") => Ok(ArgChoice::Bar),
            val if ::std::ascii::AsciiExt::eq_ignore_ascii_case(val, "Baz") => Ok(ArgChoice::Baz),
            _ => Err({
                let v = ["Foo", "Bar", "Baz"];



                ::fmt::format(::std::fmt::Arguments::new_v1_formatted(
                    &["valid values: "],
                    &match (&v.join(" ,"),) {
                        (__arg0,) => {
                            [
                                ::std::fmt::ArgumentV1::new(__arg0, ::std::fmt::Display::fmt),
                            ]
                        }
                    },
                    &[
                        ::std::fmt::rt::v1::Argument {
                            position: ::std::fmt::rt::v1::Position::At(0usize),
                            format: ::std::fmt::rt::v1::FormatSpec {
                                fill: ' ',
                                align: ::std::fmt::rt::v1::Alignment::Unknown,
                                flags: 0u32,
                                precision: ::std::fmt::rt::v1::Count::Implied,
                                width: ::std::fmt::rt::v1::Count::Implied,
                            },
                        },
                    ],
                ))
            }),
        }
    }
}
impl ArgChoice {
    fn variants() -> [&'static str; 3usize] {
        ["Foo", "Bar", "Baz"]
    }
}

Copied from original issue: #1103

@kbknapp
Copy link
Member Author

kbknapp commented Nov 12, 2017

Hey!

cc @TeXitoi

Excellent! This is a great start towards my ideas for 3.x. I've cc'd @TeXitoi because I'd like to re-invigorate the move towards 3.x and finishing the CustomDerive piece is a big part of that movement. With my job monopolizing my time this kind of fell off in #835 but now that the v3-master branch is coming together it's worth working towards again.

The grand plan is to create a clap_derive crate which contains #[derive(*)] directives:

  • ArgEnum as you've done 😍
  • IntoApp which takes an arbitrary struct and creates a clap::App (currently implemented in structopt which I'd like to pull into this clap_derive crate)
  • FromArgMatches which takes a clap::ArgMatches struct and creates the arbitrary struct (also implemented in structopt)
  • ClapApp which is a convenience marking and simply does both IntoApp and FromArgMatches
  • ClapKey (name?...maybe ArgKey?) which allows using an enum instead of strings to access clap::Args, clap::Apps (formerly clap::SubCommands in v2.x), and clap::ArgGroups.

I was tentatively planning on storing all the clap_* crates (I'm also moving the shell completions to clap_completions and macros to clap_macros) at the root of the clap repo, but individual repos are OK as well, although I'd like everything to versioned in lock-step which is harder in individual repos.

If both @Hoverbear and @TeXitoi think the design is sound/doable I'd like to make PRs against the v3-master branch, or if keeping individual repos is better, making PRs against the clap_derive crate. If that's the route we go, I would like to transfer the crate to me in order to provide less confusion to the community as this would be part of the "official" clap crates, however I would also like to keep/add @Hoverbear and @TeXitoi as authors in the Cargo.toml because they the ones deserving of it. Also at that point, both would become collaborators on this repo so they could continue to work on this even when I'm busy with my day job.

@kbknapp
Copy link
Member Author

kbknapp commented Nov 12, 2017

Here are the unknowns still, and what needs to be decided

Supporting Subcommands via CustomDerive

We need to find out how to support a struct with arbitrary options/flags and also a subcommand, and how that gets represented via CustomDerive. I believe @TeXitoi suggested an enum, where each variant holds it's arbitrary struct representation. I'm good with this, and we just need to work towards ironing out the details and actually implementing. I believe this would also require adding another #[derive(*)] directive such as ClapSubCommand for enums which are to represent a subcommand.

The hard part is how does this work ergonomically.

Some code examples. I want the end user result to be as close as possible to to this:

#[derive(ClapApp)]
struct MyApp {
    flag: bool,
    subcommand: MySubCommand
}

#[derive(ClapSubCommand)]
enum MySubCommand {
    Foo(FooCommand),
    Bar(BarCommand),
}

#[derive(ClapApp)]
struct FooCommand {
    verbose: bool
}

#[derive(ClapApp)]
struct BarCommand {
    name: String
}

fn main() {
    // run with `$ myapp bar kevin`
    let app = MyApp::parse();
    match app.subcommand {
        FooCommand(foo) => println!("Verbose used?: {:?}", foo.verbose),
        BarCommand(bar) => println!("Hello {}!", bar.name),
        _ => println!("No subcommand used."),
    }
}

Supporting ASCII Case Insensitive ArgEnums

As @Hoverbear and I discussed briefly at RustBeltRust, we need these ArgEnums to be case insensitive, but I'd also like an opt-in version that is case sensitive (perhaps via a different #[derive(*)] directive...ArgEnumCaseSenseitive? To incentivise case insensitivity due to typing length?

Supporting Hyphens - in ArgEnums

There's also discussion in #1098 about if hyphens should be ignored or allowed, etc. I'm inclined to either allow hyphens (i.e. ignore them) or require special chars such as underscore (_) to allow them. Bottom line is there should be a way for people to use hyphens in their ArgEnums

@kbknapp
Copy link
Member Author

kbknapp commented Nov 12, 2017

From @Hoverbear on November 12, 2017 19:28

@kbknapp Please be aware custom derive crates must only contain custom derives. So we'd need a clap_derive crate with all of them in it separate from clap.

@kbknapp
Copy link
Member Author

kbknapp commented Nov 12, 2017

From @Hoverbear on November 12, 2017 19:29

For case sensitivity around ArgEnum we could have an attribute that is #[case_insensitive], for example.

@kbknapp
Copy link
Member Author

kbknapp commented Nov 12, 2017

From @Hoverbear on November 12, 2017 19:33

@kbknapp You should have a transfer request in your inbox.

@kbknapp
Copy link
Member Author

kbknapp commented Nov 12, 2017

Please be aware custom derive crates must only contain custom derives. So we'd need a clap_derive crate with all of them in it separate from clap

Ah ok I wasn't aware of this having not done much with CustomDerive. OK separate repos it is, I'm good with it 😄 👍

@Hoverbear
Copy link

Hoverbear commented Nov 12, 2017

clap-rs/clap_derive#4 doesn't specify this much. Is there any remaining feature this should handle? I think expanding the examples would be a good idea as well.

  • Expand examples.

@TeXitoi
Copy link
Contributor

TeXitoi commented Nov 13, 2017

Just to comment about 2 repos: derive crates need to be separate, but you can put several crates in a repo. And such a repo will not be independent from the rest, we need a normal crate to define the traits that will be implemented by the derive crate. (maybe not for ArgEnum as it implement FromStr)

@TeXitoi
Copy link
Contributor

TeXitoi commented Nov 13, 2017

About structopt, it is feature-full for me since about a week thanks to external contributors. It now support subcommands, calling any App and Arg methods, custom converters and OsStr support.

I will not have time to contribute by porting structopt here, but I'm available to review everything about that and giving my opinion.

@kbknapp if you can check the current status of structopt. I'm very interested by your feedbacks about the interface. If you like or dislike some syntax, and if you see any critical lacking feature.

Note that I don't think having 2 derives for IntoApp and FromArgMatcher is really needed as the attributes are needed in the 2 sides.

Maybe we can open issues to talk about all that.

@kbknapp
Copy link
Member Author

kbknapp commented Nov 13, 2017

@TeXitoi no worries, I'm low on time over the next few weeks as well due to holidays and travel. I'll gladly take a look once I get some time 😄

I'll do some porting too once I get a little extra time. So that it's clear too, I intend to use as much code from structopt as possible without many changes unless I happen to see something that is directly contradictory to how clap does it...which doubt I'll find 😜

I'd also like to maintain all commit history so that @TeXitoi doesn't lose any commits. (Just FYI in case someone else starts doing some porting, I don't want to simply copy-paste and erase all commit history 😉)

@Hoverbear
Copy link

@kbknapp Maybe we could merge this repo into that one instead of the other way around?

@pksunkara pksunkara transferred this issue from clap-rs/clap_derive Feb 3, 2020
@pksunkara
Copy link
Member

@CreepySkeleton This is possibly related to multiple traits.

@pksunkara pksunkara added this to the 3.0 milestone Feb 3, 2020
@pksunkara pksunkara added A-derive Area: #[derive]` macro API W: 3.x labels Feb 3, 2020
@CreepySkeleton
Copy link
Contributor

Yeah, kind of, but it can wait until beta. The nearest milestone is alpha, just to show people the project is still alive / being resurrected.

I'm getting custom derives to work, anticipated time is ~5 hours from now (I'll get to it in ~3 hours since I have some urgent stuff to do now, sorry for delay). Then we can get rid of no_version and start working on multiple traits approach. I drew a sketch here

@pksunkara pksunkara removed the W: 3.x label Feb 3, 2020
@kbknapp kbknapp mentioned this issue Feb 3, 2020
27 tasks
@pksunkara
Copy link
Member

Some implementation is at clap-rs/clap_derive#15

@pksunkara pksunkara linked a pull request Apr 22, 2020 that will close this issue
@bors bors bot closed this as completed in #1849 Apr 22, 2020
@pksunkara
Copy link
Member

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-derive Area: #[derive]` macro API
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants