Clap: Add configuration to allow empty values in v3

[matchai]

Describe your use case

https://github.com/clap-rs/clap/blob/master/v3_changes.md has a point about:

• Allow empty values no longer default

For starship/starship, the prompt setup script may occasionally provide empty values to flags. On 3.0.0-alpha.1, it seems that empty values surface an error to the user.
It would be nice to have a configuration option to optionally accept empty values.

Describe the solution you'd like

A new clap attribute that would allow empty values to be passed in without surfacing an error.
Perhaps allow_empty, or something along those lines.

#[derive(Clap, Debug)]
enum Opts {
    Prompt {
        #[clap(short, long, allow_empty)] // <-- allow_empty would allow empty strings
        status: Option<String>, // Would be `Some("")`
    },
}

[CreepySkeleton]

It looks like empty_values is gone in 3.x, thus empty values are simply impossible in 3.0.

I would actually expect empty values to be preserved; I don't understand why would we need to strip them at all. They should be allowed by default - and processed correctly, as any other value - and maybe an optional switch to remove them, akin empty_values in 2.33.

@pksunkara @TeXitoi @Dylan-DPC what do you think?

[pksunkara]

I think this can be done by changing Option<String> to String and providing a default of empty value.

[CreepySkeleton]

@pksunkara I strongly disagree here, there is a real difference between "default value is an empty one" and "no default values but the user can pass an ampty val".

[CreepySkeleton]

This is also not about just derive, it is also about raw clap.

[matchai]

I think this can be done by changing Option<String> to String and providing a default of empty value.

Unfortunately, it seems like even defaulting to an empty value surfaces the same error.

Code:

use clap::Clap;

#[derive(Clap, Debug)]
#[clap(author, version)]
enum Opts {
    /// Prints the full starship prompt
    Prompt {
        /// The status code of the last executed command
        #[clap(short, long, default_value = "")]
        status: String
    },
}

fn main() {
    match Opts::parse() {
        Opts::Prompt { status } => {
            println!("{:?}", status);
        }
    }
}

[CreepySkeleton]

@matchai I recommend you to stick with structopt + clap 2.33 for the time being (empty vals are allowed by default there):

#[derive(StructOpt, Debug)]
enum Opts {
    Prompt {
        #[structopt(short, long)] 
        status: Option<String>, // Would be `Some("")`
    },
}

[TeXitoi]

I think that empty value should even be allowed by default. Forbidding empty value can be done without any more feature, just a custom checker. I don't have any veto against having an forbid_empty flag, but I don't think it's really needed.

[TeXitoi]

Is there any reference of why empty values are forbidden by default in v3?

[CreepySkeleton]

@kbknapp why did you removed empty values' handling in 3.0?

[CreepySkeleton]

Here's the situation:

• Empty values are disallowed by default
• User can allow them on per-arg basis via ArgSetting::AllowEmptyValues: https://github.com/clap-rs/clap/blob/aae96236b27d43ede24bd7e58668786cd1073c21/src/build/arg/settings.rs#L40
• There's no way to allow empty values globally, for all the app.

I think we should allow empty values by default (as it used to be in clap 2) and add ForbidEmptyValues setting, both app-level and arg-level.

[pksunkara]

I don't think we should allow them by default because it would conflict with default values.

[CreepySkeleton]

No, it wouldn't.

