search

jupyter / nbclassic

Jupyter Notebook as a Jupyter Server extension
https://nbclassic.readthedocs.io/en/latest
BSD 3-Clause "New" or "Revised" License
73 stars 62 forks source link

Remove redirect from /notebooks to /files #235

Closed mwouts closed 1 year ago

mwouts commented 1 year ago

This redirect causes an issue with the Jupytext extension, see https://github.com/mwouts/jupytext/issues/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).

yuvipanda commented 1 year ago

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

mwouts commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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.

mwouts commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

matthew-brett commented 1 year ago

Thanks much!

echarles commented 1 year ago

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

mwouts commented 1 year ago

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