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

Allow subcmd name to be something else than a literal in clap derive #2751

Merged
merged 1 commit into from
Sep 7, 2021

Conversation

co42
Copy link
Contributor

@co42 co42 commented Sep 3, 2021

Modify clap_derive to allow to compute sub command name dynamically (via a function for example):

    fn get_name() -> String {
        if runtime_check() {
          "some-name".to_string()
        } else {
          "some-other-name".to_string()
        }
    }

    #[derive(Clap, PartialEq, Debug)]
    struct Opt {
        #[clap(subcommand)]
        subcmd: SubCmd,
    }

    #[derive(Clap, PartialEq, Debug)]
    enum SubCmd {
        #[clap(name = get_name())]
        SubCmd1,
    }

It was expected to be a literal and was used in match, I replaced it with if statements.

@co42 co42 force-pushed the derive_subcmd_name_not_literal branch from 1f46baa to 81ace33 Compare September 3, 2021 16:11
@epage
Copy link
Member

epage commented Sep 3, 2021

Could you clarify the use case for this? If nothing else, it would help in understanding what use cases to keep in mind for future work.

Also, mind posting a cargo expand output so we can more easily review whats happening

@co42
Copy link
Contributor Author

co42 commented Sep 3, 2021

Use case :

I want to change the way a sub command (named data) work in my CLI, but I want to keep the CLI compatible until the new version is adopted by most users.

The idea is to ask the user to opt-in (or not) for the new version of the sub command.
Both versions are kept but their names depend on the user choice.

    #[derive(Clap)]
    enum SubCommand {
        #[clap(name = get_old_data_name())]
        OldData,
        #[clap(name = get_new_data_name())]
        NewData,
    }

   fn get_old_data_name() -> &'static str {
    if opt_in {
      "old-data"
    } else {
      "data"
    }
  }

   fn get_new_data_name() -> &'static str {
    if opt_in {
      "data"
    } else {
      "new-data"
    }
  }

I'll do a cargo expand

@co42
Copy link
Contributor Author

co42 commented Sep 3, 2021

A cargo expand like you asked.
The main difference is that we can't match on the name of the sub command since it may not be a literal.

Code:

    fn get_name() -> String {
        "renamed".to_string()
    }

    #[derive(Clap, PartialEq, Debug)]
    struct Opt {
        #[clap(subcommand)]
        subcmd: SubCmd,
    }

    #[derive(Clap, PartialEq, Debug)]
    enum SubCmd {
        #[clap(name = get_name())]
        SubCmd1,
        #[clap(name = "sub2")]
        SubCmd2,
    }

Before:

    impl clap::FromArgMatches for SubCmd {
        fn from_arg_matches(arg_matches: &clap::ArgMatches) -> Option<Self> {
            match arg_matches.subcommand() {
                Some((get_name(), arg_matches)) => Some(SubCmd::SubCmd1),
                Some(("sub2", arg_matches)) => Some(SubCmd::SubCmd2),
                ::std::option::Option::Some((other, sub_arg_matches)) => None,
                ::std::option::Option::None => ::std::option::Option::None,
            }
        }
        fn update_from_arg_matches<'b>(&mut self, arg_matches: &clap::ArgMatches) {
            if let Some((name, sub_arg_matches)) = arg_matches.subcommand() {
                match (name, self) {
                    (get_name(), SubCmd::SubCmd1) => {
                        let arg_matches = sub_arg_matches;
                        {}
                    }
                    ("sub2", SubCmd::SubCmd2) => {
                        let arg_matches = sub_arg_matches;
                        {}
                    }
                    (other_name, s) => {
                        if let Some(sub) =
                            <Self as clap::FromArgMatches>::from_arg_matches(arg_matches)
                        {
                            *s = sub;
                        }
                    }
                }
            }
        }
    }

    impl clap::Subcommand for SubCmd {
        ...
        fn has_subcommand(name: &str) -> bool {
            match name {
                get_name() => true,
                "sub2" => true,
                _ => false,
            }
        }
    }

After:

    impl clap::FromArgMatches for SubCmd {
        fn from_arg_matches(arg_matches: &clap::ArgMatches) -> Option<Self> {
            if let Some((name, sub_arg_matches)) = arg_matches.subcommand() {
                {
                    let arg_matches = sub_arg_matches;
                    if get_name() == name {
                        return Some(SubCmd::SubCmd1);
                    }
                    if "sub2" == name {
                        return Some(SubCmd::SubCmd2);
                    }
                }
                None
            } else {
                None
            }
        }
        fn update_from_arg_matches<'b>(&mut self, arg_matches: &clap::ArgMatches) {
            if let Some((name, sub_arg_matches)) = arg_matches.subcommand() {
                match self {
                    SubCmd::SubCmd1 if get_name() == name => {
                        let arg_matches = sub_arg_matches;
                        {}
                    }
                    SubCmd::SubCmd2 if "sub2" == name => {
                        let arg_matches = sub_arg_matches;
                        {}
                    }
                    s => {
                        if let Some(sub) =
                            <Self as clap::FromArgMatches>::from_arg_matches(arg_matches)
                        {
                            *s = sub;
                        }
                    }
                }
            }
        }
    }

    impl clap::Subcommand for SubCmd {
        ...
        fn has_subcommand(name: &str) -> bool {
            if get_name() == name {
                return true;
            }
            if "sub2" == name {
                return true;
            }
            false
        }
    }

Copy link
Member

@epage epage left a comment

Choose a reason for hiding this comment

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

Interesting use case and overall, looks good!

clap_derive/tests/subcommands.rs Outdated Show resolved Hide resolved
Copy link
Member

@pksunkara pksunkara left a comment

Choose a reason for hiding this comment

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

Other than @epage's comment, can't the use case be solved by compile time evaluation? As of 1.54 macros are allowed inside attributes

@co42 co42 force-pushed the derive_subcmd_name_not_literal branch from 81ace33 to 8df90fb Compare September 6, 2021 07:13
@co42
Copy link
Contributor Author

co42 commented Sep 6, 2021

@pksunkara I can't compute it at compile time, the name is computed based on an option stored in a config file, and this file can be modified by the user (if he want to opt in or not for this feature).

@epage
Copy link
Member

epage commented Sep 6, 2021

Yeah, having runtime feature flags for migrating functionality is a big help. I did it a lot at my last job. We even had a built-in system for it within our build tool where a flag would automatically map cmd subcmd to cmd subcmd-preview` when set so people could be testing all of the features as we introduced them.

@co42
Copy link
Contributor Author

co42 commented Sep 7, 2021

@pksunkara, @epage is there some other modification I need to do on this PR ?

@pksunkara pksunkara merged commit d03638c into clap-rs:master Sep 7, 2021
epage added a commit to epage/clap that referenced this pull request Oct 23, 2021
PR clap-rs#2751 highlighted a problem we have where the variable names we use
could collide with users.  Rather than parse out when or not to use
special names, and worry about people keeping that up to date through
refactors, I globally renamed all variables by adding a `__clap_`
prefix, which looks like what serde does to solve this problem.

I audited the result with `cargo expand`.  I didn't add any tests
because any tests would be reactionary and would give us a false sense
of protection since any new code could hit this with anything we do.
Our best route for naming is consistency so people are likely to notice
and copy.

Fixes clap-rs#2934
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