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

Create directory structure on rename #1058

Closed
wants to merge 1 commit into from

Conversation

lucas-bremond
Copy link

@lucas-bremond lucas-bremond commented Apr 7, 2023

Using a global configuration like this one:

formats = "notebooks///ipynb,scripts///py:percent"

and moving a notebook stored under ./notebook/my_notebook.ipynb to a newly created ./notebook/subdir directory (so that the destination would be ./notebook/subdir/my_notebook.ipynb) fails when subdir is not already present under ./scripts/.

The proposed change makes the process transparent to the user, by lazily creating the necessary "folder tree" in the mirrored destination.

Issue: #1059

Copy link
Owner

@mwouts mwouts left a comment

Choose a reason for hiding this comment

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

This is a nice fix, however I would like to see it tested. Can you add a test based on the existing tests for rename?

@@ -481,6 +481,7 @@ def rename_file(self, old_path, new_path):
for old_alt_path, alt_fmt in old_alt_paths:
new_alt_path = full_path(new_base, alt_fmt)
if self.exists(old_alt_path):
os.makedirs(os.path.dirname(new_alt_path), exist_ok=True)
Copy link
Owner

Choose a reason for hiding this comment

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

I think we should create the directory using the contents manager itself, not os.makedirs, because in some cases the underlying contents manager is not based on a file system.

Copy link
Author

@lucas-bremond lucas-bremond Apr 25, 2023

Choose a reason for hiding this comment

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

Hello @mwouts, that's a fair point indeed!

I am not familiar with Jupyter's internals, but I can take a look.

When talking about the "underlying contents manager", are you referring to the FileContentsManager from jupyter_server?

If so, I can see there is a _save_directory method, but it doesn't look recursive to me.

Any pointer?

@mwouts
Copy link
Owner

mwouts commented Jun 20, 2023

This should be solved by #1082 . Thanks anyway for submitting this PR!

@mwouts mwouts closed this Jun 20, 2023
@lucas-bremond lucas-bremond deleted the patch-1 branch June 20, 2023 23:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants