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.

Sign up for GitHub

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

Jump to bottom

Remove redirect from /notebooks to /files #235

Merged
merged 1 commit into from
Mar 30, 2023
Merged

Remove redirect from /notebooks to /files #235

merged 1 commit into from
Mar 30, 2023

Conversation

mwouts
Copy link
Contributor

@mwouts mwouts commented Mar 26, 2023 •
edited
Loading

This redirect causes an issue with the Jupytext extension, see mwouts/jupytext#1051.

Because of the redirect, text notebooks are downloaded in nbclassic , while the expected behavior would be to have them open as notebooks.

@jtpio do you think the redirect is still needed? I have made a quick test locally which seemed to suggest that it was not (I could click on the Edit button and get the text file open in the text editor - but that was with Jupytext installed).

This was referenced Mar 28, 2023
Closed
Closed
@yuvipanda
Copy link
Contributor

Thank you so much for digging into this and making this PR, @mwouts!

@jtpio jtpio requested a review from echarles March 28, 2023 08:01
@mwouts
Copy link
Contributor Author

mwouts commented Mar 28, 2023

Thank you so much for digging into this and making this PR, @mwouts!

You're welcome @yuvipanda ! Thank you in return for your own help. And actually I would have been completely clueless without @GeorgianaElena's comment - she was the one who spotted the issue - Thank you both!

echarles
echarles approved these changes Mar 28, 2023
View reviewed changes
Copy link
Member

@echarles echarles left a comment

Choose a reason for hiding this comment

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

LGTM. Thx @mwouts @yuvipanda @GeorgianaElena

@echarles
Copy link
Member

Oops, I have approved too quickly.

I have tested the branch on my local env and the .py files are now correctly opened in the browser, but the playwright tests are all failing with the same error (it was not before):

FAILED nbclassic/tests/end_to_end/test_save_readonly_as.py::test_save_readonly_as - playwright._impl._api_types.TimeoutError: Timeout 30000ms exceeded.
=========================== logs ===========================
waiting for locator("body")
  locator resolved to <body dir="ltr" data-ws-url="" data-base-url="/a%40b/"…>…</body>

Can you check if this could come from the changes of this PR?

@echarles
Copy link
Member

I have double checked with @ericsnekbytes who has worked recently on the tests part of nbclassic, and it looks like the failures are encountered in other PRs under flaky circunstances.

I will merge. Thx all.

@echarles echarles merged commit 8313c3d into jupyter:main Mar 30, 2023
@mwouts
Copy link
Contributor Author

mwouts commented Mar 30, 2023

Thank you @echarles . Sorry I gave a try to the testing system but that was all very new to me so I did not get very far (but the journey looked very interesting!). Thank you for merging!

@matthew-brett
Copy link

Sorry to ask - but any chance of a release with this fix? @mwouts kindly wrote up a workaround that I am using, but it would be good to go back to a released version.

@echarles
Copy link
Member

echarles commented Apr 4, 2023

@matthew-brett thx for asking. Sure, I will plan a release for tomorrow and will let you know when it is done.

@matthew-brett
Copy link

Thanks much!

@echarles
Copy link
Member

echarles commented Apr 5, 2023 •
edited
Loading

@matthew-brett nbclassic 0.5.5 is released, hope it works for you.

@mwouts
Copy link
Contributor Author

mwouts commented Apr 5, 2023

Thank you @echarles . I confirm that the new release works for me. The corresponding change on my Binder configuration is this one: https://github.com/mwouts/jupytext/pull/1056/files

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@echarles echarles echarles approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mwouts @yuvipanda @echarles @matthew-brett