Skip to content

GH-73991: Add pathlib.Path.move() #122073

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 27 commits into from
Aug 25, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
f6d09c0
GH-73991: Add `pathlib.Path.move()`
barneygale Jul 21, 2024
1dc0bfb
Simplify slightly
barneygale Jul 21, 2024
f488ff8
Test existing files/directories as destinations.
barneygale Jul 21, 2024
ff7746f
rename --> replace
barneygale Jul 21, 2024
29e51f5
Windows test fixes
barneygale Jul 21, 2024
36955ab
Revert "Windows test fixes"
barneygale Jul 21, 2024
d595047
Handle existing targets more consistently
barneygale Jul 21, 2024
5de963e
Use generic implementation on ERROR_ACCESS_DENIED.
barneygale Jul 21, 2024
e1c0d9e
Expand test coverage, lean into the Windows differences a bit.
barneygale Jul 22, 2024
a98aed4
Loosen tests for OSError types.
barneygale Jul 22, 2024
0af6396
Revert "Loosen tests for OSError types."
barneygale Jul 22, 2024
348cabd
Tweaks
barneygale Jul 22, 2024
aca70d1
Relax expectations once again.
barneygale Jul 22, 2024
aa27f48
skip tests affected by apparent os.replace() inconsistency
barneygale Jul 22, 2024
04b115a
More compatibility experiments.
barneygale Jul 22, 2024
be48d2a
Cunningly avoid specifying the problematic cases.
barneygale Jul 22, 2024
a5ee60a
Fix moving a file/directory to itself.
barneygale Jul 22, 2024
f9219ee
Undo unnecessary change
barneygale Jul 22, 2024
5ad2c1d
Ensure we exit from copytree() when copying directory over itself.
barneygale Jul 27, 2024
46b1fc2
Merge branch 'main' into path-move
barneygale Aug 7, 2024
d889dff
Merge branch 'main' into path-move
barneygale Aug 11, 2024
9d92108
Docs tweak
barneygale Aug 11, 2024
92cc785
Set `filename` and `filename2` in error messages.
barneygale Aug 11, 2024
b8372aa
Simplify patch
barneygale Aug 11, 2024
8199fd5
Undo changes from GH-122924.
barneygale Aug 13, 2024
175d687
Merge branch 'main' into path-move
barneygale Aug 23, 2024
8182a88
Merge branch 'main' into path-move
barneygale Aug 24, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 19 additions & 2 deletions Doc/library/pathlib.rst
Original file line number Diff line number Diff line change
Expand Up @@ -1536,8 +1536,8 @@ Creating files and directories
available. In previous versions, :exc:`NotImplementedError` was raised.


Copying, renaming and deleting
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Copying, moving and deleting
^^^^^^^^^^^^^^^^^^^^^^^^^^^^

.. method:: Path.copy(target, *, follow_symlinks=True, dirs_exist_ok=False, \
preserve_metadata=False, ignore=None, on_error=None)
Expand Down Expand Up @@ -1616,6 +1616,23 @@ Copying, renaming and deleting
Added return value, return the new :class:`!Path` instance.


.. method:: Path.move(target)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just me thinking loud, but do you think we should have a boolean to not overwrite an existing target (in which case, a FileExistsError would be raised)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could add a clobber=True argument, but I think that would be a separate PR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would have thought of strict (strict=False by default, strict=True would raise) or replace=True. But we can address this question later. Otherwise, it's just a three liner where users would do if not os.exists(target) if they want to avoid replacing files.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Clobber" has some currency already, e.g. in mv --no-clobber. But yeah, let's discuss later.


Move this file or directory tree to the given *target*, and return a new
:class:`!Path` instance pointing to *target*.

If the *target* doesn't exist it will be created. If both this path and the
*target* are existing files, then the target is overwritten. If both paths
point to the same file or directory, or the *target* is a non-empty
directory, then :exc:`OSError` is raised.

If both paths are on the same filesystem, the move is performed with
:func:`os.replace`. Otherwise, this path is copied (preserving metadata and
symlinks) and then deleted.

