Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

default seems broken #192

Closed
mbonnefoy opened this issue Jul 13, 2018 · 10 comments
Closed

default seems broken #192

mbonnefoy opened this issue Jul 13, 2018 · 10 comments
Labels
bug Something isn't working

Comments

@mbonnefoy
Copy link

mbonnefoy commented Jul 13, 2018

Hi there,

I've just updated from 0.4.4 to 0.4.5 and noticed a few unexpected behaviours with default. Presumably they relate to the casting changes that were introduced recently?

# the key is in my env
In [3]: env('GOOGLE_ANALYTICS_KEY')
Out[3]: 'UA-123456-78'
# I should have used a string as default here
# but I would still expect these to return my key instead of False

In [4]: env('GOOGLE_ANALYTICS_KEY', default=False)
Out[4]: False

In [7]: env('GOOGLE_ANALYTICS_KEY', default=True)
Out[7]: False
# these ones work as expected

In [5]: env('GOOGLE_ANALYTICS_KEY', default='')
Out[5]: 'UA-123456-78'

In [6]: env('GOOGLE_ANALYTICS_KEY', default=None)
Out[6]: 'UA-123456-78'

Cheers,
Matthieu

@craigds
Copy link

craigds commented Sep 7, 2018

This seems like a pretty serious issue, any chance of an update?

@fdemmer
Copy link
Contributor

fdemmer commented Jan 25, 2019

I would still expect these to return my key instead of False

i can see why you would expect that on first glance and i also could not find clear documentation :(
however, imho the current implementation makes sense, does the right thing and is not a bug.

what you are asking the software to do in the first two examples:

  • there might be an environment variable called GOOGLE_ANALYTICS_KEY, return that
  • if you can't find it, give me a boolean with value False (or True)

the default kwarg not only defines the default value to return, but also a type.

the rules for turning (string) environment variables into booleans are documented.

Since the value of your environment variable is "UA-123456-78" and not any of the BOOLEAN_TRUE_STRINGS, False is returned.

these ones work as expected

..., because None does not cause any type casting and '' is implicitly the type you want returned.

i mentioned, that i think this makes sense and to illustrate an example:

DEBUG = env('DEBUG', default=False)

i would not want my DEBUG setting to become "gibberish" (which may be seen as truthy in a lot of Python code) when the environment variable contains "gibberish". i want it to be False in any case, except when the string contains one of the BOOLEAN_TRUE_STRINGS. it always has to be a boolean.

I should have used a string as default here

this is important, because in this case i'd argue, there is a bug:

>>> env.str('NOTE_SET', default=True)
True

>>> env.str('GOOGLE_ANALYTICS_KEY', default=True)
False

imho, this absolutely should return a boolean, when the environment variable is not set and the str value when it is, because that is explicitly what the code says :)

@jbagot
Copy link

jbagot commented Feb 5, 2019

It's really interesting. It was failing some weeks ago. But now I'm trying another time the same and it's not failing anymore.

>>> import environ
>>> env = environ.Env()
>>> env.list('ALLOWED_HOSTS')
['localhost', '127.0.0.1']
>>> env.list('ALLOWED_HOSTS2', default=True)
True
>>> env.list('ALLOWED_HOSTS2', default=False)
False
>>> env('WRONG', default=True)
True
>>> env('WRONG', default=False)
False

It's working as expected and I'm using the version 0.4.5.

But it was failing 18 days ago at least. And was a pretty serious issue because a lot of projects rely on it and in one minor update, without any mention in the changelog the default behavior changed.

@kozlek
Copy link

kozlek commented Feb 7, 2019

Same for me. Avoid using python True or False with the default parameter.

@mbonnefoy
Copy link
Author

@fdemmer Thanks for the feedback. I am not using django-environ anymore, please feel free to close.

@janhh
Copy link

janhh commented May 6, 2019

In case we expect a string from the environment variable and use the default parameter, should we use default='' or default=None? Will default=None always return None when variable isn't set? I think the use of default alone to decide casting of environment variable is more implicit than explicit and maybe not the most pythonic. Would it make sense to at least introduce the cast argument in the introduction example?

I got bitten by the 0.4.4 to 0.4.5 default behavior as well, will future proof our settings.py with explicit casting now.

@joke2k
Copy link
Owner

joke2k commented Jul 12, 2019

Hi @mbonnefoy thank you for posting the issue,
I'm digging in the code to understand what produces this behaviour:

v0.4.4...v0.4.5#diff-674c4b6fffd8c014550873f5a20c5149R286

This is the change from 0.4.4 to 0.4.5.
With smart casting, I introduced more complexity and this is a side effect.

@jbagot the issue is when environ try to cast a str value (all truable: int, list, dict...) as a bool (taken from type of default):

Passing a boolean as default will cast the value to True if the key is found (bug), or the default itself if not (not a bug).

@Tberdy is right, for now just avoid to use boolean as default in not boolean variables (that makes sense).

I'll fix this issue as soon as possible (days, not months :D)

@joke2k joke2k added the bug Something isn't working label Jul 12, 2019
@mbonnefoy
Copy link
Author

Thanks @joke2k 🙂

@pablobuenaposada
Copy link

pablobuenaposada commented Oct 28, 2019

I have been taking a look at this and I cannot see any benefit of this smart casting from default type.

To have a variable that normally is supposed to contain for example an int but in case of no definition from the env you want to explicitly set it to None or False is something common, with the new version this is not possible anymore, you are killing the dynamic typing Python offers.

If you want to force type casting for a specific variable you can currently do it like this:

>>> import environ
>>> env = environ.Env()
>>> env('HOME')
'/Users/pablobuenaposada'
>>> env('HOME', cast=list)
['/Users/pablobuenaposada']

this is ok, is working fine, but if you don't specify any type casting I think it shouldn't do it for you because is a feature.

Why not revert this smart cast changes?

@joke2k
Copy link
Owner

joke2k commented Mar 16, 2020

Hi, I added a new option to disable the smart casting.

4028a79

The next release came soon with latest django/python support

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

8 participants