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

Re-use ServerApp.config_file_paths for consistency #715

Merged
merged 2 commits into from
Mar 10, 2022

Conversation

minrk
Copy link
Contributor

@minrk minrk commented Mar 10, 2022

Rather than duplicating its logic, re-use the value. JupyterHub overrides this in the base ServerApp (jupyterhub/jupyterhub#3804), but the overridden value should be used for both:

  1. locating extensions,
  2. when extensions look for their own config files

@minrk
Copy link
Contributor Author

minrk commented Mar 10, 2022

Wait, better idea. all set.

This property in the base class already has the same logic to add config_dir to jupyter_config_path

JupyterHub overrides this to have slightly different logic, and the same should apply here
@codecov-commenter
Copy link

codecov-commenter commented Mar 10, 2022

Codecov Report

Merging #715 (c19aad8) into main (426a4c6) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #715      +/-   ##
==========================================
- Coverage   70.84%   70.82%   -0.02%     
==========================================
  Files          62       62              
  Lines        7490     7486       -4     
  Branches     1187     1186       -1     
==========================================
- Hits         5306     5302       -4     
  Misses       1829     1829              
  Partials      355      355              
Impacted Files Coverage Δ
jupyter_server/serverapp.py 65.38% <100.00%> (-0.13%) ⬇️

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 426a4c6...c19aad8. Read the comment docs.

@minrk minrk changed the title Allow override of server_extension_path Use app.config_file_path to locate server extensions Mar 10, 2022
base ServerApp should have total control over config file loading
@minrk minrk changed the title Use app.config_file_path to locate server extensions Re-use ServerApp.config_file_path for consistency Mar 10, 2022
@minrk minrk changed the title Re-use ServerApp.config_file_path for consistency Re-use ServerApp.config_file_paths for consistency Mar 10, 2022
@blink1073 blink1073 added the bug label Mar 10, 2022
Copy link
Collaborator

@blink1073 blink1073 left a comment

Choose a reason for hiding this comment

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

LGTM!

@Zsailer
Copy link
Member

Zsailer commented Mar 10, 2022

Thanks, @minrk! Test failures appear unrelated.

@Zsailer Zsailer merged commit 653740c into jupyter-server:main Mar 10, 2022
blink1073 added a commit to blink1073/jupyter_server that referenced this pull request Mar 14, 2022
@minrk minrk deleted the server_extension_paths branch March 14, 2022 12:07
@minrk minrk mentioned this pull request Mar 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants