[jszwedko]

When assigning values to flags (also when interogatting via
context.(Global)IsSet.

Using syscall.Getenv rather than os.LookupEnv in order to support older Golang versions.

For boolean flags, consider empty as false.

One important consideration here is that context.(Global)IsSet() will return
true for flags whose specified environment variable has been set even when
the value of that variable fails to be parsed as the type that the flag
requires; however, I think this behavior is better than the previous behavior
where IsSet would return false even if the variable truly was set by the
environment.

In order to properly support fix considering environment variables with invalid
values (which are currently silently ignored) is to augment the Flag
interface's Apply() method to be able to return an error (which I'd suggest
we do in v2 since we cannot do this here without breaking compatibilty).

Fixes #517

[jszwedko]

I have a couple of ideas for poor hacks to make IsSet consistent with the Apply implementations wherein it returns false if the env var was set, but was not parsable (while maintaining backwards compatibility).

I'd be interested in feedback on the current changes though (particularly around bool handling).

/cc @meatballhat @vikstrous

[meatballhat]

LGTM 🤘

[vikstrous]

This sounds reasonable. I agree that values that failed to parse shouldn't be considered set. If any variables fail to parse the program shouldn't even get to execute IMO. The behaviour you describe is indeed an improvement and works for my use case.

[jszwedko]

After discussing briefly with @meatballhat, we decided that it is closer to the original intent of parsing environment variables to error if they are set to unparsable values (rather than silently ignoring). I pushed a second commit, b816af8, which does this and, by consequence, (Global)IsSet will not return true if the environment variables failed to be parsed.

@meatballhat do you mind reviewing the implementation here? It's a bit hacky right now to maintain compatibility, but I think we can slim it down in v2 by modifying the existing Flag interface rather than having to have both and reflect.