Add is_env_present to ArgMatches

[davidMcneil]

Closes #1345

Signed-off-by: David McNeil mcneil.david2@gmail.com

[CreepySkeleton]

So far looks good, but let's rename it to is_env to be consistent with Arg::env.

Also, I would go with this description

/// Returns true if, and only if, the value was supplied through an environment variable. 
///
/// If the value was either supplied through a command line option or was set with a default_value
/// or was not supplied at all, returns false.

[CreepySkeleton]

Why do you need this helper? It's a two line function and you're using it in only one place.

FYI: if a function is not supposed to be exported to user (it's not a part of public API), you should use pub(crate) but not pub.

[davidMcneil]

Why do you need this helper?

I liked the encapsulation it provided. Otherwise, I had to bring MatchedArg into parser.rs. Happy to remove it if you prefer.

[pksunkara]

The name here is not readable. Once you rename the function and the field, you should probably rename this.

[davidMcneil]

Also, I would go with this description

/// Returns true if, and only if, the value was supplied through an environment variable. 
///
/// If the value was either supplied through a command line option or was set with a default_value
/// or was not supplied at all, returns false.

This actually is different than how is_env_present behaves. is_env_present only checks if any value for the argument was from the environment variable. If the user set the environment value and the value on the command line is_env_present would still be true (I changed the examples to make this clear).

[CreepySkeleton]

is_env_present only checks if any value for the argument was from the environment variable.

Then this function is pointless since it would be a synonym for

std::env::var("ENV_VAR") == Err(ValError::NotPresent)

No point to bring it in clap 🤷

[CreepySkeleton]

What is your use case? I suspect the semantic I described is the one you really need.

[davidMcneil]

Then this function is pointless since it would be a synonym for

While this is mostly true, there could potentially be a race condition from when clap reads the env var to when this check is called. I admit that this would be very rare/odd.

The real problem is that if all you have is an ArgMatches how do you determine the name of the environment variable for a specific argument? You have to keep a separate lookup.

What is your use case?

I am trying to distinguish between when an argument's value was set with an environment variable vs when it was set with a default value. One way to do this would be to check if the environment variable was set.

I suspect the semantic I described is the one you really need.

Yes, this would work too and I agree would be more useful. But because arguments can have multiple values does this basically just ensure that the first argument in values was from the environment variable? How does that work with multiple?

[CreepySkeleton]

While this is mostly true, there could potentially be a race condition from when clap reads the env var to when this check is called. I admit that this would be very rare/odd.

clap never sets/unsets environment variables. If it was available just before clap started, it is available just after it has finished. You can argue here, "What if I set an env var from inside a validator?". Well if you need to do that, something is very wrong with your design already.

The real problem is that if all you have is an ArgMatches how do you determine the name of the environment variable for a specific argument?

You are kinda the developer of your CLI, aren't you? 😄 How can you not know it? You define these arguments, you define the env name, therefore you know it.

I am trying to distinguish between when an argument's value was set with an environment variable vs when it was set with a default value. One way to do this would be to check if the environment variable was set.

The proper way to do this is adding is_default_value which returns true if, and only if, the arg was not supplied at all and was set via "default value". I would accept such a PR.

I don't really think that introducing a full synonym of a well known function from std will get us anywhere, no offense.

How does that work with multiple?

If you ask me, env variables are meant to be fallbacks, not additional_source; the correct behavior here is: command line replaces env var as env was never used. But the simple test proves me wrong:

use clap::{App, Arg};

fn main() {
    std::env::set_var("ENV_VAR", "val_env");
    
    let m = App::new("app")
        .arg(
            Arg::with_name("test")
                .env("ENV_VAR") 
                .min_values(0)
        )
        .get_matches_from(&["test", "val1", "val2"]);
    
    let values =  m.values_of("test").unwrap().collect::<Vec<_>>();
    assert_eq!(values, &["val1", "val2", "val_env"]);
}

I'm going to raise an issue about that.

[davidMcneil]

How can you not know it? You define these arguments, you define the env name, therefore you know it.

If you are iterating over a list of argument names and getting matches from ArgMatches it would be nice to not have to keep a separate mapping of argument name to environment name. But yes you are right ultimately you know it 😄

The proper way to do this is adding is_default_value which returns true if, and only if, the arg was not supplied at all and was set via "default value".

This sounds like a better approach, thanks. I will close this PR and work on that solution.

If you ask me, env variables are meant to be fallbacks, not additional_source

Yes! Completely agree. This was very confusing to me and I would love to see it behave the way you describe.