GH-73991: Prune pathlib.Path.delete()

Remove the *ignore_errors* and *on_error* arguments from `Path.delete()`. This functionality was carried over from `shutil`, but its design needs to be re-considered in its new context. For example, we may wish to support a *missing_ok* argument (like `Path.unlink()`), or automatically `chmod()` and retry operations when we hit a permission error (like `tempfile.TemporaryDirectory`), or retry operations with a backoff (like `test.support.os_helper.rmtree()`), or utilise exception groups, etc. It's best to leave our options open for now.
python · barneygale · Aug 19, 2024 · Aug 19, 2024 · Aug 19, 2024 · Aug 19, 2024
commit 3a03d81669fd5f173983298183d4e8ff48e37f2d
@@ -1637,20 +1637,11 @@ Copying, renaming and deleting
    :meth:`Path.delete` to remove a non-empty directory.
 
 
-.. method:: Path.delete(ignore_errors=False, on_error=None)
+.. method:: Path.delete()
 
    Delete this file or directory. If this path refers to a non-empty
    directory, its files and sub-directories are deleted recursively.
 
-   If *ignore_errors* is true, errors resulting from failed deletions will be
-   ignored. If *ignore_errors* is false or omitted, and a callable is given as
-   the optional *on_error* argument, it will be called with one argument of
-   type :exc:`OSError` each time an exception is raised. The callable can
-   handle the error to continue the deletion process or re-raise it to stop.
-   Note that the filename is available as the :attr:`~OSError.filename`
-   attribute of the exception object. If neither *ignore_errors* nor
-   *on_error* are supplied, exceptions are propagated to the caller.
-
    .. note::
 
       When deleting non-empty directories on platforms that lack the necessary

@@ -923,23 +923,13 @@ def rmdir(self):
         """
         raise UnsupportedOperation(self._unsupported_msg('rmdir()'))
 
-    def delete(self, ignore_errors=False, on_error=None):
+    def delete(self):
         """
         Delete this file or directory (including all sub-directories).
-
-        If *ignore_errors* is true, exceptions raised from scanning the
-        filesystem and removing files and directories are ignored. Otherwise,
-        if *on_error* is set, it will be called to handle the error. If
-        neither *ignore_errors* nor *on_error* are set, exceptions are
-        propagated to the caller.
         """
-        if ignore_errors:
-            def on_error(err):
-                pass
-        elif on_error is None:
+        if self.is_dir(follow_symlinks=False):
             def on_error(err):
                 raise err
-        if self.is_dir(follow_symlinks=False):
             results = self.walk(
                 on_error=on_error,
                 top_down=False,  # So we rmdir() empty directories.
@@ -955,14 +945,9 @@ def on_error(err):
                         dirpath.joinpath(name).rmdir()
                     except OSError as err:
                         on_error(err)
-            delete_self = self.rmdir
+            self.rmdir()
         else:
-            delete_self = self.unlink
-        try:
-            delete_self()
-        except OSError as err:
-            err.filename = str(self)
-            on_error(err)
+            self.unlink()
     delete.avoids_symlink_attacks = False
 
     def owner(self, *, follow_symlinks=True):

@@ -824,32 +824,14 @@ def rmdir(self):
         """
         os.rmdir(self)
 
-    def delete(self, ignore_errors=False, on_error=None):
+    def delete(self):
         """
         Delete this file or directory (including all sub-directories).
-
-        If *ignore_errors* is true, exceptions raised from scanning the
-        filesystem and removing files and directories are ignored. Otherwise,
-        if *on_error* is set, it will be called to handle the error. If
-        neither *ignore_errors* nor *on_error* are set, exceptions are
-        propagated to the caller.
         """
         if self.is_dir(follow_symlinks=False):
-            onexc = None
-            if on_error:
-                def onexc(func, filename, err):
-                    err.filename = filename
-                    on_error(err)
-            shutil.rmtree(str(self), ignore_errors, onexc=onexc)
+            shutil.rmtree(str(self))
         else:
-            try:
-                self.unlink()
-            except OSError as err:
-                if not ignore_errors:
-                    if on_error:
-                        on_error(err)
-                    else:
-                        raise
+            self.unlink()
 
     delete.avoids_symlink_attacks = shutil.rmtree.avoids_symlink_attacks
 

