tomerfiliba / plumbum

Plumbum: Shell Combinators
https://plumbum.readthedocs.io
MIT License
2.81k stars 183 forks source link

Typed env docs #493

Closed koreno closed 4 years ago

koreno commented 4 years ago

@AndydeCleyre - maybe you can comment on these docs?

coveralls commented 4 years ago

Coverage Status

Coverage decreased (-0.01%) to 82.831% when pulling 72c649131cb5745d0e9abc5bc3f44b2718afc99a on typed-env-docs into 0d0f7bf1aea72db8d1e463b959ce7c2512c145bf on master.

AndydeCleyre commented 4 years ago

@koreno Thank you! That's definitely very helpful.

Some things that still confuse me about TypedEnv:

from plumbum.typed_env import TypedEnv
from plumbum import local

class AppEnv(TypedEnv):
    somenum = TypedEnv.Int("SOMENUM")

ae = AppEnv()
local.env['SOMENUM'] = 5
ae.somenum
---------------------------------------------------------------------------
KeyError                                  Traceback (most recent call last)
<ipython-input-6-49f2d6b17817> in <module>
----> 1 ae.somenum

/usr/lib/python3.8/site-packages/plumbum/typed_env.py in __get__(self, instance, owner)
     52                 return self
     53             try:
---> 54                 return self.convert(instance._raw_get(*self.names))
     55             except KeyError:
     56                 if self.default is NO_DEFAULT:

/usr/lib/python3.8/site-packages/plumbum/typed_env.py in _raw_get(self, *key_names)
    118                 return value
    119         else:
--> 120             raise KeyError(key_names[0])
    121 
    122     def __contains__(self, key):

KeyError: 'SOMENUM'

Re-instantiating ae also doesn't pick up on the value. Actually redefining and then re-instantiating still doesn't pick up on it, while local.env does see the value. Even creating a newly named class at this point and instantiating that doesn't pick up on the value. Is it frozen to the environment state at time of python process instantiation?

Also, do you think it would be a good idea for TypedEnv to support dict-like .get access for fallback values, or is the idea to just use one of (default during the class definition; @property during class definition; try/except during variable access) for that kind of behavior?

koreno commented 4 years ago

Thanks for the comments!

  • rules used for TypedEnv.Bool

I will add those to the docs.

  • AttributeError seems to fit better

You might be right, however I did want to make it clear that there is "more than meets the eye"... Maybe a new class EnvironmentVariableError(AttributeError) exception?

  • when are the env values evaluated

They indeed are evaluated at access time. It is local.env that's actually "freezing" the environment and copying it internally. I'm not sure why that is the implementation, it predates me... If you want changes to be visible, use os.environ instead.

AndydeCleyre commented 4 years ago

Thank you, the feature is very nice, and I appreciate the docs additions.

Yeah, as long as isinstance(the exception, AttributeError) I think that's great. And providing info or a clue that it's about env vars seems wise :+1: . I don't know if that kind of change needs to wait for a bigger version release. @henryiii -- any thoughts on the exception type or policy for changing it regarding versions?

On the evaluation time and local.env behavior -- oh, of course! Whoops. But it still doesn't seem right that this doesn't work:

from plumbum import local
from plumbum.typed_env import TypedEnv

class AppEnv(TypedEnv):
    somenum = TypedEnv.Int("SOMENUM")

ae = AppEnv()

with local.env(SOMENUM=50):
    print(local.env["SOMENUM"])
    print(ae.somenum)
50
---------------------------------------------------------------------------
KeyError                                  Traceback (most recent call last)
<ipython-input-1-f3811e69ac87> in <module>
     10 with local.env(SOMENUM=50):
     11     print(local.env["SOMENUM"])
---> 12     print(ae.somenum)
     13 

/usr/lib/python3.8/site-packages/plumbum/typed_env.py in __get__(self, instance, owner)
     52                 return self
     53             try:
---> 54                 return self.convert(instance._raw_get(*self.names))
     55             except KeyError:
     56                 if self.default is NO_DEFAULT:

/usr/lib/python3.8/site-packages/plumbum/typed_env.py in _raw_get(self, *key_names)
    118                 return value
    119         else:
--> 120             raise KeyError(key_names[0])
    121 
    122     def __contains__(self, key):

KeyError: 'SOMENUM'
koreno commented 4 years ago

as long as isinstance(the exception, AttributeError) I think that's great

Unfortunately using AttributeError messes up the implementation (it actually indicates to python that it should try the __getattr__). I've made it inherit from KeyError, since this is still about looking up a name in a namespace, in the general sense. The exception is more to show clear error message than for use in try/except blocks, since normally you'd use a default if you expect the variable to be missing sometimes.

support dict-like .get access

Indeed there is a .get(name, default=None) method on TypedEnv

AndydeCleyre commented 4 years ago

I thought maybe instantiating ae within the with local.env block would make SOMENUM available to ae, but it didn't. Then I tried putting the whole class definition in that block, too, but it's still a KeyError. It should be documented if and how one can share state between local.env and TypedEnv.

koreno commented 4 years ago

to be honest, this sharing of state between local.env and TypedEnv never came up in my work. Can you elaborate on the use-case you're looking to solve? I do agree it doesn't feel "complete", but I fear breaking things that work in an effort to fix something that won't actually be used.

My usage of TypedEnv (the reason I created it) was to centralize environment variable definition and parsing for an app, not so much as means to pass environment on to other subprocs (local.env is used when running commands via the local object)

AndydeCleyre commented 4 years ago

local.env has been the way of working with environment variables in plumbum, so if using TypedEnv means that the user shouldn't be using local.env at all anymore, or that if they do they need to consider it totally incompatible with and isolated from uses of TypedEnv, that should be made a big loud clear warning.

koreno commented 4 years ago

I'll clarify that in the docs, good point.

On Thu, Jan 23, 2020, 23:04 Andy Kluger notifications@github.com wrote:

local.env has been the way of working with environment variables in plumbum, so if using TypedEnv means that the user shouldn't be using local.env at all anymore, or that if they do they need to consider it totally incompatible with and isolated from uses of TypedEnv, that should be made a big loud clear warning.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/tomerfiliba/plumbum/pull/493?email_source=notifications&email_token=AABMCR5LOSJIVJRGH3YQ6A3Q7IA6JA5CNFSM4KJBEZHKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEJY3B3Q#issuecomment-577876206, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABMCRZXXKVTXHWDEOHWUFDQ7IA6JANCNFSM4KJBEZHA .

henryiii commented 4 years ago

Is this ready for review&merge?

koreno commented 4 years ago

Is this ready for review&merge?

It is now