From 85944b414d98da41732f96f1b9412e5342a566ac Mon Sep 17 00:00:00 2001 From: barneygale Date: Sun, 8 Dec 2024 02:38:00 +0000 Subject: [PATCH] GH-127381: pathlib ABCs: remove `PathBase.unlink()` and `rmdir()` Virtual filesystems don't always make a distinction between deleting files and empty directories, and sometimes support deleting non-empty directories in a single operation. Here we remove `PathBase.unlink()` and `rmdir()`, leaving `_delete()` as the sole deletion method, now made abstract. I hope to drop the underscore prefix later on. --- Lib/pathlib/_abc.py | 43 +++-------------- Lib/pathlib/_local.py | 16 +++++-- Lib/test/test_pathlib/test_pathlib.py | 19 ++++++++ Lib/test/test_pathlib/test_pathlib_abc.py | 56 +++++------------------ 4 files changed, 48 insertions(+), 86 deletions(-) diff --git a/Lib/pathlib/_abc.py b/Lib/pathlib/_abc.py index 820970fcd5889b..309eab2ff855c3 100644 --- a/Lib/pathlib/_abc.py +++ b/Lib/pathlib/_abc.py @@ -840,6 +840,12 @@ def copy_into(self, target_dir, *, follow_symlinks=True, dirs_exist_ok=dirs_exist_ok, preserve_metadata=preserve_metadata) + def _delete(self): + """ + Delete this file or directory (including all sub-directories). + """ + raise UnsupportedOperation(self._unsupported_msg('_delete()')) + def move(self, target): """ Recursively move this file or directory tree to the given destination. @@ -874,43 +880,6 @@ def lchmod(self, mode): """ self.chmod(mode, follow_symlinks=False) - def unlink(self, missing_ok=False): - """ - Remove this file or link. - If the path is a directory, use rmdir() instead. - """ - raise UnsupportedOperation(self._unsupported_msg('unlink()')) - - def rmdir(self): - """ - Remove this directory. The directory must be empty. - """ - raise UnsupportedOperation(self._unsupported_msg('rmdir()')) - - def _delete(self): - """ - Delete this file or directory (including all sub-directories). - """ - if self.is_symlink() or self.is_junction(): - self.unlink() - elif self.is_dir(): - self._rmtree() - else: - self.unlink() - - def _rmtree(self): - def on_error(err): - raise err - results = self.walk( - on_error=on_error, - top_down=False, # So we rmdir() empty directories. - follow_symlinks=False) - for dirpath, _, filenames in results: - for filename in filenames: - filepath = dirpath / filename - filepath.unlink() - dirpath.rmdir() - def owner(self, *, follow_symlinks=True): """ Return the login name of the file owner. diff --git a/Lib/pathlib/_local.py b/Lib/pathlib/_local.py index 250bc12956f5bc..f87069ce70a2de 100644 --- a/Lib/pathlib/_local.py +++ b/Lib/pathlib/_local.py @@ -846,10 +846,18 @@ def rmdir(self): """ os.rmdir(self) - def _rmtree(self): - # Lazy import to improve module import time - import shutil - shutil.rmtree(self) + def _delete(self): + """ + Delete this file or directory (including all sub-directories). + """ + if self.is_symlink() or self.is_junction(): + self.unlink() + elif self.is_dir(): + # Lazy import to improve module import time + import shutil + shutil.rmtree(self) + else: + self.unlink() def rename(self, target): """ diff --git a/Lib/test/test_pathlib/test_pathlib.py b/Lib/test/test_pathlib/test_pathlib.py index 8c9049f15d5bf9..ce0f4748c860b1 100644 --- a/Lib/test/test_pathlib/test_pathlib.py +++ b/Lib/test/test_pathlib/test_pathlib.py @@ -1352,6 +1352,25 @@ def test_group_no_follow_symlinks(self): self.assertEqual(expected_gid, gid_2) self.assertEqual(expected_name, link.group(follow_symlinks=False)) + def test_unlink(self): + p = self.cls(self.base) / 'fileA' + p.unlink() + self.assertFileNotFound(p.stat) + self.assertFileNotFound(p.unlink) + + def test_unlink_missing_ok(self): + p = self.cls(self.base) / 'fileAAA' + self.assertFileNotFound(p.unlink) + p.unlink(missing_ok=True) + + def test_rmdir(self): + p = self.cls(self.base) / 'dirA' + for q in p.iterdir(): + q.unlink() + p.rmdir() + self.assertFileNotFound(p.stat) + self.assertFileNotFound(p.unlink) + @needs_symlinks def test_delete_symlink(self): tmp = self.cls(self.base, 'delete') diff --git a/Lib/test/test_pathlib/test_pathlib_abc.py b/Lib/test/test_pathlib/test_pathlib_abc.py index bf9ae6cc8a2433..675abf30a9f13c 100644 --- a/Lib/test/test_pathlib/test_pathlib_abc.py +++ b/Lib/test/test_pathlib/test_pathlib_abc.py @@ -1370,8 +1370,6 @@ def test_unsupported_operation(self): self.assertRaises(e, p.touch) self.assertRaises(e, p.chmod, 0o755) self.assertRaises(e, p.lchmod, 0o755) - self.assertRaises(e, p.unlink) - self.assertRaises(e, p.rmdir) self.assertRaises(e, p.owner) self.assertRaises(e, p.group) self.assertRaises(e, p.as_uri) @@ -1493,31 +1491,18 @@ def mkdir(self, mode=0o777, parents=False, exist_ok=False): self.parent.mkdir(parents=True, exist_ok=True) self.mkdir(mode, parents=False, exist_ok=exist_ok) - def unlink(self, missing_ok=False): - path = str(self) - name = self.name - parent = str(self.parent) - if path in self._directories: - raise IsADirectoryError(errno.EISDIR, "Is a directory", path) - elif path in self._files: - self._directories[parent].remove(name) - del self._files[path] - elif not missing_ok: - raise FileNotFoundError(errno.ENOENT, "File not found", path) - - def rmdir(self): + def _delete(self): path = str(self) if path in self._files: - raise NotADirectoryError(errno.ENOTDIR, "Not a directory", path) - elif path not in self._directories: - raise FileNotFoundError(errno.ENOENT, "File not found", path) - elif self._directories[path]: - raise OSError(errno.ENOTEMPTY, "Directory not empty", path) - else: - name = self.name - parent = str(self.parent) - self._directories[parent].remove(name) + del self._files[path] + elif path in self._directories: + for name in list(self._directories[path]): + self.joinpath(name)._delete() del self._directories[path] + else: + raise FileNotFoundError(errno.ENOENT, "File not found", path) + parent = str(self.parent) + self._directories[parent].remove(self.name) class DummyPathTest(DummyPurePathTest): @@ -2245,30 +2230,11 @@ def test_is_char_device_false(self): self.assertIs((P / 'fileA\udfff').is_char_device(), False) self.assertIs((P / 'fileA\x00').is_char_device(), False) - def test_unlink(self): - p = self.cls(self.base) / 'fileA' - p.unlink() - self.assertFileNotFound(p.stat) - self.assertFileNotFound(p.unlink) - - def test_unlink_missing_ok(self): - p = self.cls(self.base) / 'fileAAA' - self.assertFileNotFound(p.unlink) - p.unlink(missing_ok=True) - - def test_rmdir(self): - p = self.cls(self.base) / 'dirA' - for q in p.iterdir(): - q.unlink() - p.rmdir() - self.assertFileNotFound(p.stat) - self.assertFileNotFound(p.unlink) - def test_delete_file(self): p = self.cls(self.base) / 'fileA' p._delete() self.assertFileNotFound(p.stat) - self.assertFileNotFound(p.unlink) + self.assertFileNotFound(p._delete) def test_delete_dir(self): base = self.cls(self.base) @@ -2347,7 +2313,7 @@ def setUp(self): def tearDown(self): base = self.cls(self.base) - base._rmtree() + base._delete() def test_walk_topdown(self): walker = self.walk_path.walk()