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

Config loader needs callable #69

Open
jb098 opened this issue Aug 29, 2019 · 1 comment
Open

Config loader needs callable #69

jb098 opened this issue Aug 29, 2019 · 1 comment

Comments

@jb098
Copy link
Collaborator

jb098 commented Aug 29, 2019

Currently the config load expects a callable which seems to prevent it from loading in something as simple as a dictionary, which seems better.

I feel like we should be checking whether it's a callable or a dict as I think these are the two most common cases and loading in accordingly.

I'm looking at you for feedback on this one @aecay .

@aecay
Copy link
Collaborator

aecay commented Sep 4, 2019

IMO, we should require it to be something that implements the Mapping interface. That would include dictionaries, but not callables. However, we only need the callable for default_config_loader (in config.py). We can rewrite that as a Mapping. (And in general, any hypothetical 3rd party callable could be so rewritten). Then we don't need to typecheck anything and can just use the methods provided by the Mapping interface.

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

No branches or pull requests

2 participants