-
-
Notifications
You must be signed in to change notification settings - Fork 200
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
Facilitate adding new config-file loaders #470
Conversation
Wherever a config_file.{py|json} would be loaded, also load any/all config in config_file.d/*.{py|json}.
Similar to ipython#360, trying introduce a mew config-loader (e.g. YAML or salt) without this commmit would require to re-implent `Application._load_config_files()`.
>5% speed-up in my PC, from 6.70ms-->6.00ms for all TCs.
…xtensible - Deprecate Application class-attribute: - python_config_loader_class - json_config_loader_class replaced by `supported_cfg_loaders` ordered mapping: {.ext: loader-class} - To extend with a new config-file, simply add its loader class in the `supported_cfg_loaders` mapping. - Merge-conflicts resolved in Application.
traitlets/config/application.py
Outdated
for ext, loader in cls.supported_cfg_loaders.items(): | ||
if name.endswith(ext): | ||
return loader(name, path, log=log) | ||
raise AssertionError("Unknown file-extension in config-file %r!" % name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AssertionErrors shouldn't generally be raised. Probably best to raise KeyError or ValueError. It might also be nice to include the supported extensions in the error message.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, converted to ValueError
.
I did the same fix on an another case using AssertionError
class (missing sub-command).
That's where i copied the code from. It turned out that it was my code in the first place :-)
Since this includes #242, that PR will need to be resolved and this rebased after that before it can be merged. |
What can i do now while waiting #242? |
I think this PR is fine, but since it pulled in #242 which isn't done, it needs to wait until that PR finishes and then be rebased. If it hadn't pulled in that PR, I would be happy to merge it. |
For quite some time I'm actually using a small utility class to load config files, and not traitlet's default code.
I needed this because with multiple source config-paths, that was always the difficult part to debug: Given #473 can detect regressions, is there interest to port this util class in traitlets? |
Closing as state for a few years, apologies for not getting this in. I want to avoid having multiple page on stale PRs on this repository. Labelling accordingly feel free to reopen. |
Rational:
If someone needs to introduce a new config-loader,
(e.g. YAML), currently has must re-implent the full
Application._load_config_files()
,and change just a single line.
Similar to #360 for cmd-line argument loaders,
this PR facilitates introducing new config-loaders without estensive changes in the code.
NOTE: this PR conceptually contans 2 "tips",
one before the merge with @minrk 's #242 and one with it (in case this is merged as well)
Before merge with #242
To introduce a newconfig-loader, simply overridde
Application._make_loaders()
to return an orderd list of loaders.After merge with #242
supported_cfg_loaders: {.ext: loader-class}
ordered mapping.Application
class-attributes:python_config_loader_class
json_config_loader_class
replaced by the
supported_cfg_loaders
mapping:Bonus
A minor speedup is introduced when accesing only once the
HasTrairs.section_names()
outside of_find_my_config()
loop.