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

Give kernel providers opportunity to load configuration #20

Merged
merged 1 commit into from
Aug 27, 2019
Merged

Give kernel providers opportunity to load configuration #20

merged 1 commit into from
Aug 27, 2019

Conversation

kevin-bates
Copy link
Collaborator

This change adds a new method to KernelProviderBase names load_config(). When KernelFinder initializes itself with a set of providers, it calls this method on each provider passing the hosting application's Config instance, if available. If an application is not currently available or not initialized, the method is still called with config=None in cases where providers may not be hosted within an application and/or can perform their own loading of configuration - however that is defined by the provider.

Fixes #17

@takluyver
Copy link
Owner

My thinking was that the application would pass in its config to KernelFinder, rather than relying on getting the global application singleton. No strong preference, though.

@kevin-bates
Copy link
Collaborator Author

Thanks for the comment. I don't have a strong preference either, although I think that if we left it up to the hosting application, then for those applications that pass config=None (or don't specify a parameter) in KernelFinder's constructor, it seems the unconditional call on the provider's load_config() would be less intuitive. Also, because 'config' in this case is not just a dictionary (but rather a Config class instance), I think it's less error prone to manage that ourselves. That said, I could be swayed by counter arguments as well.

@codecov-io
Copy link

codecov-io commented Aug 16, 2019

Codecov Report

Merging #20 into master will increase coverage by 0.59%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #20      +/-   ##
==========================================
+ Coverage   70.45%   71.05%   +0.59%     
==========================================
  Files          27       27              
  Lines        2004     2045      +41     
==========================================
+ Hits         1412     1453      +41     
  Misses        592      592
Impacted Files Coverage Δ
jupyter_kernel_mgmt/tests/test_discovery.py 95.68% <100%> (+1.71%) ⬆️
jupyter_kernel_mgmt/discovery.py 64.22% <100%> (+2.83%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5cc31f6...9e37f57. Read the comment docs.

@takluyver takluyver merged commit 4902927 into takluyver:master Aug 27, 2019
@kevin-bates kevin-bates deleted the convey-config branch August 27, 2019 17:16
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.

Add ability to convey application configuration to providers
3 participants