Skip to content

Commit

Permalink
fix(derive): Clap should not derive ArgEnum
Browse files Browse the repository at this point in the history
While having convinience derives can be helpful, deriving traits that
are not used in similar situations (`Clap` and `ArgEnum`) can make
things harder
- From a user, derives are opaque and create uncertainty on how to use
  the API if not kept crystal clear (deriving a name gives you the trait
  by that name)
- This makes documentation harder to write and read
- You can use types in unintended places, which is made worse for crate
  APIs because changing this breaks compatibility.

Fixes clap-rs#2584
  • Loading branch information
epage committed Jul 14, 2021
1 parent 3be79b9 commit 6cc76e7
Show file tree
Hide file tree
Showing 6 changed files with 42 additions and 23 deletions.
4 changes: 2 additions & 2 deletions clap_derive/examples/arg_enum.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,9 @@
//!
//! All the variants of the enum and the enum itself support `rename_all`
use clap::Clap;
use clap::{ArgEnum, Clap};

#[derive(Clap, Debug, PartialEq)]
#[derive(ArgEnum, Debug, PartialEq)]
enum ArgChoice {
Foo,
Bar,
Expand Down
4 changes: 2 additions & 2 deletions clap_derive/examples/value_hints_derive.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,14 +12,14 @@
//! . ./value_hints_derive.fish
//! ./target/debug/examples/value_hints_derive --<TAB>
//! ```
use clap::{App, AppSettings, Clap, IntoApp, ValueHint};
use clap::{App, AppSettings, ArgEnum, Clap, IntoApp, ValueHint};
use clap_generate::generators::{Bash, Elvish, Fish, PowerShell, Zsh};
use clap_generate::{generate, Generator};
use std::ffi::OsString;
use std::io;
use std::path::PathBuf;

#[derive(Clap, Debug, PartialEq)]
#[derive(ArgEnum, Debug, PartialEq)]
enum GeneratorChoice {
Bash,
Elvish,
Expand Down
4 changes: 1 addition & 3 deletions clap_derive/src/derives/clap.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
// MIT/Apache 2.0 license.

use crate::{
derives::{arg_enum, from_arg_matches, into_app, subcommand},
derives::{from_arg_matches, into_app, subcommand},
dummies,
};

Expand Down Expand Up @@ -71,14 +71,12 @@ fn gen_for_enum(name: &Ident, attrs: &[Attribute], e: &DataEnum) -> TokenStream
let into_app = into_app::gen_for_enum(name, attrs);
let from_arg_matches = from_arg_matches::gen_for_enum(name);
let subcommand = subcommand::gen_for_enum(name, attrs, e);
let arg_enum = arg_enum::gen_for_enum(name, attrs, e);

quote! {
impl clap::Clap for #name {}

#into_app
#from_arg_matches
#subcommand
#arg_enum
}
}
1 change: 0 additions & 1 deletion clap_derive/src/dummies.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ pub fn clap_enum(name: &Ident) {
into_app(name);
from_arg_matches(name);
subcommand(name);
arg_enum(name);
append_dummy(quote!( impl clap::Clap for #name {} ));
}

Expand Down
24 changes: 12 additions & 12 deletions clap_derive/tests/arg_enum.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ use clap::{ArgEnum, Clap};

#[test]
fn basic() {
#[derive(Clap, PartialEq, Debug)]
#[derive(ArgEnum, PartialEq, Debug)]
enum ArgChoice {
Foo,
Bar,
Expand Down Expand Up @@ -40,7 +40,7 @@ fn basic() {

#[test]
fn multi_word_is_renamed_kebab() {
#[derive(Clap, PartialEq, Debug)]
#[derive(ArgEnum, PartialEq, Debug)]
#[allow(non_camel_case_types)]
enum ArgChoice {
FooBar,
Expand Down Expand Up @@ -70,7 +70,7 @@ fn multi_word_is_renamed_kebab() {

#[test]
fn variant_with_defined_casing() {
#[derive(Clap, PartialEq, Debug)]
#[derive(ArgEnum, PartialEq, Debug)]
enum ArgChoice {
#[clap(rename_all = "screaming_snake")]
FooBar,
Expand All @@ -93,7 +93,7 @@ fn variant_with_defined_casing() {

#[test]
fn casing_is_propogated_from_parent() {
#[derive(Clap, PartialEq, Debug)]
#[derive(ArgEnum, PartialEq, Debug)]
#[clap(rename_all = "screaming_snake")]
enum ArgChoice {
FooBar,
Expand All @@ -116,7 +116,7 @@ fn casing_is_propogated_from_parent() {

#[test]
fn casing_propogation_is_overridden() {
#[derive(Clap, PartialEq, Debug)]
#[derive(ArgEnum, PartialEq, Debug)]
#[clap(rename_all = "screaming_snake")]
enum ArgChoice {
#[clap(rename_all = "camel")]
Expand All @@ -141,7 +141,7 @@ fn casing_propogation_is_overridden() {

#[test]
fn case_insensitive() {
#[derive(Clap, PartialEq, Debug)]
#[derive(ArgEnum, PartialEq, Debug)]
enum ArgChoice {
Foo,
}
Expand All @@ -168,7 +168,7 @@ fn case_insensitive() {

#[test]
fn case_insensitive_set_to_false() {
#[derive(Clap, PartialEq, Debug)]
#[derive(ArgEnum, PartialEq, Debug)]
enum ArgChoice {
Foo,
}
Expand All @@ -190,7 +190,7 @@ fn case_insensitive_set_to_false() {

#[test]
fn alias() {
#[derive(Clap, PartialEq, Debug)]
#[derive(ArgEnum, PartialEq, Debug)]
enum ArgChoice {
#[clap(alias = "TOTP")]
TOTP,
Expand Down Expand Up @@ -218,7 +218,7 @@ fn alias() {

#[test]
fn multiple_alias() {
#[derive(Clap, PartialEq, Debug)]
#[derive(ArgEnum, PartialEq, Debug)]
enum ArgChoice {
#[clap(alias = "TOTP", alias = "t")]
TOTP,
Expand Down Expand Up @@ -252,7 +252,7 @@ fn multiple_alias() {

#[test]
fn option() {
#[derive(Clap, PartialEq, Debug)]
#[derive(ArgEnum, PartialEq, Debug)]
enum ArgChoice {
Foo,
Bar,
Expand Down Expand Up @@ -282,7 +282,7 @@ fn option() {

#[test]
fn vector() {
#[derive(Clap, PartialEq, Debug)]
#[derive(ArgEnum, PartialEq, Debug)]
enum ArgChoice {
Foo,
Bar,
Expand Down Expand Up @@ -312,7 +312,7 @@ fn vector() {

#[test]
fn from_str_invalid() {
#[derive(Clap, PartialEq, Debug)]
#[derive(ArgEnum, PartialEq, Debug)]
enum ArgChoice {
Foo,
}
Expand Down
28 changes: 25 additions & 3 deletions src/derive.rs
Original file line number Diff line number Diff line change
Expand Up @@ -206,12 +206,34 @@ pub trait Subcommand: Sized {
fn augment_subcommands_for_update(app: App<'_>) -> App<'_>;
}

/// @TODO @release @docs
/// Parse arguments into enums.
///
/// When deriving [`Clap`], a field whose type implements `ArgEnum` can have the attribute
/// `#[clap(arg_enum)]`. In addition to parsing, help and error messages may report possible
/// variants.
///
/// # Example
///
/// ```rust
/// #[derive(clap::Clap)]
/// struct Args {
/// #[clap(arg_enum)]
/// level: Level,
/// }
///
/// #[derive(clap::ArgEnum)]
/// enum Level {
/// Debug,
/// Info,
/// Warning,
/// Error,
/// }
/// ```
pub trait ArgEnum: Sized {
/// @TODO @release @docs
/// All possible argument choices, in display order.
const VARIANTS: &'static [&'static str];

/// @TODO @release @docs
/// Parse an argument into `Self`.
fn from_str(input: &str, case_insensitive: bool) -> Result<Self, String>;
}

Expand Down

0 comments on commit 6cc76e7

Please sign in to comment.