-
Notifications
You must be signed in to change notification settings - Fork 391
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
adding jupyterlab file paths for preview #925
Conversation
72a4a27
to
a98cfed
Compare
binderhub/main.py
Outdated
|
||
# Check if we have a JupyterLab + file path, if so then use it for the filepath | ||
urlpath = self.get_argument('urlpath', '').lstrip('/') | ||
if 'lab/tree/' in urlpath: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if 'lab/tree/' in urlpath: | |
if urlpath.startswith("lab/tree"): |
would this be an improvement/less prone to matching accidentally?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was originally doing it this way to allow for workspaces in the url too, but upon looking it seems like that'd actually be a more complicated rule. What if instead we did something like:
if urlpath.startswith("lab") and `/tree/` in urlpath:
and then do the split on /tree/
and take whatever is to the right of the first instance of /tree/
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is now in the latest push, it seemed to work for me locally (both regular lab URLs and workspace URLs)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What different kinds of URLs could we get here? I am not really familiar with what Jupyter Lab does/what workspaces are :-/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See the link I sent above, that's what I was using to figure out what urls to expect
Merge? If you've tested it locally I'd be happy to press merge and see what happens. Investigating and learning about URL schemes and how they could go wrong seems like a high effort thing compared to the worst that could happen if we just deploy it. |
I'm +1 on merging, the only thing that can go wrong is that it doesn't work, which would be our current situation :-) |
closes #645
I think that this should add support for JupyterLab paths in the nbviewer preview.
It is pretty simple, basically just adds a few lines to:
urlpath
is provided in the URLlab/tree/
exists in theurlpath
filepath
to whatever is on the right side oflab/tree/
.I think this should work for at least vanilla jupyterlab paths. It might break in unpredicted ways if people start using jupyterlab but also other kinds of URL patterns, but we can beef up this function when we get to those situations.