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

merge nbserver_extensions #2108

Merged
merged 5 commits into from Feb 2, 2017
Merged

merge nbserver_extensions #2108

merged 5 commits into from Feb 2, 2017

Conversation

@minrk
Copy link
Member

@minrk minrk commented Feb 1, 2017

  • test
  • handle config_file_name and config_dir Application settings

Load with ConfigManager so that merge happens recursively, unlike normal config values.

Makes loading more consistent with frontend extensions, which is good.

It is a bit icky because the same files are loaded and read twice, and one key in a traitlets-defined config file is loaded differently than all others.

This does behave more like we intend and people expect for extensions, though.

closes #2063

Load with ConfigManager so that merge happens recursively,
unlike normal config values.

Makes loading more consistent with frontend extensions,
but is a bit icky because the same files are loaded and read twice,
and one key in a traitlets-defined config file is loaded differently than all others.

This does behave more like we intend and people expect for extensions, though.
minrk added 2 commits Feb 1, 2017
and refactor mock test environment a bit

ensures no system config files loaded or affected
- unused imports
- call jupyter_path instead of reimplementing it
@minrk minrk force-pushed the minrk:merge-server-extensions branch from 75df326 to e70e7be Feb 1, 2017
@minrk minrk added this to the 5.0 milestone Feb 1, 2017
# This enables merging on keys, which we want for extension enabling,.
# Regular config loading only merges at the class level,
# so each level (use > env > system) clobbers the previous.
manager = ConfigManager(read_config_path=jupyter_config_path())

This comment has been minimized.

@blink1073

blink1073 Feb 1, 2017
Member

This will not pick up any overrides given to the NotebookApp - config_file_name, config_dir.

This comment has been minimized.

@minrk

minrk Feb 1, 2017
Author Member

Good call, and further evidence that this is possibly too fiddly to be worth doing in-place.

This comment has been minimized.

@minrk

minrk Feb 1, 2017
Author Member

Should work with config_dir and config_file_name now, good catch.

@minrk minrk changed the title Merge nbserver_extensions [wip] merge nbserver_extensions Feb 1, 2017
could be important for subclasses
@minrk minrk changed the title [wip] merge nbserver_extensions merge nbserver_extensions Feb 1, 2017
if self.nbserver_extensions[modulename]:

# Load server extensions with ConfigManager.
# This enables merging on keys, which we want for extension enabling,.

This comment has been minimized.

@blink1073

blink1073 Feb 2, 2017
Member

Extra comma at the end

This comment has been minimized.

@Carreau

Carreau Feb 2, 2017
Member

removed.

# Load server extensions with ConfigManager.
# This enables merging on keys, which we want for extension enabling,.
# Regular config loading only merges at the class level,
# so each level (use > env > system) clobbers the previous.

This comment has been minimized.

This comment has been minimized.

@Carreau
Copy link
Member

@Carreau Carreau commented Feb 2, 2017

Thanks Steve !

@Carreau Carreau merged commit 90d30d2 into jupyter:master Feb 2, 2017
4 checks passed
4 checks passed
codecov/patch 98.61% of diff hit (target 0%)
Details
codecov/project 76.96% (+0.17%) compared to ad805b1
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@gnestor gnestor added this to Merged PRs in 5.0 Feb 4, 2017
@minrk minrk deleted the minrk:merge-server-extensions branch Feb 6, 2017
@Madhu94
Copy link
Contributor

@Madhu94 Madhu94 commented Oct 27, 2017

@minrk @Carreau @blink1073 This was supposed to merge nbserver_extensions for jupyter_notebook_config.py files as well, not just jupyter_notebook_config.json files ?

@blink1073
Copy link
Member

@blink1073 blink1073 commented Oct 27, 2017

Looking down the rabbit hole it looks like it only applies to .json files.

@Madhu94
Copy link
Contributor

@Madhu94 Madhu94 commented Oct 27, 2017

Thanks @blink1073! Was that done on purpose ? Any reason the notebook doesn't do the merge for nbserver_extensions in .py files ?

@blink1073
Copy link
Member

@blink1073 blink1073 commented Oct 27, 2017

I'm not sure if that was a conscious decision historically or not, @minrk would be the best person to know for sure.

@minrk
Copy link
Member Author

@minrk minrk commented Oct 30, 2017

jupyter nbextension enable/disable always happens in .json files, so that's the mechanism by which this is generally happening. Including .py files would further complicate this already complicated band-aid. My answer would be that nbextension config shouldn't ever occur in .py config files because of this special handling.

.py config files also already have a clearer, explicit merge mechanism, so shouldn't have to get mixed up in this:

c.Class.trait.update({'x': 5}) # update existing config, if any
# vs
c.Class.trait = {'x': 5} # override
@Madhu94
Copy link
Contributor

@Madhu94 Madhu94 commented Nov 6, 2017

Thanks @minrk

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
5.0
Merged PRs
Linked issues

Successfully merging this pull request may close these issues.

4 participants
You can’t perform that action at this time.