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

Allowing non empty dirs to be deleted #3108

Merged
merged 13 commits into from Dec 5, 2017
Merged

Conversation

@kirit93
Copy link
Contributor

@kirit93 kirit93 commented Dec 4, 2017

Fixes #2760

@kirit93
Copy link
Contributor Author

@kirit93 kirit93 commented Dec 5, 2017

@takluyver It seems that the delete function is unable to delete the unicode directory from the test case. However the only changes I made to the delete function are those that allow directories to be deleted so I am unsure why unicode isn't being deleted.

Loading


if self.delete_to_trash:
if os.path.isdir(os_path):
listing = os.listdir(os_path)
# Don't send non-empty directories to trash.
Copy link
Member

@takluyver takluyver Dec 5, 2017

Choose a reason for hiding this comment

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

I think you've got this backwards. The idea is that if we're using trash (if self.delete_to_trash is true), then we can delete non-empty directories, because the user can get them back from trash. If we're not using trash, then deleting is permanent, so we should still refuse to delete non-empty directories.

Loading

@@ -518,7 +518,8 @@ def test_delete_dirs(self):
for name in sorted(self.dirs + ['/'], key=len, reverse=True):
listing = self.api.list(name).json()['content']
for model in listing:
self.api.delete(model['path'])
if os.path.exists(model['path']):
Copy link
Member

@takluyver takluyver Dec 5, 2017

Choose a reason for hiding this comment

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

API paths shouldn't be used as filesystem paths - you can't pass them to functions like os.path.exists(), because they probably don't start from the same directory where the tests are running. This check shouldn't be needed in any case.

Loading

@kirit93 kirit93 closed this Dec 5, 2017
@kirit93 kirit93 reopened this Dec 5, 2017
@kirit93
Copy link
Contributor Author

@kirit93 kirit93 commented Dec 5, 2017

@takluyver the GROUP=js/services tests fails with The command "pip install --upgrade setuptools wheel nose coverage codecov" failed and exited with 2 during .
Any ideas what this could be due to?

Loading

@takluyver
Copy link
Member

@takluyver takluyver commented Dec 5, 2017

Looks like it's passed now. Sometimes there's a random connection failure while installing a package.

Loading

Copy link
Member

@takluyver takluyver left a comment

Thanks, this is looking good.

Loading

@@ -522,11 +522,6 @@ def test_delete_dirs(self):
listing = self.api.list('/').json()['content']
self.assertEqual(listing, [])

def test_delete_non_empty_dir(self):
Copy link
Member

@takluyver takluyver Dec 5, 2017

Choose a reason for hiding this comment

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

Can we add a test that deleting a non-empty directory does work?

Loading

def test_delete_non_empty_dir(self):
# Test that non empty directory can be deleted
self.api.delete(u'å b')
# Assertion will pass only if self.api.delete does not throw and error
Copy link
Member

@takluyver takluyver Dec 5, 2017

Choose a reason for hiding this comment

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

A test doesn't have to have an assert - you can leave this off, and it will still fail if there's an error. When a test just checks that something runs without an error, that's a 'smoketest' (as in 'does this make smoke come out').

However, it would be good to check that it has actually been removed as well. Maybe copy some code from test_list_nonexistant_dir.

Loading

Copy link
Contributor Author

@kirit93 kirit93 Dec 5, 2017

Choose a reason for hiding this comment

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

Oh okay. Sure, I'll make these changes!

Loading

@kirit93 kirit93 closed this Dec 5, 2017
@kirit93 kirit93 reopened this Dec 5, 2017
@kirit93
Copy link
Contributor Author

@kirit93 kirit93 commented Dec 5, 2017

@takluyver I've added the required tests. Is the PR okay to be merged?

Loading

@takluyver takluyver added this to the 5.3 milestone Dec 5, 2017
@takluyver
Copy link
Member

@takluyver takluyver commented Dec 5, 2017

Yup, thanks!

Loading

@takluyver takluyver merged commit ae011a1 into jupyter:master Dec 5, 2017
4 checks passed
Loading
@Dmitry1987
Copy link

@Dmitry1987 Dmitry1987 commented May 18, 2020

Non-empty folders can't be deleted in version 6 of the notebook, was it changed intentionally back to old behavior? #4916

Loading

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants