-
Notifications
You must be signed in to change notification settings - Fork 201
Remove 'bool' misconception in place of strtobool #71
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there are some minor fixes that need to be done, and a small internal api design decision, but this pr it looks good to me.
@@ -3,6 +3,7 @@ | |||
import pytest | |||
from mock import patch | |||
from decouple import AutoConfig, UndefinedValueError, RepositoryEmpty | |||
from distutils.util strtobool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a small syntax error. Missing import
keyword
from distutils.util import strtobool
@@ -4,6 +4,7 @@ | |||
from mock import patch | |||
import pytest | |||
from decouple import Config, RepositoryEnv, UndefinedValueError | |||
from distutils.util strtobool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here.
a small syntax error. Missing import
keyword
from distutils.util import strtobool
@@ -4,6 +4,8 @@ | |||
from mock import patch, mock_open | |||
import pytest | |||
from decouple import Config, RepositoryIni, UndefinedValueError | |||
from distutils.util strtobool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here.
a small syntax error. Missing import
keyword
from distutils.util import strtobool
@@ -88,7 +88,7 @@ Then use it on your ``settings.py``. | |||
.. code-block:: python | |||
|
|||
SECRET_KEY = config('SECRET_KEY') | |||
DEBUG = config('DEBUG', default=False, cast=bool) | |||
DEBUG = config('DEBUG', default=False, cast=strtobool) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Explicit is better than implicit."
it would be good to add from distutils.util import strtobool
to example.
if value.lower() not in self._BOOLEANS: | ||
raise ValueError('Not a boolean: %s' % value) | ||
|
||
return self._BOOLEANS[value.lower()] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
strtobool return 0 and 1. for backward compatibility, should we convert to bool?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or change the internal api to use strtobool
when cast
is bool
35931d3
to
774b40f
Compare
No description provided.