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 path option to Configuration class #38

Open
lundybernard opened this issue Aug 8, 2024 · 3 comments · May be fixed by #45
Open

Add path option to Configuration class #38

lundybernard opened this issue Aug 8, 2024 · 3 comments · May be fixed by #45

Comments

@lundybernard
Copy link
Owner

The Configuration class currently relies on the config_class __module__ attribute for its namespacing.
While this is a reasonable default/fallback, esp. for simple modules, It forces some undesirable constraints on the structure of our configuration.

Proposed improvement:

Add a path parameter to Configuration.
It should replace the _mod_ property.

  • Ensure that, given a Config Dataclass, get_config can find its path, and retrieve values using it. This may require adding a __path attribute to the dataclass.

  • Optional validation: Ensure the config_class path matches the path+name of the parent config.

@lundybernard
Copy link
Owner Author

This is turning into a major change to the way configuration structure is defined.

Originally, we wanted the ability to get the config for sub-modules directly: sub_cfg = get_config(SubModuleConfigClass)
To do this, the Configuration needs to know the path to the sub-module in the configuration sources, so it can call source_list.get(key='key', path='path.to.submodule')
We chose to bind the path to the sub-module's actual module name __module__, this has the advantages that we do not need to pass a path parameter to get_config or the Configuration constructor. The disadvantage is that it forces us to replicate the module path in the structure of the config sources.

@lundybernard
Copy link
Owner Author

lundybernard commented Aug 18, 2024

Discovered a related bug:

When a Configuration instances contains a sub-Configuration, accessing the sub-config should return a Configuration instance. It currently returns the Config dataclass.

Edit: fixed in dev branch.

@lundybernard
Copy link
Owner Author

We need to decide on our priority between performance and type-checking. The more stream-lined implementation confuses the type-checker, and rewriting it to be both type-safe and fast will require significantly more work. for now i'm in favor of performance, and quieting the type-checker.

Changes to the Configuration class mean we can probably drop the DataclassConfig configuration source, which has always been a little awkward. Since all of the config dataclasses to be included in the config tree are included when we create the configuration object, we can extract the default values when we build the configuration and return them if the source lookup fails. This implementation should be much easier to read and understand than the dataclass config source.

lundybernard added a commit that referenced this issue Aug 19, 2024
Fixes: #38

* The Configuration class now handles default values set in Config
dataclasses.  As a result, we no longer need the DataclassConfig source
to lookup default values.
* Improve Configuration repr for paths and child-configs
* Remove DataclassConfig from example code and docs
@lundybernard lundybernard linked a pull request Aug 19, 2024 that will close this issue
lundybernard added a commit that referenced this issue Aug 20, 2024
Fixes: #38

* The Configuration class now handles default values set in Config
dataclasses.  As a result, we no longer need the DataclassConfig source
to lookup default values.
* Improve Configuration repr for paths and child-configs
* Remove DataclassConfig from example code and docs
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 a pull request may close this issue.

1 participant