Perform type validation on string var_type for benefit of default value

[matthewarmand]

The Problem

I wouldn't call this a bug with this library per se, but it does create a situation where it's easy for a developer writing client code using envs to get themselves into trouble.

At various points I've seen devs omit the var_type argument on the env() call out of habit, but supply a non-string argument as the default; for example, env('TIMEOUT_IN_S', 5).

The 'string' var_type is the default if no other is supplied, but is mapped to None as a validator/caster in the valid_types dictionary, meaning it'll just pass-through whatever value is supplied. This is fine when it's reading the value out of the environment, since os.getenv returns a str.

However, this also operates on the default argument passed in if no value is found in the environment. Effectively, the 'string' var_type will allow any old type to be used as a default. It's worth pointing out this is not the case for the other types; using a var_type='boolean' will validate the default to be one of the acceptable boolean values. It's only because of the pass-through behavior for 'string' that this type mismatch can occur.

This is a problem because client code now has a bug with inconsistent types that won't be discovered until the value is overridden in the environment. In the TIMEOUT_IN_S example above, if no environment override is set, the value will be the int 5. However when the environment specifies 10, that will be read as a string rather than an integer, which can break the code depending what this value is passed into.

Proposed Solution

The behavior we want, arguably, is that when var_type and default are both supplied, they must be type-compatible with one another based on the logic in validate_type. Because validate_type is called after os.getenv(key, default), it's already operating on both the default value and any value found in the environment. The least-intrusive change is to remove the None bypass logic for strings in valid_types and validate_type.

After this change, strings read out of the environment will still resolve just fine in the if isinstance(value, klass): return value branch, but a programmer supplying an integer default while omitting the var_type argument (or even just supplying a var_type argument that doesn't match the type of default) will be cautioned immediately by the call throwing a TypeError.

I don't believe there's any existing valid use case that this change breaks; I've also verified all existing unit tests still pass successfully, and added a case for the behavior against which I'm trying to guard.

Let me know if you have any questions or feedback on the change that would block this from being accepted, thanks!