GH-127381: pathlib ABCs: remove `PathBase.unlink()` and `rmdir()` (#127736)

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.

diff --git a/Lib/pathlib/_abc.py b/Lib/pathlib/_abc.py
index 820970fcd5889b83d811dd5fb949a9db65f5c821..309eab2ff855c3b391d5cf52070728f260b94141 100644 (file)
--- a/Lib/pathlib/_abc.py
+++ b/Lib/pathlib/_abc.py
@@ -840,6 +840,12 @@ class PathBase(PurePathBase):
                          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 @@ class PathBase(PurePathBase):
         """
         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 250bc12956f5bca9d6d1e9f4d226116c24b13be5..f87069ce70a2de7eda39b7c529e9a6b017300cb6 100644 (file)
--- a/Lib/pathlib/_local.py
+++ b/Lib/pathlib/_local.py
@@ -846,10 +846,18 @@ class Path(PathBase, PurePath):
         """
         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 8c9049f15d5bf9a75d068d4fbadf5580983139e5..ce0f4748c860b112597e42acf9bdc891068ce0fa 100644 (file)
--- a/Lib/test/test_pathlib/test_pathlib.py
+++ b/Lib/test/test_pathlib/test_pathlib.py
@@ -1352,6 +1352,25 @@ class PathTest(test_pathlib_abc.DummyPathTest, PurePathTest):
         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 bf9ae6cc8a2433e746d258bdfdce6787c4f4eda6..675abf30a9f13cc1adc07196f693de2cd5fc364c 100644 (file)
--- a/Lib/test/test_pathlib/test_pathlib_abc.py
+++ b/Lib/test/test_pathlib/test_pathlib_abc.py
@@ -1370,8 +1370,6 @@ class PathBaseTest(PurePathBaseTest):
         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 @@ class DummyPath(PathBase):
             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 @@ class DummyPathTest(DummyPurePathTest):
         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 @@ class DummyPathWalkTest(unittest.TestCase):
 
     def tearDown(self):
         base = self.cls(self.base)
-        base._rmtree()
+        base._delete()
 
     def test_walk_topdown(self):
         walker = self.walk_path.walk()