GH-73991: Prune pathlib.Path.copy() and copy_into() arguments (#123337)

barneygale · web-flow · commit 7bd6ebf696ef · 2024-08-26T17:05:34.000+01:00
Remove *ignore* and *on_error* arguments from `pathlib.Path.copy[_into]()`,
because these arguments are under-designed. Specifically:

- *ignore* is appropriated from `shutil.copytree()`, but it's not clear
  how it should apply when the user copies a non-directory. We've changed
  the callback signature from the `shutil` version, but I'm not confident
  the new signature is as good as it can be.
- *on_error* is a generalisation of `shutil.copytree()`'s error handling,
  which is to accumulate exceptions and raise a single `shutil.Error` at
  the end. It's not obvious which solution is better.

Additionally, this arguments may be challenging to implement in future user
subclasses of `PathBase`, which might utilise a native recursive copying
method.
diff --git a/Doc/library/pathlib.rst b/Doc/library/pathlib.rst
@@ -1540,7 +1540,7 @@ Copying, moving and deleting
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 .. method:: Path.copy(target, *, follow_symlinks=True, dirs_exist_ok=False, \
-                      preserve_metadata=False, ignore=None, on_error=None)
+                      preserve_metadata=False)
 
    Copy this file or directory tree to the given *target*, and return a new
    :class:`!Path` instance pointing to *target*.
@@ -1563,21 +1563,11 @@ Copying, moving and deleting
    This argument has no effect when copying files on Windows (where
    metadata is always preserved).
 
-   If *ignore* is given, it should be a callable accepting one argument: a
-   source file or directory path. The callable may return true to suppress
-   copying of the path.
-
-   If *on_error* is given, it should be a callable accepting one argument: an
-   instance of :exc:`OSError`. The callable may re-raise the exception or do
-   nothing, in which case the copying operation continues. If *on_error* isn't
-   given, exceptions are propagated to the caller.
-
    .. versionadded:: 3.14
 
 
 .. method:: Path.copy_into(target_dir, *, follow_symlinks=True, \
-                           dirs_exist_ok=False, preserve_metadata=False, \
-                           ignore=None, on_error=None)
+                           dirs_exist_ok=False, preserve_metadata=False)
 
    Copy this file or directory tree into the given *target_dir*, which should
    be an existing directory. Other arguments are handled identically to
diff --git a/Lib/pathlib/_abc.py b/Lib/pathlib/_abc.py
@@ -865,48 +865,35 @@ def _copy_file(self, target):
                     raise
 
     def copy(self, target, *, follow_symlinks=True, dirs_exist_ok=False,
-             preserve_metadata=False, ignore=None, on_error=None):
+             preserve_metadata=False):
         """
         Recursively copy this file or directory tree to the given destination.
         """
         if not isinstance(target, PathBase):
             target = self.with_segments(target)
-        try:
-            self._ensure_distinct_path(target)
-        except OSError as err:
-            if on_error is None:
-                raise
-            on_error(err)
-            return
+        self._ensure_distinct_path(target)
         stack = [(self, target)]
         while stack:
             src, dst = stack.pop()
-            try:
-                if not follow_symlinks and src.is_symlink():
-                    dst._symlink_to_target_of(src)
-                    if preserve_metadata:
-                        src._copy_metadata(dst, follow_symlinks=False)
-                elif src.is_dir():
-                    children = src.iterdir()
-                    dst.mkdir(exist_ok=dirs_exist_ok)
-                    for child in children:
-                        if not (ignore and ignore(child)):
-                            stack.append((child, dst.joinpath(child.name)))
-                    if preserve_metadata:
-                        src._copy_metadata(dst)
-                else:
-                    src._copy_file(dst)
-                    if preserve_metadata:
-                        src._copy_metadata(dst)
-            except OSError as err:
-                if on_error is None:
-                    raise
-                on_error(err)
+            if not follow_symlinks and src.is_symlink():
+                dst._symlink_to_target_of(src)
+                if preserve_metadata:
+                    src._copy_metadata(dst, follow_symlinks=False)
+            elif src.is_dir():
+                children = src.iterdir()
+                dst.mkdir(exist_ok=dirs_exist_ok)
+                stack.extend((child, dst.joinpath(child.name))
+                             for child in children)
+                if preserve_metadata:
+                    src._copy_metadata(dst)
+            else:
+                src._copy_file(dst)
+                if preserve_metadata:
+                    src._copy_metadata(dst)
         return target
 
     def copy_into(self, target_dir, *, follow_symlinks=True,
-                  dirs_exist_ok=False, preserve_metadata=False, ignore=None,
-                  on_error=None):
+                  dirs_exist_ok=False, preserve_metadata=False):
         """
         Copy this file or directory tree into the given existing directory.
         """
