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

adding config files as a Configuration argument #80

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

beasteers
Copy link
Contributor

Ok so I figured I would pump this out tonight so that I can continue on with other things tomorrow :)

Summary of things:

  • you can pass custom config directories:
    • Configuration('myapp', source='~/mypkg')
    • Configuration('myapp', source='~/mypkg/settings.yml')
    • Configuration('myapp', source=['~/mypkg', '/etc/mypkg/settings.yml'])
    • environment vars: MYAPPDIR=~/mypkg/, MYAPPDIR=~/mypkg/app.yml
    • config.set_file('~/mypkg', default=True) # ~/mypkg/config_default.yaml
  • I added config_filename and default_filename as arguments as well (cuz why not?)

One bigger change - system files are searched for in __init__ instead of every time config_dir is called. config_dir() will now iterate over the loaded (or unloaded) sources and take the first config that has a filename as the config directory. This means the environment variable must be set before instantiating (which is how one might expect anyways)

I'm open to more discussion about this, but to me it feels a bit unstable to have the detected system config potentially change mid program. This way, the only time config_dir may change is if you call .set().

@sampsyo
Copy link
Member

sampsyo commented Apr 22, 2020

Awesome; this seems like it's on the right track to me. My gut reaction is that changing the time at which we do the search is probably the right thing to do. But what about one small change: the directory creation could still happen in config_dir, i.e., when doing read or when explicitly asking for it.

Without this, unless I'm mistaken, LazyConfig is no longer truly lazy—as soon as you construct it, it will try to touch the filesystem. Beyond LazyConfig itself, it's pretty nice to be able to (optionally) separate the part of the setup where you create the configuration object from the part where you do stuff to files, including reading them and checking whether directories exist.

@beasteers
Copy link
Contributor Author

beasteers commented Apr 22, 2020

True! that's a good point that I hadn't considered. It shouldn't be a difficult thing to do tho.

On a related note, I'm having a little trouble understanding the lazy config. What does splicing sources out of self.sources (into _lazy_prefix, _lazy_suffix) do? I wonder if it'd be possible to make .add() and .set() lazy as well. (Idk if it's necessary but it might simplify things?)

I was thinking that you could extend ConfigSource to encapsulate the data loading so it will load only before you try to access the dict contents (i.e. __getitem__, __iter__, keys, etc.) that way all ConfigSources would be lazy until you need them. I started playing around with it last night to see if it was possible and I had reasonable success. I was able to implement an environment variable source pretty simply as well. I won't paste the full demo script I was working on

but basically to demonstrate usage:

# equivalent
c = ConfigSource.of('asdf.yml')
c = YamlSource('asdf.yml')
# lazy loading
c = ConfigSource.of('asdfa.yml')
print('unloaded', c)
print('a =', c['a'])
print('loaded', c)

outputs

unloaded ConfigSource({}, asdfa.yml, False)
a = 5
loaded ConfigSource({'a': 5, 'b': 6, 'file': 'asdfa.yml'}, asdfa.yml, False)

Sorry I feel weird coming in and proposing a bunch of changes so please feel free to push back if you don't like an idea. 😝

@sampsyo
Copy link
Member

sampsyo commented Apr 24, 2020

On a related note, I'm having a little trouble understanding the lazy config. What does splicing sources out of self.sources (into _lazy_prefix, _lazy_suffix) do?

Good question—this is a little subtle. The idea is to let applications create the in-memory part of their configuration without touching the filesystem, but to have the same effect as loading everything up front.

So if you were to do something like this with the normal, eager Config class:

config = Config('myapp', __name__)
config.set(A)
config.add(B)

Then the resulting "stack" of configuration bits would look like:

A -> user -> default -> B

We want LazyConfig to appear to behave the same way, even though it actually reads the user and default sources later than A and B, which are given up front. The way we do that is by buffering up A in _lazy_prefix and B in _lazy_suffix and then assembling everything together once "materialization" (i.e., actually reading the files) eventually happens. The separate lists are necessary to keep track of where A and B go in the stack.

I was thinking that you could extend ConfigSource to encapsulate the data loading so it will load only before you try to access the dict contents

I like this idea! It's nice in the way that we get to keep track of "loadedness" state locally for a given source rather than globally for the entire configuration. That seems cool! I can't really think. of any downsides with respect to the way that the current LazyConfig works… and we could still provide the LazyConfig class, as just a convenient way to construct all sources lazily.

(It still might be nice to have an eager version, I guess?? At least transitionally? Maybe it would be more predictable in some circumstances?)

Anyway, if you're interested, maybe we should work on that in a separate PR? We could get that right, and then maybe the implementation of this bit could be a little simpler because we don't have to worry about laziness anymore.

@beasteers
Copy link
Contributor Author

Great! I'll submit a PR!

And thanks for the explanation, that makes sense. I was having some trouble with load order so I think that'll be the one thing we need to figure out in this next PR.

It still might be nice to have an eager version, I guess?? At least transitionally? Maybe it would be more predictable in some circumstances?
If you want the sources to be eager, you just call .load() (i.e. ConfigSource.of(file).load()) right away.

@beasteers beasteers mentioned this pull request Apr 24, 2020
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

Successfully merging this pull request may close these issues.

2 participants