Clap: Enable Arg::env also for options which do not take value

[izderadicka]

Affected Version of clap

2.33.0

Feature Request Summary

It would nice if Arg::env would work also for argument that do not take value. In this case is_present will mean that either:

• Argument is present on command line, or
• There is environment variable, with given name, which is not empty (similar to test -n "$MY_VAR" in shell)

Expected Behavior Summary

Arg::env should not automatically set takes_values (but this is a breaking change, so maybe just leave it as it is).
If Arg::env has defined related env variable and takes_value is false, then ArgMatches::is_present will be true if argument is present on command line or env variable exists and is not empty.

Actual Behavior Summary

Currently clap Arg::env works only for arguments which take value - environment value is used as default if argument is not provided on command line. Arg::env also automatically sets takes_value to true automatically, which can lead to bit confusing behavior (if you do not know that Arg::env works only for Arguments which takes value (and considering that takes_value = false is default). See short code example below:

extern crate clap;

use clap::{App,Arg};
use std::env;

fn main() {
    env::set_var("MY_FLAG", "1");

    let matches_not_ok = App::new("prog")
        .arg(Arg::with_name("flag")
            .long("flag")
            .env("MY_FLAG")
            .takes_value(false)
            )
        .get_matches_from(vec![
            "prog"
        ]);

    // Not working for argument which does not take value :-(
     assert!(! matches_not_ok.is_present("flag"));  

     let matches_ok = App::new("prog")
        .arg(Arg::with_name("flag")
            .long("flag")
            .takes_value(false) // switching order of takes_value and env
            .env("MY_FLAG")
            )
        .get_matches_from(vec![
            "prog"
        ]);

    // And it looks like it's working 
     assert!(matches_ok.is_present("flag"));  

     //but flag will now consumes value, cause env will set takes_value to true behind scenes

     env::remove_var("MY_FLAG");

     let matches_ok = App::new("prog")
        .arg(Arg::with_name("pos"))
        .arg(Arg::with_name("flag")
            .long("flag")
            .takes_value(false) 
            .env("MY_FLAG")
            )
        .get_matches_from(vec![
            "prog", "--flag", "pos"
        ]);

    // now works as expected, however now flag takes value
     assert!(matches_ok.is_present("flag"));  
     // and pos argument is consumed
     assert!(! matches_ok.is_present("pos"));  
     //by flag
     assert_eq!( matches_ok.value_of("flag"), Some("pos"));

}

[pksunkara]

We don't need to backport this 2.x, so we can remove W: 2.x label.

[CreepySkeleton]

Here's what I propose:

• Arg::env no longer implies takes_value(true). It's up to user to set it.
• If env("ENV") IS set:
  
  
  
  • if takes_value(true) IS NOT set (the arg is a flag), than the flag is considered raised if ENV is defined. Its precise value doesn't matter, empty or not, its precise value will not be recorded.
  
  
  • if takes_value(true) IS set, than ENV behaves just like it does today.
  
  

@pksunkara Any objections?

[pksunkara]

Agreed. Marking this for 3.0

[pksunkara]

@ldm0 Would you like to take this next?

[ldm0]

No problem.