Closed kohlivarun5 closed 10 months ago
@dbuenzli Any thoughts ?
What is this supposed to do ?
Currently, flags created using flag
support specifying them as environment variables. So, for a flag say --some-flag
, one can also specify SOME_FLAG_ENVIRONMENT
and enable the flag via environment
https://github.com/dbuenzli/cmdliner/blob/master/src/cmdliner_arg.ml#L102
That doesn't work for vflag
. This PR adds similar support for environment parsing for vflag
So this adds one env variable per case in the list. I'm not sure I'm very happy about the idea. This makes a lot of irrepresentable states legal e.g. for:
Cmdliner.Arg.(vflag 0 [1, info ["one"]; 2, info ["two"]; 3, info ["three"]]);;
You would rather like a single variable to define the actual value rather than three mutually exclusive boolean variables.
The code prevents representation of illegal states (commented inline about prevention of duplicate options)
Not really, rather it errors on illegal states. But the thing is that mutually exclusive environment variables is a terrible user interface – it shouldn't be hard to convince yourself that to set the value via an environment variable reliably you will have to unset all other variables that could define the value at the same time since those could exist in your environment.
I don't have that in my head but I suspect that the fact that cmdliner doesn't support this for vflag is because I couldn't retro-fit environment variable support it in the current scheme.
If you are dealing with a code base that already uses vflag
is rather suggest to turn the values of the vflag
to options with [None] as a default and do the look up yourself when it's [None]. That is something like (pseudo-code):
let myopt =
let opt =
let opts = [(Some 1, info ["one"]; Some 2, info ["two"]; (Some 3), info ["three"]] in
Cmdliner.Arg.(vflag None opts)
in
let lookup_env = function
| Some v -> `Ok v
| None ->
begin match Sys.getenv with
| exception Not_found -> 0
| v -> match parse v with
| Error e -> `Error (false, e)
| Ok v -> v
in
Term.(ret (lookup_env $ opt))
it shouldn't be hard to convince yourself
I hope you did. Personally I'm not convinced by this proposal.
Currently,
vflag
does not check environment specification for an argument (likeflag
does). This change adds env parsing forvflag