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

[WIP] Add conf.d config-loading #242

Closed
wants to merge 1 commit into from
Closed

Conversation

minrk
Copy link
Member

@minrk minrk commented Jun 10, 2016

Wherever a config_file.{py|json} would be loaded, also load any/all config in config_file.d/*.{py|json}.

This is a prototype for discussion in jupyter/notebook#1508

closes #192

if log:
log.debug("Looking for %s in %s", basefilename, path)
jsonloader = cls.json_config_loader_class(basefilename+'.json', path=path, log=log)
# path list is in descending priority order, so load files backwards:
def new_loader(name, path):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No reason to define new_loader inside the for-loop over path[::-1] on line 537

Wherever a config_file.{py|json} would be loaded, also load any/all config in config_file.d/*.{py|json}.
@Madhu94
Copy link

Madhu94 commented Oct 11, 2017

@minrk This change hasn't been merged yet ? Is it because we don't need it in light of jupyter/notebook#2108 ?

@minrk
Copy link
Member Author

minrk commented Oct 30, 2017

I think we still do want this in general, but it is no longer an urgent need thanks to that PR.

@bollwyvl
Copy link
Contributor

bollwyvl commented Dec 7, 2017

Raised some questions about py/json vs env/home precendence. Would love to see this finally put to rest!

jsonloader = cls.json_config_loader_class(basefilename+'.json', path=path, log=log)
pyloader = new_loader(basefilename + '.py', path=path)
jsonloader = new_loader(basefilename + '.json', path=path)
loaders = [pyloader, jsonloader]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a question not strictly related to the core functionality of this PR
Why did you changed the separate cls.XXX_config_loader_class() calls with new_loader()?

I also need to change the abov 3 lines,
but for adding a new loader (i.e. YAML) without overriding and reimplementing _load_config_files(),
like that:

-            pyloader = cls.python_config_loader_class(basefilename+'.py', path=path, log=log)
             if log:
                 log.debug("Looking for %s in %s", basefilename, path or os.getcwd())
-            jsonloader = cls.json_config_loader_class(basefilename+'.json', path=path, log=log)
+           loaders = cls._make_loaders(basefilename, path=path, log=log)
+           log.debug("Looking for %s in %s", basefilename, path or os.getcwd())

You may implement _make_loaders() either with new_loader() or cls.XXX_config_loader_class() calls.
Then adding a new loader requires only to overrd this trivial method.

ankostis added a commit to ankostis/traitlets that referenced this pull request Feb 4, 2018
…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.
ankostis added a commit to ankostis/traitlets that referenced this pull request Feb 4, 2018
…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.
@minrk minrk modified the milestones: wishlist, 5.0 Feb 9, 2018
ankostis added a commit to ankostis/polyvers that referenced this pull request Feb 13, 2018
ankostis added a commit to ankostis/polyvers that referenced this pull request Feb 14, 2018
@Carreau
Copy link
Member

Carreau commented May 31, 2020

Closing as state for 2 years Labelling accordingly to easily reopen if necessary. Will try to do some cleanup to not have multiple pages of stale PR.

@Carreau Carreau added the Closed PR Stalled PR, feel free to resurrect. label May 31, 2020
@Carreau Carreau closed this May 31, 2020
@Carreau Carreau removed this from the 5.0 milestone Jun 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Closed PR Stalled PR, feel free to resurrect.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

conf.d-style loading of a bag of files
6 participants