Save as improvements

[andrewfulton9]

trafficstars

Fixes for #266

Adds functionality for inferring new notebook name from current notebook and creating directories from "save as".

No backwards incompatibilities as far as I am aware.

https://github.com/jupyter/nbclassic/assets/13774419/48344170-025b-4afb-8312-a7f1b2a27cab

[andrewfulton9]

I am running into issues running tests both locally and in CI on my fork that I am unsure how to fix. I am unsure if they will replicate in the CI here.

[RRosio]

Thank you for submitting this PR @andrewfulton9, the new CI failures that are present seem to be the Linux JS Tests which I believe could be failing due to a change in the JS dependencies. I am trying to pinpoint which dependency could be the . Locally, I also see the test_save_readonly_as.py failing, but this test fails locally in latest main branch as well and iirc is also the reason for the Playwright test failures in the latest main branch CI run.

I am wondering, after trying some different paths, using the new function, makeParentDirectory, how can we inform users when the path they are attempting to save the notebook to is not a valid path? From my local testing, at one point when I entered a path, I was returned to the prompt for the path after clicking the create button, but saw no message in the UI.

[andrewfulton9]

Thanks for taking a look at this @RRosio!

Thank you for submitting this PR @andrewfulton9, the new CI failures that are present seem to be the Linux JS Tests which I believe could be failing due to a change in the JS dependencies. I am trying to pinpoint which dependency could be the . Locally, I also see the test_save_readonly_as.py failing, but this test fails locally in latest main branch as well and iirc is also the reason for the Playwright test failures in the latest main branch CI run.

I agree that this seems to be an issue with JS dependencies. I have been trying to figure out a fix, but have yet to find one. I was able to get the tests to go through CI by hard pinning the all the versions, but oddly this isn't working for me locally. Once I get that figured out I will fix and update test_save_readonly_as.py to handle testing the cases, but I'll admit I'm pretty stuck on this dependency issue. What would the next step be for the dependency issue? Should another issue be opened to specifically address that?

I am wondering, after trying some different paths, using the new function, makeParentDirectory, how can we inform users when the path they are attempting to save the notebook to is not a valid path? From my local testing, at one point when I entered a path, I was returned to the prompt for the path after clicking the create button, but saw no message in the UI.

Good catch. So if an invalid path is given inmakeParentDirectory, it's currently just failing silently. Am I understanding that correctly?

[RRosio]

I was able to get the tests to go through CI by hard pinning the all the versions, but oddly this isn't working for me locally.

Have you by any chance cleared the bower cache located in ~/.cache/bower? I remember experiencing issues locally due to some of the package versions cached in this location. I can give it a try locally as well!

Should another issue be opened to specifically address that?

That is a good idea, I went ahead and opened https://github.com/jupyter/nbclassic/issues/268.

So if an invalid path is given inmakeParentDirectory, it's currently just failing silently. Am I understanding that correctly?

Yes that is correct!

[RRosio]

Closing and reopening to rerun CI checks

[RRosio]

Closing and reopening to rerun CI checks Pressed the wrong button.

[krassowski]

@andrewfulton9 it looks like the only failure in Playwright tests is in test_save_as_nb so that sounds related to the changes in this PR (FYI on main branch all tests are passing with green status):

