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

App feels like an awkward name in the API #3089

Closed
epage opened this issue Dec 8, 2021 · 3 comments
Closed

App feels like an awkward name in the API #3089

epage opened this issue Dec 8, 2021 · 3 comments
Labels
A-builder Area: Builder API C-bug Category: Updating dependencies E-easy Call for participation: Experience needed to fix: Easy / not much M-breaking-change Meta: Implementing or merging this will introduce a breaking change.
Milestone

Comments

@epage
Copy link
Member

epage commented Dec 8, 2021

Maintainer's notes

  • The plan is to rename App to Command, including in other names like app_from_crate and AppSettings

clap version: master

Existing code:

use clap::{app_from_crate, arg, App, AppSettings};

fn main() {
    let matches = app_from_crate!()
        .global_setting(AppSettings::PropagateVersion)
        .global_setting(AppSettings::UseLongFormatForHelpSubcommand)
        .setting(AppSettings::SubcommandRequiredElseHelp)
        .subcommand(
            App::new("add")
                .about("Adds files to myapp")
                .arg(arg!([NAME])),
        )
        .get_matches();

    match matches.subcommand() {
        Some(("add", sub_matches)) => println!(
            "'myapp add' was used, name is: {:?}",
            sub_matches.value_of("NAME")
        ),
        _ => unreachable!(),
    }
}

Personally, its always felt weird constructing an "App" and then adding "apps" as subcommands.

@epage epage added C-bug Category: Updating dependencies A-builder Area: Builder API labels Dec 8, 2021
@epage
Copy link
Member Author

epage commented Dec 8, 2021

I've considered names like Parser or ParserBuilder (see #2911) but we run into conflicts between builder and derive traits.

When splitting up the impl blocks in the docs, I had the idea for Command. I think this makes more sense for the argument to Command::new, in subcommand context, etc.

See

use clap::{app_from_crate, arg, App, AppSettings};

fn main() {
    let matches = command_from_crate!()
        .global_setting(CommandSettings::PropagateVersion)
        .global_setting(CommandSettings::UseLongFormatForHelpSubcommand)
        .setting(CommandSettings::SubcommandRequiredElseHelp)
        .subcommand(
            Command::new("add")
                .about("Adds files to myapp")
                .arg(arg!([NAME])),
        )
        .get_matches();

    match matches.subcommand() {
        Some(("add", sub_matches)) => println!(
            "'myapp add' was used, name is: {:?}",
            sub_matches.value_of("NAME")
        ),
        _ => unreachable!(),
    }
}

@nicoburns
Copy link

Command seems right to me. That's what I've seen other CLI parsing libraries use for this concept. I suppose Subcommand could also work to match the method name. App definitely seems confusing to me. I would expect App to contain the core logic of my program not CLI argument parsing.

@epage epage added M-breaking-change Meta: Implementing or merging this will introduce a breaking change. E-easy Call for participation: Experience needed to fix: Easy / not much labels Dec 9, 2021
@epage epage added this to the 4.0 milestone Dec 14, 2021
@epage
Copy link
Member Author

epage commented Feb 15, 2022

We've now done

  • App -> Command
  • app_from_crate -> command
  • IntoApp::into_app -> CommandFactory::command

The only thing left is removing the deprecations in 4.0. Since we have #3021 for that, going to close this out.

@epage epage closed this as completed Feb 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-builder Area: Builder API C-bug Category: Updating dependencies E-easy Call for participation: Experience needed to fix: Easy / not much M-breaking-change Meta: Implementing or merging this will introduce a breaking change.
Projects
None yet
Development

No branches or pull requests

2 participants