Allowing non empty dirs to be deleted #3108
Conversation
|
@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. |
|
|
||
| if self.delete_to_trash: | ||
| if os.path.isdir(os_path): | ||
| listing = os.listdir(os_path) | ||
| # Don't send non-empty directories to trash. |
There was a problem hiding this comment.
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.
| @@ -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']): | |||
There was a problem hiding this comment.
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.
|
@takluyver the |
|
Looks like it's passed now. Sometimes there's a random connection failure while installing a package. |
| @@ -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): | |||
There was a problem hiding this comment.
Can we add a test that deleting a non-empty directory does work?
| 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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Oh okay. Sure, I'll make these changes!
|
@takluyver I've added the required tests. Is the PR okay to be merged? |
|
Yup, thanks! |
|
Non-empty folders can't be deleted in version 6 of the notebook, was it changed intentionally back to old behavior? #4916 |
Fixes #2760