2024-05-08T19:11:01.0775134Z =================================== FAILURES ===================================
2024-05-08T19:11:01.0776443Z _______________________________ test_save_as_nb ________________________________
2024-05-08T19:11:01.0777452Z Traceback (most recent call last):
2024-05-08T19:11:01.0779217Z   File "/opt/hostedtoolcache/Python/3.7.17/x64/lib/python3.7/site-packages/_pytest/runner.py", line 341, in from_call
2024-05-08T19:11:01.0780068Z     result: Optional[TResult] = func()
2024-05-08T19:11:01.0781011Z   File "/opt/hostedtoolcache/Python/3.7.17/x64/lib/python3.7/site-packages/_pytest/runner.py", line 262, in <lambda>
2024-05-08T19:11:01.0781893Z     lambda: ihook(item=item, **kwds), when=when, reraise=reraise
2024-05-08T19:11:01.0782907Z   File "/opt/hostedtoolcache/Python/3.7.17/x64/lib/python3.7/site-packages/pluggy/_hooks.py", line 433, in __call__
2024-05-08T19:11:01.0783823Z     return self._hookexec(self.name, self._hookimpls, kwargs, firstresult)
2024-05-08T19:11:01.0784894Z   File "/opt/hostedtoolcache/Python/3.7.17/x64/lib/python3.7/site-packages/pluggy/_manager.py", line 112, in _hookexec
2024-05-08T19:11:01.0785814Z     return self._inner_hookexec(hook_name, methods, kwargs, firstresult)
2024-05-08T19:11:01.0787192Z   File "/opt/hostedtoolcache/Python/3.7.17/x64/lib/python3.7/site-packages/pluggy/_callers.py", line 155, in _multicall
2024-05-08T19:11:01.0787980Z     return outcome.get_result()
2024-05-08T19:11:01.0788874Z   File "/opt/hostedtoolcache/Python/3.7.17/x64/lib/python3.7/site-packages/pluggy/_result.py", line 108, in get_result
2024-05-08T19:11:01.0789704Z     raise exc.with_traceback(exc.__traceback__)
2024-05-08T19:11:01.0790591Z   File "/opt/hostedtoolcache/Python/3.7.17/x64/lib/python3.7/site-packages/pluggy/_callers.py", line 80, in _multicall
2024-05-08T19:11:01.0791292Z     res = hook_impl.function(*args)
2024-05-08T19:11:01.0792155Z   File "/opt/hostedtoolcache/Python/3.7.17/x64/lib/python3.7/site-packages/_pytest/runner.py", line 177, in pytest_runtest_call
2024-05-08T19:11:01.0793229Z     raise e
2024-05-08T19:11:01.0794077Z   File "/opt/hostedtoolcache/Python/3.7.17/x64/lib/python3.7/site-packages/_pytest/runner.py", line 169, in pytest_runtest_call
2024-05-08T19:11:01.0794788Z     item.runtest()
2024-05-08T19:11:01.0795531Z   File "/opt/hostedtoolcache/Python/3.7.17/x64/lib/python3.7/site-packages/_pytest/python.py", line 1792, in runtest
2024-05-08T19:11:01.0796272Z     self.ihook.pytest_pyfunc_call(pyfuncitem=self)
2024-05-08T19:11:01.0797312Z   File "/opt/hostedtoolcache/Python/3.7.17/x64/lib/python3.7/site-packages/pluggy/_hooks.py", line 433, in __call__
2024-05-08T19:11:01.0798142Z     return self._hookexec(self.name, self._hookimpls, kwargs, firstresult)
2024-05-08T19:11:01.0799112Z   File "/opt/hostedtoolcache/Python/3.7.17/x64/lib/python3.7/site-packages/pluggy/_manager.py", line 112, in _hookexec
2024-05-08T19:11:01.0799949Z     return self._inner_hookexec(hook_name, methods, kwargs, firstresult)
2024-05-08T19:11:01.0800903Z   File "/opt/hostedtoolcache/Python/3.7.17/x64/lib/python3.7/site-packages/pluggy/_callers.py", line 116, in _multicall
2024-05-08T19:11:01.0801715Z     raise exception.with_traceback(exception.__traceback__)
2024-05-08T19:11:01.0802610Z   File "/opt/hostedtoolcache/Python/3.7.17/x64/lib/python3.7/site-packages/pluggy/_callers.py", line 80, in _multicall
2024-05-08T19:11:01.0803293Z     res = hook_impl.function(*args)
2024-05-08T19:11:01.0804192Z   File "/opt/hostedtoolcache/Python/3.7.17/x64/lib/python3.7/site-packages/pytest_tornasync/plugin.py", line 45, in pytest_pyfunc_call
2024-05-08T19:11:01.0804961Z     pyfuncitem.obj(**testargs)
2024-05-08T19:11:01.0805757Z   File "/home/runner/work/nbclassic/nbclassic/nbclassic/tests/end_to_end/test_save_as_notebook.py", line 115, in test_save_as_nb
2024-05-08T19:11:01.0806737Z     notebook_frontend.wait_for_condition(attempt_form_fill_and_save, timeout=900, period=1)
2024-05-08T19:11:01.0807692Z   File "/home/runner/work/nbclassic/nbclassic/nbclassic/tests/end_to_end/utils.py", line 694, in wait_for_condition
2024-05-08T19:11:01.0808404Z     raise EndToEndTimeout()
2024-05-08T19:11:01.0808791Z nbclassic.tests.end_to_end.utils.EndToEndTimeout
2024-05-08T19:11:01.0809285Z ==================================== PASSES ====================================

[andrewfulton9]

Thanks for the heads up @krassowski, I should have some time next week to look at this.

[RRosio]

After re-running the CI Playwright Tests, one of the three failures passed but two others are still failing. To me it seems that these failures may not actually be related to the PR itself. However it has been difficult to consistently reproduce these failures locally, where the entire test suite passes with no notable issues.

[RRosio]

I opened #277 after some local runs of the tests, and I see test failures in #277 in a different test (test_display_isolation.py) to what I modified, but test_save_as_notebook.py is passing there. I am not really confident that the CI is reliably indicating issues.