Closed glassfordm closed 3 years ago
Thank you for your PR.
There is a design decision issue that I would like to discuss.
By design decouple
should only load from simple unidimensional text files. The .env
text file and .ini
files respect that restriction.
This is because decouple requires symmetry between the text values on environ
and the configuration files.
Adding support for PythonRepository, JsonRepository, DictRepositories violate that design principle. That's why I've rejected PRs with similar ideas before. Do you have any practical use case that would require this features?
Also adding support for MultiConfig doesn't seems useful since you can add blank and commented lines to create sections on the already supported text files. Do you have any practical use case that would require this features?
I saw that you've made some nice cleanups and refactorings that I would very much like to incorporate. But I'm not confident about the new features.
I'm glad you like the cleanups and refactorings. Feel free to use them even if you decide not to use the other changes. I'll explain my motivations for the individual changes below, but the overall summary is that they make decouple
applicable to more project types.
MultiConfig allows you to choose whether or not you want to load settings from environ
(by whether you include '.os'
in the parameter list). If you choose not to, then there's no reason to preserve the symmetry between environ
and the configuration files. Even if you choose to use environ
, I think there are typically only a few settings that most people want to be able to override using an environment variable, and the symmetry wouldn't need to be preserved for the rest. With MultiConfig, if you wanted, you could even be explicit about which settings could be overridden using an environment variable by having two configs, one that allows overriding by by an environment variable and one that doesn't:
config_environ = MultiConfig('.os', 'config.env') # includes environ, uses .env file
config_python = MultiConfig('config.py') # excludes environ, uses python file
SETTING1 = config_environ('SETTING1')
SETTING2 = config_python('SETTING2')
SETTING3 = config_python('SETTING3')
If you decided to adopt PythonRepository and JsonRepository, perhaps the documentation could just make clear that they don't preserve that symmetry, and the consequences of that?
The primary use case for PythonRepository and JsonRepository is that they make it possible to load structured data (such as dictionaries) without encoding it as a string and resorting to the cast parameter to decode the string, a potentially error prone process. The is especially useful for projects that have many structured settings or long ones that would be difficult to encode accurately as as string or read when they have been so encoded
A secondary use case for some people is that, if you use python/json settings exclusively, you don't need the cast parameter at all.
I only created DictRepository because MultiConfig uses it internally, but what I said above could apply to that as well.
This is actually the first addition I made, and its use case is to allow separating settings of different types into separate config files. Specifically, I needed to maintain primary settings files for production, staging, and local environments that are under source control, and another settings file containing passwords and other secrets that is not under source control. MultiConfig allows me to load settings from the appropriate primary settings file, and also from the secrets file.
A further comment on the "Preserving symmetry" issue: it should be possible to add error checking that prevents any problems.
One simple but effective way would be to prohibit the use of typed (PythonRepository, JsonRepository, DictRepository) and untyped (RepositoryOS, RepositoryIni, RepositoryEnv) sources at the same time. This would force the explicit separation code example I used in my previous comment.
Another more flexible, but more complicated and slower, method would be to check for settings that exist in multiple places and make sure they're the same type. If there's a cast function, it could either skip the check (assuming the cast function would take care of it) or call the cast function for each and make sure the results are the same type after the cast.
This pull request adds support for MultiConfig, a class that loads settings from multiple config files so that settings of different types can be stored separately.
It also adds the ability to load settings from Python and JSON config files.
It also refactors all knowledge of environment variables into a single class, instead of being spread between many classes.
I've made no attempt yet to edit the documentation, but have added pretty thorough comments explaining the motivation and usage of the new classes.
There are tests for the new functionality.