.. versionadded:: 3.14


.. method:: Path.unlink(missing_ok=False)

Remove this file or symbolic link. If the path points to a directory,
Expand Down
5 changes: 4 additions & 1 deletion Doc/whatsnew/3.14.rst
Original file line number Diff line number Diff line change
Expand Up @@ -164,10 +164,13 @@ os
pathlib
-------

* Add methods to :class:`pathlib.Path` to recursively copy or remove files:
* Add methods to :class:`pathlib.Path` to recursively copy, move, or remove
files and directories:

* :meth:`~pathlib.Path.copy` copies a file or directory tree to a given
destination.
* :meth:`~pathlib.Path.move` moves a file or directory tree to a given
destination.
* :meth:`~pathlib.Path.delete` removes a file or directory tree.

(Contributed by Barney Gale in :gh:`73991`.)
Expand Down
21 changes: 20 additions & 1 deletion Lib/pathlib/_abc.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
import functools
import operator
import posixpath
from errno import EINVAL
from errno import EINVAL, EXDEV
from glob import _GlobberBase, _no_recurse_symlinks
from stat import S_ISDIR, S_ISLNK, S_ISREG, S_ISSOCK, S_ISBLK, S_ISCHR, S_ISFIFO
from pathlib._os import copyfileobj
Expand Down Expand Up @@ -928,6 +928,25 @@ def replace(self, target):
"""
raise UnsupportedOperation(self._unsupported_msg('replace()'))

def move(self, target):
"""
Recursively move this file or directory tree to the given destination.
"""
self._ensure_different_file(target)
try:
return self.replace(target)
except UnsupportedOperation:
pass
except TypeError:
if not isinstance(target, PathBase):
raise
except OSError as err:
if err.errno != EXDEV:
raise
target = self.copy(target, follow_symlinks=False, preserve_metadata=True)
self.delete()
return target

def chmod(self, mode, *, follow_symlinks=True):
"""
Change the permissions of the path, like os.chmod().
Expand Down
62 changes: 62 additions & 0 deletions Lib/test/test_pathlib/test_pathlib.py
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,19 @@
{os.open, os.stat, os.unlink, os.rmdir} <= os.supports_dir_fd and
os.listdir in os.supports_fd and os.stat in os.supports_follow_symlinks)

def patch_replace(old_test):
def new_replace(self, target):
raise OSError(errno.EXDEV, "Cross-device link", self, target)

def new_test(self):
old_replace = self.cls.replace
self.cls.replace = new_replace
try:
old_test(self)
finally:
self.cls.replace = old_replace
return new_test

#
# Tests for the pure classes.
#
Expand Down Expand Up @@ -799,6 +812,55 @@ def test_copy_dir_preserve_metadata_xattrs(self):
target_file = target.joinpath('dirD', 'fileD')
self.assertEqual(os.getxattr(target_file, b'user.foo'), b'42')

@patch_replace
def test_move_file_other_fs(self):
self.test_move_file()

@patch_replace
def test_move_file_to_file_other_fs(self):
self.test_move_file_to_file()

@patch_replace
def test_move_file_to_dir_other_fs(self):
self.test_move_file_to_dir()

@patch_replace
def test_move_dir_other_fs(self):
self.test_move_dir()

@patch_replace
def test_move_dir_to_dir_other_fs(self):
self.test_move_dir_to_dir()

@patch_replace
def test_move_dir_into_itself_other_fs(self):
self.test_move_dir_into_itself()

@patch_replace
@needs_symlinks
def test_move_file_symlink_other_fs(self):
self.test_move_file_symlink()

@patch_replace
@needs_symlinks
def test_move_file_symlink_to_itself_other_fs(self):
self.test_move_file_symlink_to_itself()

@patch_replace
@needs_symlinks
def test_move_dir_symlink_other_fs(self):
self.test_move_dir_symlink()

@patch_replace
@needs_symlinks
def test_move_dir_symlink_to_itself_other_fs(self):
self.test_move_dir_symlink_to_itself()

@patch_replace
@needs_symlinks
def test_move_dangling_symlink_other_fs(self):
self.test_move_dangling_symlink()

