Replace template string {resource_dir} in kernelspec #932
Conversation
* Adds anonymous users * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * removes unused import * Removes random colors Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Zachary Sailer <zsailer@apple.com>
… compatibility with windows
Codecov Report
@@ Coverage Diff @@
## main #932 +/- ##
==========================================
- Coverage 72.33% 72.31% -0.03%
==========================================
Files 65 65
Lines 7997 8015 +18
Branches 1335 1338 +3
==========================================
+ Hits 5785 5796 +11
- Misses 1803 1811 +8
+ Partials 409 408 -1
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Hi @mlhenderson - I apologize for the delayed review on this PR.
This incantation seems a bit less risky than #818 - although I do have to question my previous comment regarding the addition of a resource_dir field to the kernelspec model as constituting an API change. Seems like additional fields should not be considered breaking changes, but I'm not sure I've been around Jupyter long enough to know the precedent. Perhaps others can shed some insight on that relative to past changes (although its moot if we pursue this PR)?
| def update_kernelspec_model(spec_dict): | ||
| """Returns the original kernelspec unless template substitutions are available.""" | ||
| model = spec_dict | ||
| if "resource_dir" in spec_dict: |
There was a problem hiding this comment.
Not sure when this will be true since it's asking if the kernelspec model contains the key "resource_dir" yet, in the models, resource_dir is used to identify the other resources of the kernel (icon, etc.) and not retained as a key/value. As a result, this logic is probably better suited in kernelspec_model() directly.
I don't think this resource_dir-substitution logic applies to spec_dicts that satisfy the is_kernelspec_model() check because, for those, the water is under the bridge and the resource_dir is not available (nor is it local). The specs returned from a Gateway server are examples of those.
| template_found = "{resource_dir}" in spec_str | ||
| resource_dir_exists = os.path.exists(spec_dict["resource_dir"]) | ||
| if template_found and resource_dir_exists: | ||
| spec_str = spec_str.replace("{resource_dir}", spec_dict["resource_dir"]) |
There was a problem hiding this comment.
This change will result in the substitution of {resource_dir} with the local value (see the previous comment about non-local specs returned from a Gateway) which typically occurs at the time of launch in jupyter_client.
(I don't see any issue with that but just wanted to make that comment in case others do.)
This is an alternate pull request to #818, which added a resource_dir field to the response output. Instead, this PR replaces the template string of {resource_dir} with the provided resource_dir path string, if one exists.