-
Notifications
You must be signed in to change notification settings - Fork 2
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
WIP: Add typing to configstore #28
Conversation
configstore/store.py
Outdated
self._backends += (backend,) | ||
|
||
def get_setting(self, key, default=_no_default): | ||
def get_setting(self, key: str, default=_no_default) -> str: |
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.
Ici, default devrait être un string. Sauf que si default peut être Union[object, str]
, ça veut dire que le retour de la méthode peut être lui aussi Union[object, str]
, mais ce n'est pas du tout ce qu'on veut.
J'aurais tendance à faire un _no_default avec une valeur en string, mais faut trouver un default qui ne se retrouverait pas dans un default. Tu en pense quoi @merwok ?
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.
_no_default = '~~!!configstore-no-default!!~~'
configstore/store.py
Outdated
|
||
|
||
Backends = TypeVar('Backends', EnvVarBackend, DotenvBackend, | ||
AwsSsmBackend, DockerSecretBackend) |
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.
It’s not great to have to import all backends here.
Could we declare and interface or ABC or base type and use that?
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.
I woudl really prefer to declare a base class or an interface. It was my first plan, but I wasn't sure if you'd like it and I saw the TypeVar in the documentation.
My toher questions is since we'll have a backend inteface. could we move all backend in a backends.py
file with the Backend
class ?
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.
Go for an ABC or whatever is recommended by typing / mypy.
I don’t see benefits in moving the concrete backend classes though.
This way, it's really easier to make typing work without ugly hack or long Union typing
There is support for typing in python 2.7 but only in comments and we don't want this
@@ -63,57 +61,6 @@ jobs: | |||
- store_test_results: | |||
path: ~/reports | |||
|
|||
test-py35: |
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.
Comme python 3.5 est juste en security status et que certaine type hint ne sont pas supporté dedans, mais le sont dans python 3.6 et 3.7, j'ai enlevé le support de 3.5.
@@ -6,6 +6,8 @@ __pycache__ | |||
/.coverage | |||
/.tox/ | |||
/venv/ | |||
/.pyre/ | |||
.pyre_configuration |
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.
Does this file live at the repo root?
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.
Normally yes.
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.
This is great! Let’s discuss how to type-check duck-typed backend classes.
name: Test with Python 2.7 | ||
command: | | ||
venv/bin/tox --sdistonly | ||
venv/bin/tox -e py27 -- --junitxml=~/reports/tox/coverage.xml |
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’s some config that can be removed from tox.ini too if you want to clean up.
pass | ||
|
||
def get_setting(self, key: str) -> Optional[str]: | ||
raise NotImplementedError |
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.
I would like this to be a real abstract base class, and also keep the duck typing (not require that classes inherit from the base class). We could chat about this on Monday, the background and reasoning could be quite long for a review message!
(Duck typing / virtual subclasses can be handled by typing: https://mypy.readthedocs.io/en/stable/protocols.html#protocol-types )
|
||
class EnvVarBackend(Backend): | ||
|
||
def get_setting(self, key: str) -> Optional[str]: |
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.
Great job fixing the param names in get_setting methods!
Fixes #25