def test_resolve_nonexist_relative_issue38671(self):
p = self.cls('non', 'exist')

Expand Down
119 changes: 119 additions & 0 deletions Lib/test/test_pathlib/test_pathlib_abc.py
Original file line number Diff line number Diff line change
Expand Up @@ -2072,6 +2072,125 @@ def test_copy_dangling_symlink(self):
self.assertTrue(target2.joinpath('link').is_symlink())
self.assertEqual(target2.joinpath('link').readlink(), self.cls('nonexistent'))

def test_move_file(self):
base = self.cls(self.base)
source = base / 'fileA'
source_text = source.read_text()
target = base / 'fileA_moved'
result = source.move(target)
self.assertEqual(result, target)
self.assertFalse(source.exists())
self.assertTrue(target.exists())
self.assertEqual(source_text, target.read_text())

def test_move_file_to_file(self):
base = self.cls(self.base)
source = base / 'fileA'
source_text = source.read_text()
target = base / 'dirB' / 'fileB'
result = source.move(target)
self.assertEqual(result, target)
self.assertFalse(source.exists())
self.assertTrue(target.exists())
self.assertEqual(source_text, target.read_text())

def test_move_file_to_dir(self):
base = self.cls(self.base)
source = base / 'fileA'
target = base / 'dirB'
self.assertRaises(OSError, source.move, target)

def test_move_file_to_itself(self):
base = self.cls(self.base)
source = base / 'fileA'
self.assertRaises(OSError, source.move, source)

def test_move_dir(self):
base = self.cls(self.base)
source = base / 'dirC'
target = base / 'dirC_moved'
result = source.move(target)
self.assertEqual(result, target)
self.assertFalse(source.exists())
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_move_dir_to_dir(self):
base = self.cls(self.base)
source = base / 'dirC'
target = base / 'dirB'
self.assertRaises(OSError, source.move, target)
self.assertTrue(source.exists())
self.assertTrue(target.exists())

def test_move_dir_to_itself(self):
base = self.cls(self.base)
source = base / 'dirC'
self.assertRaises(OSError, source.move, source)
self.assertTrue(source.exists())

def test_move_dir_into_itself(self):
base = self.cls(self.base)
source = base / 'dirC'
target = base / 'dirC' / 'bar'
self.assertRaises(OSError, source.move, target)
self.assertTrue(source.exists())
self.assertFalse(target.exists())

@needs_symlinks
def test_move_file_symlink(self):
base = self.cls(self.base)
source = base / 'linkA'
source_readlink = source.readlink()
target = base / 'linkA_moved'
result = source.move(target)
self.assertEqual(result, target)
self.assertFalse(source.exists())
self.assertTrue(target.is_symlink())
self.assertEqual(source_readlink, target.readlink())

@needs_symlinks
def test_move_file_symlink_to_itself(self):
base = self.cls(self.base)
source = base / 'linkA'
self.assertRaises(OSError, source.move, source)

@needs_symlinks
def test_move_dir_symlink(self):
base = self.cls(self.base)
source = base / 'linkB'
source_readlink = source.readlink()
target = base / 'linkB_moved'
result = source.move(target)
self.assertEqual(result, target)
self.assertFalse(source.exists())
self.assertTrue(target.is_symlink())
self.assertEqual(source_readlink, target.readlink())

@needs_symlinks
def test_move_dir_symlink_to_itself(self):
base = self.cls(self.base)
source = base / 'linkB'
self.assertRaises(OSError, source.move, source)

@needs_symlinks
def test_move_dangling_symlink(self):
base = self.cls(self.base)
source = base / 'brokenLink'
source_readlink = source.readlink()
target = base / 'brokenLink_moved'
result = source.move(target)
self.assertEqual(result, target)
self.assertFalse(source.exists())
self.assertTrue(target.is_symlink())
self.assertEqual(source_readlink, target.readlink())

def test_iterdir(self):
P = self.cls
p = P(self.base)
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Add :meth:`pathlib.Path.move`, which moves a file or directory tree.
Loading