@@ -919,8 +906,7 @@ def copy_into(self, target_dir, *, follow_symlinks=True,
             target = self.with_segments(target_dir, name)
         return self.copy(target, follow_symlinks=follow_symlinks,
                          dirs_exist_ok=dirs_exist_ok,
-                         preserve_metadata=preserve_metadata, ignore=ignore,
-                         on_error=on_error)
+                         preserve_metadata=preserve_metadata)
 
     def rename(self, target):
         """
diff --git a/Lib/test/test_pathlib/test_pathlib_abc.py b/Lib/test/test_pathlib/test_pathlib_abc.py
@@ -1984,14 +1984,6 @@ def test_copy_dir_to_itself(self):
         self.assertRaises(OSError, source.copy, source)
         self.assertRaises(OSError, source.copy, source, follow_symlinks=False)
 
-    def test_copy_dir_to_itself_on_error(self):
-        base = self.cls(self.base)
-        source = base / 'dirC'
-        errors = []
-        source.copy(source, on_error=errors.append)
-        self.assertEqual(len(errors), 1)
-        self.assertIsInstance(errors[0], OSError)
-
     def test_copy_dir_into_itself(self):
         base = self.cls(self.base)
         source = base / 'dirC'
@@ -2000,61 +1992,6 @@ def test_copy_dir_into_itself(self):
         self.assertRaises(OSError, source.copy, target, follow_symlinks=False)
         self.assertFalse(target.exists())
 
-    def test_copy_missing_on_error(self):
-        base = self.cls(self.base)
-        source = base / 'foo'
-        target = base / 'copyA'
-        errors = []
-        result = source.copy(target, on_error=errors.append)
-        self.assertEqual(result, target)
-        self.assertEqual(len(errors), 1)
-        self.assertIsInstance(errors[0], FileNotFoundError)
-
-    def test_copy_dir_ignore_false(self):
-        base = self.cls(self.base)
-        source = base / 'dirC'
-        target = base / 'copyC'
-        ignores = []
-        def ignore_false(path):
-            ignores.append(path)
-            return False
-        result = source.copy(target, ignore=ignore_false)
-        self.assertEqual(result, target)
-        self.assertEqual(set(ignores), {
-            source / 'dirD',
-            source / 'dirD' / 'fileD',
-            source / 'fileC',
-            source / 'novel.txt',
-        })
-        self.assertTrue(target.is_dir())
-        self.assertTrue(target.joinpath('dirD').is_dir())
-        self.assertTrue(target.joinpath('dirD', 'fileD').is_file())
-        self.assertEqual(target.joinpath('dirD', 'fileD').read_text(),
-                         "this is file D\n")
-        self.assertTrue(target.joinpath('fileC').is_file())
-        self.assertTrue(target.joinpath('fileC').read_text(),
-                        "this is file C\n")
-
-    def test_copy_dir_ignore_true(self):
-        base = self.cls(self.base)
-        source = base / 'dirC'
-        target = base / 'copyC'
-        ignores = []
-        def ignore_true(path):
-            ignores.append(path)
-            return True
-        result = source.copy(target, ignore=ignore_true)
-        self.assertEqual(result, target)
-        self.assertEqual(set(ignores), {
-            source / 'dirD',
-            source / 'fileC',
-            source / 'novel.txt',
-        })
-        self.assertTrue(target.is_dir())
-        self.assertFalse(target.joinpath('dirD').exists())
-        self.assertFalse(target.joinpath('fileC').exists())
-        self.assertFalse(target.joinpath('novel.txt').exists())
-
     @needs_symlinks
     def test_copy_dangling_symlink(self):
         base = self.cls(self.base)
