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

Defer preferred_dir validation until root_dir is set #826

Merged

Conversation

kevin-bates
Copy link
Member

When one trait depends on another for its validity, both traits should be loaded prior to the validation of the dependent trait. Such a relationship occurs between preferred_dir and root_dir, where preferred_dir must reside within the hierarchy of root_dir. However, when preferred_dir is defined via a CLI parameter (e.g., --ServerApp.preferred_dir = /foo), its validation will occur prior to the finalization of root_dir if root_dir is defined in a configuration file (since CLI traits are validated prior to those loaded from config files). In such cases, we need to defer the validation of preferred_dir (relative to the value of root_dir) until root_dir is validated. This pull request detects when preferred_dir is set via the CLI and, in such cases, defers its validation until root_dir is validated OR the application's superclass is initialized. This latter scenario will occur if the default value of root_dir is used (i.e., current working directory).

In the course of testing, it turns out that there's no way to test various configuration locations of root_dir since the pytest fixtures pass this trait directly (so even the default value can't be used). This pull request introduces the ability to configure root_dir by detecting it has a None value and, in such cases, removes it from the keyword arguments.

Resolves: #806

@codecov-commenter
Copy link

codecov-commenter commented May 2, 2022

Codecov Report

Merging #826 (25144f4) into main (196ee87) will increase coverage by 0.12%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #826      +/-   ##
==========================================
+ Coverage   70.09%   70.21%   +0.12%     
==========================================
  Files          63       63              
  Lines        7480     7494      +14     
  Branches     1253     1258       +5     
==========================================
+ Hits         5243     5262      +19     
+ Misses       1855     1851       -4     
+ Partials      382      381       -1     
Impacted Files Coverage Δ
jupyter_server/pytest_plugin.py 88.18% <100.00%> (+0.10%) ⬆️
jupyter_server/serverapp.py 65.31% <100.00%> (+0.39%) ⬆️
jupyter_server/services/kernels/handlers.py 58.51% <0.00%> (+1.06%) ⬆️

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 196ee87...25144f4. Read the comment docs.

Copy link
Contributor

@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, thanks!

@blink1073 blink1073 merged commit 283c41c into jupyter-server:main May 3, 2022
@echarles
Copy link
Member

echarles commented May 3, 2022

@meeseeksdev please backport to 1.x

meeseeksmachine pushed a commit to meeseeksmachine/jupyter_server that referenced this pull request May 3, 2022
blink1073 pushed a commit that referenced this pull request May 3, 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.

preferred_dir bug
5 participants