@@ -904,10 +904,7 @@ def test_delete_unwritable(self):
             child_dir_path.chmod(new_mode)
             tmp.chmod(new_mode)
 
-            errors = []
-            tmp.delete(on_error=errors.append)
-            # Test whether onerror has actually been called.
-            self.assertEqual(len(errors), 3)
+            self.assertRaises(PermissionError, tmp.delete)
         finally:
             tmp.chmod(old_dir_mode)
             child_file_path.chmod(old_child_file_mode)
@@ -1003,16 +1000,6 @@ def close(fd):
         self.assertTrue(dir2.is_dir())
         self.assertEqual(close_count, 2)
 
-        close_count = 0
-        errors = []
-
-        with swap_attr(os, 'close', close) as orig_close:
-            dir1.delete(on_error=errors.append)
-        self.assertEqual(len(errors), 2)
-        self.assertEqual(errors[0].filename, str(dir2))
-        self.assertEqual(errors[1].filename, str(dir1))
-        self.assertEqual(close_count, 2)
-
     @unittest.skipUnless(hasattr(os, "mkfifo"), 'requires os.mkfifo()')
     @unittest.skipIf(sys.platform == "vxworks",
                      "fifo requires special path on VxWorks")
@@ -1028,63 +1015,6 @@ def test_delete_on_named_pipe(self):
         p.delete()
         self.assertFalse(p.exists())
 
-    @unittest.skipIf(sys.platform[:6] == 'cygwin',
-                     "This test can't be run on Cygwin (issue #1071513).")
-    @os_helper.skip_if_dac_override
-    @os_helper.skip_unless_working_chmod
-    def test_delete_deleted_race_condition(self):
-        # bpo-37260
-        #
-        # Test that a file or a directory deleted after it is enumerated
-        # by scandir() but before unlink() or rmdr() is called doesn't
-        # generate any errors.
-        def on_error(exc):
-            assert exc.filename
-            if not isinstance(exc, PermissionError):
-                raise
-            # Make the parent and the children writeable.
-            for p, mode in zip(paths, old_modes):
-                p.chmod(mode)
-            # Remove other dirs except one.
-            keep = next(p for p in dirs if str(p) != exc.filename)
-            for p in dirs:
-                if p != keep:
-                    p.rmdir()
-            # Remove other files except one.
-            keep = next(p for p in files if str(p) != exc.filename)
-            for p in files:
-                if p != keep:
-                    p.unlink()
-
-        tmp = self.cls(self.base, 'delete')
-        tmp.mkdir()
-        paths = [tmp] + [tmp / f'child{i}' for i in range(6)]
-        dirs = paths[1::2]
-        files = paths[2::2]
-        for path in dirs:
-            path.mkdir()
-        for path in files:
-            path.write_text('')
-
-        old_modes = [path.stat().st_mode for path in paths]
-
-        # Make the parent and the children non-writeable.
-        new_mode = stat.S_IREAD | stat.S_IEXEC
-        for path in reversed(paths):
-            path.chmod(new_mode)
-
-        try:
-            tmp.delete(on_error=on_error)
-        except:
-            # Test failed, so cleanup artifacts.
-            for path, mode in zip(paths, old_modes):
-                try:
-                    path.chmod(mode)
-                except OSError:
-                    pass
-            tmp.delete()
-            raise
-
     def test_delete_does_not_choke_on_failing_lstat(self):
         try:
             orig_lstat = os.lstat

@@ -2705,14 +2705,6 @@ def test_delete_missing(self):
         # filename is guaranteed not to exist
         filename = tmp / 'foo'
         self.assertRaises(FileNotFoundError, filename.delete)
-        # test that ignore_errors option is honored
-        filename.delete(ignore_errors=True)
-        # test on_error
-        errors = []
-        filename.delete(on_error=errors.append)
-        self.assertEqual(len(errors), 1)
-        self.assertIsInstance(errors[0], FileNotFoundError)
-        self.assertEqual(errors[0].filename, str(filename))
 
     def setUpWalk(self):
         # Build: