Fix editable install and update CONTRIBUTING.md #391
Conversation
Using install:extension can lead to nested pip install . situtation
|
Thanks for submitting your first pull request! You are awesome! 🤗 |
There was a problem hiding this comment.
Thank you @mahendrapaipuri for helping out resolving this!!! ❤️ 🎉 🌻
I made some comments favoring reducing code complexity by hardcoding a labextension name not expected to change. Besides that, I think this LGTM!
@bollwyvl being a jupyterlab savvy person, does this look good to you?
jupyter_server_proxy/__init__.py
Outdated
| def _jupyter_labextension_paths(): | ||
| return [{ | ||
| "src": "labextension", | ||
| "dest": data["name"] |
There was a problem hiding this comment.
This is not something we should be changing, so I figure we don't need to have logic to import this name.
| "dest": data["name"] | |
| "dest": "@jupyterhub/jupyter-server-proxy", |
jupyter_server_proxy/__init__.py
Outdated
| HERE = Path(__file__).parent.resolve() | ||
|
|
||
| with (HERE / "labextension" / "package.json").open() as fid: | ||
| data = json.load(fid) | ||
|
|
There was a problem hiding this comment.
| HERE = Path(__file__).parent.resolve() | |
| with (HERE / "labextension" / "package.json").open() as fid: | |
| data = json.load(fid) |
jupyter_server_proxy/__init__.py
Outdated
| import json | ||
| from pathlib import Path | ||
|
|
There was a problem hiding this comment.
| import json | |
| from pathlib import Path |
| [tool.hatch.build.hooks.jupyter-builder.editable-build-kwargs] | ||
| path = "labextension" | ||
| build_cmd = "install:extension" | ||
| build_cmd = "build" |
There was a problem hiding this comment.
I'm not 100% confident on the change from install:extension to build, but I see this be done also in another project and Jeremy that made the that change is experienced with these matters.
Do you think we should have force = true as well as they have in that project? I think it relates to forcing rebuild of the labextension if it already exists.
But, our contributing docs mentions jlpm watch, which seem to trigger build of this also... Overall I'm not at all confident here, but I've seen this to work and the previous to not work at all.
I feel confident this is an incremental improvement, so I'm okay with a merge.
There was a problem hiding this comment.
I have been working on JupyterLab extensions for the past few months and I have noticed that build is used as a standard for editable installs in different extension sources and cookie-cutter examples. But I am happy to get feedback from JupyterLab developers.
I think we can set force = true without any side effect. Yes, jlpm run watch is more convenient way to develop extensions as every change to source code will be compiled in real time.
pyproject.toml
Outdated
| artifacts = [ | ||
| "jupyter_server_proxy/labextension", | ||
| ] | ||
|
|
There was a problem hiding this comment.
If we are hard coding the extension name, we do not need to distribute the labextension files in the package as we do not need to read package.json file anymore
|
@mahendrapaipuri this now seems to me like a very clear improvement, I'll go for a merge here now! Thank you!!! ❤️ 🎉 🌻 |
|
Congrats on your first merged pull request in this project! 🎉 |
2 participants
EDIT by Erik: for a background of changes, see #389 (comment)