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

Update Jupytext Notebook url path #6961

Merged
merged 2 commits into from
Jul 18, 2023
Merged

Update Jupytext Notebook url path #6961

merged 2 commits into from
Jul 18, 2023

Conversation

RRosio
Copy link
Collaborator

@RRosio RRosio commented Jul 6, 2023

Fixes #6942
I believe this addresses the last point in the issue above, as when a Jupytext Notebook is opened it no longer adds the /edit route but rather /notebooks.

@github-actions
Copy link
Contributor

github-actions bot commented Jul 6, 2023

Binder 👈 Launch a Binder on branch RRosio/notebook/jupytext_url

@parmentelat
Copy link
Contributor

cool, thanks @RRosio to haven care of this, please let me know if/when this gets released so I can give it a try

@RRosio RRosio marked this pull request as ready for review July 6, 2023 19:07
@jtpio
Copy link
Member

jtpio commented Jul 17, 2023

@parmentelat were you able to try this change with the built assets for this PR?

@jtpio jtpio added the bug label Jul 17, 2023
@@ -42,7 +42,8 @@ const opener: JupyterFrontEndPlugin<IDocumentWidgetOpener> = {
let route = 'edit';
if (
(widgetName === 'default' && ext === '.ipynb') ||
widgetName === 'Notebook'
widgetName === 'Notebook' ||
widgetName === 'Jupytext Notebook'
Copy link
Member

Choose a reason for hiding this comment

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

Or maybe we can check if widgetName contains the string Notebook?

jupytext is a third-party extension and not officially supported, so not sure we should hardcode its special case here.

@jtpio jtpio added this to the 7.0 milestone Jul 17, 2023
@parmentelat
Copy link
Contributor

parmentelat commented Jul 17, 2023

@jtpio

Hi; unfortunately no, I am mostly away from keyboard these days
plus, I am not completely comfortable testing unreleased code, as my fuzzy understanding of the build process sometimes leads me to wrong conclusions, and so I tend to stick to pip installs as much as I can
So, as much as I'd love to see the issue solved, I'm afraid I am not going to be able to help, at least not for the uncoming release of notebook if that's still the plan

@jtpio
Copy link
Member

jtpio commented Jul 17, 2023

I tend to stick to pip installs as much as I can

Normally the built artifact (wheel) can be installed just like a released version with pip install. There wouldn't be any major differences between unreleased and released code if this PR gets merged and a new release is made right after.

Likely the most important part would be to make sure it is tested in an isolated virtual environment.

@parmentelat
Copy link
Contributor

I made quick test

however with this setup, on matter how I open a jupytext notebook, I still see the /edit/ url

@RRosio
Copy link
Collaborator Author

RRosio commented Jul 17, 2023

With the PR I observed that right-clicking Open with and selecting Jupytext Notebook opens the jupytext notebook with the /notebooks route rather than the /edit route. However when double-clicking the jupytext notebook indeed it goes on to open at the /edit route unlike the classic Notebook which after double-clicking opens a jupytext notebook under the /notebooks route.
Is this a matter of updating Notebook 7 so that the double-click behavior be what it was in classic Notebook?

@jtpio
Copy link
Member

jtpio commented Jul 18, 2023

Is this a matter of updating Notebook 7 so that the double-click behavior be what it was in classic Notebook?

Normally double clicking should open the document with its default factory. Without configuration this seems to still be Editor for a Jupytext Notebook:

image

But testing with this PR and using the user settings provided by @parmentelat in #6914 (comment) to override the default factory, double clicking seems to be opening the file with /notebooks:

jupytext-default-factory-settings.mp4

@RRosio
Copy link
Collaborator Author

RRosio commented Jul 18, 2023

But testing with this PR and using the user settings provided by @parmentelat in #6914 (comment) to override the default factory, double clicking seems to be opening the file with /notebooks

Ah okay, I was missing the user settings. Once using those settings, I too witness that same behavior.

Copy link
Member

@jtpio jtpio left a comment

Choose a reason for hiding this comment

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

Thanks @RRosio for looking into this issue!

The change looks good, and should already improve the case of Jupytext 👍

@jtpio jtpio merged commit 34ca875 into jupyter:main Jul 18, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 19, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

opening jupytext notebook with a base_url is failing
3 participants