Feature proposal: SafeEnvironWrapper

[antonagestam]

When writing applications that take configuration from the environment it's often a good idea to err on failing fast if the application isn't correctly configured. The current implementation of EnvironWrapper lies on the other side of the spectrum, choosing to return None if values aren't found. This moves the responsibility of validating configuration unnecessarily deep into applications.

I propose creating a new class, SafeEnvironWrapper that will throw an exception when values aren't found and no default is provided. This allows the parsing methods to be accurately typed as returning only an instance of the specific type when no default is provided.

This also requires having a static type for UNDEFINED. We can achieve that either with NewType or with a class:

Undefined = NewType("Undefined", object)
UNDEFINED = Undefined(object())

class _Undefined: ...
UNDEFINED = _Undefined()

[lundberg]

I like the idea, but maybe it could be implemented in current EnvironWrapper as a setting, i.e. EnvironWrapper(require_defaults=True).

That way the api could stay the same, and one could either instantiate or set it...

# settings.py
from bananas.environment import env
env.require_defaults = True

env.get("FOO")  # raise

thoughts?

[antonagestam]

@lundberg For the plain get, I think the current API is fine, it works like get on dict/Mapping and so is not surprising to users, but it could be updated and hinted as returning Optional[str] (overloaded with Union[str, Default]).

My issue lies more with the typed coercing get methods: get_bool, get_int, etc. I want them to have signatures like:

@overload
def get_bool(key: str, default: Undefined) -> bool:
    ...  # raises if the var isn't set or its value can't be coerced to a bool
@overload
def get_bool(key: str, default: DT) -> Union[DT, bool]:
    ... # returns `default` if  the var isn't set, raises if it can't be coerced

I would then suggest introducing get_str that works identically to the other typed getters.

I think changing the behavior based on a class attribute becomes extremely hard to type accurately, and the same goes for the change you're suggesting unfortunately. I would prefer to create a new class, but we could let it live in something like bananas.safe_environment and be imported as from bananas.safe_environment import env, if we wanted the exposed variable to be named the same.

[lundberg]

Good points, totally agree.

Only itching part left to solve IMO is import/instantiating of the "safe" version. I'm thinking, if you know you want the safe raising version, then you already know that you'll need to provide a default ;-)

What I think I'm trying to say is that I'd like to see current env to work as your proposal by default, but without breaking backwards compatibility, which of course is impossible ;-). So in other words, naming is key here.

Maybe just...

# settings.py
from bananas.environment import strict_env as env

env.get_bool("FOO")   # raises if the var isn't set etc...

...is enough to please that.

[antonagestam]

+1 for "strict" over "safe".

I'll probably get a PR up for the implementation soon, but let's ponder naming etc in the meantime.

Could it be worth a breaking change and entirely discarding the current behavior? We could introduce a deprecation warning in place of raising in a minor version.

[lundberg]

Could it be worth a breaking change and entirely discarding the current behavior? We could introduce a deprecation warning in place of raising in a minor version.

Had the same thoughts and was why I was trying to have it implemented in the current env but with the setting, i.e. require_default=False with a deprecation warning.

What about having both bananas.environment.env and banans.environment.strict_env in the minor release, and later in a major release change to env = strict_env and introduce non_strict_env or something, making it easy for upgraders that don't want/need strict to just do from bananas.environment import non_strict_env as env.

[antonagestam]

@lundberg My thoughts exactly! 👍

[antonagestam]

Once I find the time to get back to this, it would be nice to also add a env.get_enum(SomeEnum, "SOME_VAR") util.

Another useful would be a get_mapping(enum: E, prefix: str, type_: T) -> MappingProxyType[E, T], that would read up prefix + enum.name for each possible enum. So to map an internal Market enum to account IDs in a foreign domain would look like:

FOREIGN_ACCOUNT_IDS = env.get_mapping(Market, "FOREIGN_ACCOUNT_ID_", int)

And result in:

# Given there are env vars:
# FOREIGN_ACCOUNT_ID_SE=1234
# FOREIGN_ACCOUNT_ID_DK=4321
FOREIGN_ACCOUNT_IDS = {
    Market.SE: 1234,
    Market.DK: 4321,
    ...
}

[Mojken]

@antonagestam what is the status on this? It sounds like you made some progress on it, could you open a draft PR to let some else pick up the flag?

[antonagestam]

@Mojken I think this is as far as I got: https://github.com/5monkeys/django-bananas/compare/feature/safe-environ-wrapper?expand=1

I'm not sure it makes sense to work on that until we have full support for shipping type hints, and we need to have types for the entire library to do that. There's been progress on that, but there's a good chunk of work left.