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

Add shortcuts? #27

Open
merwok opened this issue Oct 10, 2018 · 5 comments
Open

Add shortcuts? #27

merwok opened this issue Oct 10, 2018 · 5 comments

Comments

@merwok
Copy link
Contributor

merwok commented Oct 10, 2018

Would it be useful to add syntax shortcuts to make client code shorter?

store = configstore.Store()
store += configstore.EnvVarBackend()

if something:
    store += configstore.DotEnvBackend()

DEBUG = store['DEBUG']
DATABASES = {'default': dj_database_url.parse(store['DATABASE_URL'])}
@merwok
Copy link
Contributor Author

merwok commented Oct 10, 2018

@just1602 @JulienLabonte In general I prefer something explicit like store.add_backend(SomeBackend()) but I think += could be okay here; what do you think?

@just1602
Copy link

I'm not sure if I like that. I'm a big fan of clean method name like add_backend and keep operator for mathematical stuff. Like if we implement a class representing a matrix, it'd be nice to have the operator implementation, but for this case I don't know if I like it. But we could implement it since some people could use it and it'd be a good practice for the don't repeat yourself (DRY) principle. :)

@merwok
Copy link
Contributor Author

merwok commented Oct 11, 2018

I think it’s okay that classes that are not related to math concepts redefine operators to mean something meaningful for them (e.g. str + str, Path / str).

What I don’t like is that store += backend is equivalent to store = store + backend, and that does not make sense (compared to list1 += list2 <=> list1 = list1 + list2).

For the other shortcut, I don’t like that store['DEBUG'] looks like a direct access to local data but may actually involve I/O and AWS requests. (Same design rule that says a property should be a direct access, but a big computation or remote request needs a method call.) I suppose I would feel differently if instantiating the backend did all the I/O, and store[setting] only looked up in local data, but that’s not (except for DotEnvBackend!) the design we have now (and probably not one we want).

I do like add_backend, which is clear and unambiguous (compared to e.g. store.add). For get_setting, instead of dict item access maybe we could rename the method to get? (the similarity with dict.get is actually a pro, since we have equivalent signature and behaviour (default value or exception))

@merwok
Copy link
Contributor Author

merwok commented Oct 24, 2018

Other ideas for shortcuts:

store = configstore.Store(envvar=True)

if something:
    store.add_backend(dotenv='/path/to/.env')
# the dotenv backend will be added if envvar PROJECT_DOTENV_PATH exists
store = configstore.Store(envvar=True, dotenv='PROJECT_DOTENV_PATH')

# built-in backend
store.add_backend('docker')
# custom backend
store.add_backend(VaultBackend)
store = configstore.Store('envvar, dotenv:PROJECT_DOTENV_PATH')

@merwok
Copy link
Contributor Author

merwok commented Oct 27, 2018

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

No branches or pull requests

2 participants