```
app --foo '' < EXPLICIT EMPTY VALUE
app --foo= < IMPLICIT EMPTY VALUE
app --foo < NO VALUE - DEFAULT
app < NO OPTION NO VALUE - DEFAULT

[CreepySkeleton]

See also https://github.com/clap-rs/clap/pull/1587#issuecomment-568829343

[CreepySkeleton]

@kbknapp While you're still here, I'd love to hear your feedback on this one. Why did you decide to disallow empty values by default in 3.0? For instance, argparse allows them. What do you think about my comment above?

[kbknapp]

I'll preface this with I don't have strong feelings either way on the matter.

The main reason the were removed was that in 2.x they were a source of confusion and complicated the parsing rules solely because of the way clap was implemented. If clap had parsed values slightly differently it may have been less an issue.

The problem was only with implicit empty values (--foo= and --foo [with no min_values(0)]) and came from clap not really having much look-ahead/look-behind ability while in the "value parsing" state. So it was hard to distinguish between an error (missing value), or the user just not wanting a value without it being an explicit empty value "".

One thing I wanted to prevent (which could be argued was too strict or over-reaching) was to prevent typos where instead of --foo=bar one accidentally types --foo= bar. If one truly wanted an empty value they could do --foo="".

This was somewhat compounded when parsing short arguments that coalesce (-f accepts a value):

-abcf bar --other
-abcf= bar --other

vs

-abcf"" bar --other
-abcf="" bar --ohter

In the first case, is bar a positional value, or the value for -f?

So that's a long answer for, "It stemmed only from the way in which clap was implemented, and made the parsing rules more complicated if you need be able to handle when the consumer hasn't told you if they want to accept implicit empty values. It's difficult to know if the value you're parsing is really a value, or a positional argument, etc."

[CreepySkeleton]

Regarding to typoproof behavior - sorry Kevin, but it never worked anyway :)

As @rivy pointed in that thread:

most shells parse and return --foo= as the resultant argument for all of --foo=, --foo='', and --foo="". These are supplied, and there is usually no reasonable way to differentiate at the level of the executable. So, generally, --foo= is / has to be interpreted as foo with an empty string argument.

Notably, windows CMD is, as usual, an exception, but most option libraries for windows emulate the *nix standard.

So basically, there's no way to differentiate between --foo="" and --foo= on most systems. I even took some time to tests it :)

fn main() {
    for a in std::env::args().skip(1) {
        println!("{:?}", a);
    }
}

Windows:

D:\workspace\probe>cargo run -- --foo=
    Finished dev [unoptimized + debuginfo] target(s) in 0.18s
     Running `target\debug\probe.exe --foo=`
"--foo="

D:\workspace\probe>cargo run -- --foo=""
    Finished dev [unoptimized + debuginfo] target(s) in 0.20s
     Running `target\debug\probe.exe --foo=`
"--foo="

D:\workspace\probe>cargo run -- --foo=''
    Finished dev [unoptimized + debuginfo] target(s) in 0.17s
     Running `target\debug\probe.exe --foo=''`
"--foo=\'\'"

Ubuntu:

user@user-PC:/mnt/d/workspace/probe$ cargo run -- --foo=
    Finished dev [unoptimized + debuginfo] target(s) in 0.19s
     Running `target/debug/probe --foo=`
"--foo="

user@user-PC:/mnt/d/workspace/probe$ cargo run -- --foo=""
    Finished dev [unoptimized + debuginfo] target(s) in 0.25s
     Running `target/debug/probe --foo=`
"--foo="

user@user-PC:/mnt/d/workspace/probe$ cargo run -- --foo=''
    Finished dev [unoptimized + debuginfo] target(s) in 0.21s
     Running `target/debug/probe --foo=`
"--foo="

As you see, they are the same from clap's standpoint. There's no -abcf bar --other vs -abcf"" bar --other because they are identical.

Well, windows doesn't special case '', but this is because strings on Win CMD are always written as "...", it doesn't understand '...' strings (Powershell works as bash in this regard). Nobody will try it on Win because this is not how strings work on Win.

It's all or nothing: empty values are either allowed (and I really think we should be) and work as I described above or empty values are disallowed and neither --foo= nor --foo="" nor --foo "" work.

[kbknapp]

I agree empty values should be allowed.

Thanks for taking the time to test/report all that!

[pksunkara]

Found the commit which removed this. https://github.com/clap-rs/clap/commit/cacc23473c64ec8a57471285b3978face05b4d86

[pksunkara]

Just found that AllowEmptyValues flag is still available in settings here