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.

Sign up for GitHub

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

Jump to bottom

WIP: Add typing to configstore #28

Closed
wants to merge 13 commits into from
Closed

WIP: Add typing to configstore #28

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
b6eeecf
Ignore .mypy_cache/
just1602 Oct 4, 2018
7041518
Add basic typing stuff
just1602 Oct 16, 2018
3988b89
Remove unsued imports
just1602 Oct 16, 2018
9945746
Merge remote-tracking branch 'origin/master' into typing
just1602 Oct 18, 2018
6307d64
Use a `_no_default` str variable instead of object
Oct 20, 2018
06efe00
Remove python 2.7 tests since typing isn't support
Oct 20, 2018
b92c96a
Remove python 2.7 support from flit packaging config file
Oct 20, 2018
441cb85
Also drop the py35 support since not all type hint are supported
Oct 20, 2018
4b9007c
Tottaly remove python 3.5
Oct 20, 2018
123f2b1
All backends inherint from the Bcakend base class
Oct 20, 2018
252f455
Fix backends typing
Oct 20, 2018
7019745
Add Backend abstract class test
Oct 20, 2018
c7d465a
merge master
merwok Dec 17, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
__pycache__

# Test artifacts
/.mypy_cache/
/.pytest_cache/
/.coverage
/.tox/
Expand Down
18 changes: 15 additions & 3 deletions configstore/store.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,15 @@
from typing import TypeVar

from .backends.env_var import EnvVarBackend
from .backends.dotenv import DotenvBackend
from .backends.awsssm import AwsSsmBackend
from .backends.docker_secret import DockerSecretBackend


Backends = TypeVar('Backends', EnvVarBackend, DotenvBackend,
AwsSsmBackend, DockerSecretBackend)
Copy link
Contributor

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?

Copy link
Author

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 ?

Copy link
Contributor

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.



class SettingNotFoundException(Exception):
pass

Expand All @@ -7,13 +19,13 @@ class SettingNotFoundException(Exception):

class Store(object):

def __init__(self, backends):
def __init__(self, backends) -> None:
self._backends = tuple(backends)

def add_backend(self, backend):
def add_backend(self, backend: Backends):
self._backends += (backend,)

def get_setting(self, key, default=_no_default):
def get_setting(self, key: str, default=_no_default) -> str:
Copy link
Author

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 ?

Copy link
Contributor

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!!~~'

for backend in self._backends:
ret = backend.get_setting(key)
if ret is None:
Expand Down