From 7bd6ebf696efcd5cf8e4e7946f9d8d8aee05664c Mon Sep 17 00:00:00 2001 From: Barney Gale Date: Mon, 26 Aug 2024 17:05:34 +0100 Subject: GH-73991: Prune `pathlib.Path.copy()` and `copy_into()` arguments (#123337) 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. --- Doc/library/pathlib.rst | 14 +------ Lib/pathlib/_abc.py | 52 ++++++++++--------------- Lib/test/test_pathlib/test_pathlib_abc.py | 63 ------------------------------- 3 files changed, 21 insertions(+), 108 deletions(-) diff --git a/Doc/library/pathlib.rst b/Doc/library/pathlib.rst index 4a8de26..1c54cea 100644 --- 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 index 2c24643..11c8018 100644 --- a/Lib/pathlib/_abc.py +++ b/Lib/pathlib/_abc.py @@ -865,48 +865,35 @@ class PathBase(PurePathBase): 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 @@ class PathBase(PurePathBase): 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 index 7cddc38..08355a7 100644 --- a/Lib/test/test_pathlib/test_pathlib_abc.py +++ b/Lib/test/test_pathlib/test_pathlib_abc.py @@ -1984,14 +1984,6 @@ class DummyPathTest(DummyPurePathTest): 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 @@ class DummyPathTest(DummyPurePathTest): 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) -- cgit v0.12