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

Dynamic settings: there is no way to return None, False, 0, empty string #68

Closed
unknownlighter opened this issue Oct 27, 2017 · 8 comments · Fixed by #69
Closed

Dynamic settings: there is no way to return None, False, 0, empty string #68

unknownlighter opened this issue Oct 27, 2017 · 8 comments · Fixed by #69

Comments

@unknownlighter
Copy link

if self._dynamic_reader:
    dynamic_result = self._dynamic_reader.get(attr)
    if dynamic_result:
        self._dict[attr] = dynamic_result
        result = dynamic_result

This code does not allow to return from dynamic_reader value that doesn't meet the condition if self._dynamic_reader: such as None, False, 0, ''

@unknownlighter
Copy link
Author

I suggest that _dynamic_reader should raise AttributeError if attr not found. Then

if self._dynamic_reader:
    try:
    	dynamic_result = self._dynamic_reader.get(attr)
    except AttributeError:
    	pass
    else:
        self._dict[attr] = dynamic_result
        result = dynamic_result

@drgarcia1986
Copy link
Owner

drgarcia1986 commented Oct 27, 2017

make sense, how do you think about handle this behavior (raise AttributeError) on dynamic_settings backends (e.g. Redis)?

@drgarcia1986
Copy link
Owner

to begin we can check if result is diferent of None and not a "non zero" value, e.g.:

if self._dynamic_reader:
    dynamic_result = self._dynamic_reader.get(attr)
    if dynamic_result is not None:  # <---
        self._dict[attr] = dynamic_result
        result = dynamic_result

This change fix issues with 0, '' and False, what do you think @unknownlighter ?

@unknownlighter
Copy link
Author

python redis client has a different behavior for cases when key was not found and when key has None value:

>>> import redis
>>> r = redis.StrictRedis()
>>> r.get('foo')
>>> r.set('foo', None)
True
>>> r.get('foo')
b'None'

so we can raise AttributeError when redis returned None and return None when redis returned b'None'

@unknownlighter
Copy link
Author

Anyway, check for None value (if dynamic_result is not None:) much better than current behavior

@drgarcia1986
Copy link
Owner

Great, I'll work on it!

@drgarcia1986
Copy link
Owner

Thank you @unknownlighter for your feedback, I'll release a hotfix soon.

@unknownlighter
Copy link
Author

Great. Thank you @drgarcia1986 for fast response and fix

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants