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.
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.
_no_default = '~~!!configstore-no-default!!~~'
configstore/store.py
Outdated
|
|
||
|
|
||
| Backends = TypeVar('Backends', EnvVarBackend, DotenvBackend, | ||
| AwsSsmBackend, DockerSecretBackend) |
There was a problem hiding this comment.
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.
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.
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.
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.
Does this file live at the repo root?
| 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.
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.
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.
Great job fixing the param names in get_setting methods!
Fixes #25