bpo-32689: Updates shutil.move to allow for Path objects to be used as source arg (GH-15326)
Important work originally done by @emilyemorehouse two years ago and nearly ready to go in. This bug has affected many people and in some cases has been a dealbreaker to the adoption of the otherwise wonderful pathlib and PEP519. https://stackoverflow.com/questions/33625931/copy-file-with-pathlib-in-python. This adds the outstanding test request from that PR @vstinner (https://github.com/python/cpython/pull/5393). Test fails without the change, passes with it, along with every other test in test_shutil. Some variants were experimented with to make the one line change and the most performant one was picked. # Added Test for PathLike directory destination, the current fail case ``` Lib/test/test_shutil.py::TestMove::test_move_file_pathlike FAILED [100%] ============================================================== FAILURES =============================================================== __________________________________________________ TestMove.test_move_file_pathlike ___________________________________________________ self = <test.test_shutil.TestMove testMethod=test_move_file_pathlike> def test_move_file_pathlike(self): # Move a file to another location on the same filesystem. src = pathlib.Path(self.src_file) > self._check_move_file(src, self.dst_dir, self.dst_file) Lib/test/test_shutil.py:1563: _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ Lib/test/test_shutil.py:1545: in _check_move_file shutil.move(src, dst) /Library/Frameworks/Python.framework/Versions/3.7/lib/python3.7/shutil.py:562: in move real_dst = os.path.join(dst, _basename(src)) _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ path = PosixPath('/var/folders/r2/psq74t5x3nbfzlph8bh2pvdw0000gn/T/tmp9ie0wh9_/foo') def _basename(path): # A basename() variant which first strips the trailing slash, if present. # Thus we always get the last component of the path, even for directories. sep = os.path.sep + (os.path.altsep or '') > return os.path.basename(path.rstrip(sep)) E AttributeError: 'PosixPath' object has no attribute 'rstrip' /Library/Frameworks/Python.framework/Versions/3.7/lib/python3.7/shutil.py:526: AttributeError ============================================== 1 failed, 102 deselected in 0.30 seconds =============================================== ``` After change: ``` ========================================================= test session starts ========================================================= platform darwin -- Python 3.7.4, pytest-5.0.1, py-1.8.0, pluggy-0.12.0 -- /Users/maxwellmckinnon/.venvs/TA3.7/bin/python3.7 cachedir: .pytest_cache rootdir: /Users/maxwellmckinnon/dev/cpython plugins: cov-2.7.1, mock-1.10.4 collected 103 items / 102 deselected / 1 selected Lib/test/test_shutil.py::TestMove::test_move_file_pathlike PASSED [100%] ============================================== 1 passed, 102 deselected in 0.06 seconds =============================================== ``` Running all the tests in test_shutil.py ``` ╰─ pytest Lib/test/test_shutil.py -v ========================================================= test session starts ========================================================= platform darwin -- Python 3.7.4, pytest-5.0.1, py-1.8.0, pluggy-0.12.0 -- /Users/maxwellmckinnon/.venvs/TA3.7/bin/python3.7 cachedir: .pytest_cache rootdir: /Users/maxwellmckinnon/dev/cpython plugins: cov-2.7.1, mock-1.10.4 collected 103 items Lib/test/test_shutil.py::TestShutil::test_chown PASSED [ 0%] Lib/test/test_shutil.py::TestShutil::test_copy PASSED [ 1%] ... Lib/test/test_shutil.py::TermsizeTests::test_stty_match SKIPPED [ 99%] Lib/test/test_shutil.py::PublicAPITests::test_module_all_attribute PASSED [100%] ================================================ 96 passed, 7 skipped in 1.25 seconds ================================================= ``` # Performance Considerations Is it considered poor form to get rid of _basename altogether and make use of pathlib in the move function? I'm not sure if the idea is for all these modules to strictly avoid circular dependencies. They are already using os.path which is just as much a citizen in 3.8 as pathlib right? e.g. `real_dst = os.path.join(dst, _basename(src))` becomes `real_dst = Path(dst) / Path(src).name` I've looked around and familiarized myself, and I now think importing pathlib here is fine. My only remaining concern is that of performance. Here's the performance difference for this step. ``` In [46]: %timeit real_dst = os.path.join("a/b/c", _basename('b/')) 2.71 µs ± 62.6 ns per loop (mean ± std. dev. of 7 runs, 100000 loops each) In [47]: %timeit real_dst = Path("a/b/c") / Path('b/').name 12.4 µs ± 65.3 ns per loop (mean ± std. dev. of 7 runs, 100000 loops each) ``` Is 10us significant or insignificant compared to the least expensive operation this function will do? I don't know. Let's find out. ``` In [55]: %timeit os.rename('/tmp/a/a.txt', '/tmp/a/b.txt'); os.rename('/tmp/a/b.txt', '/tmp/a/a.txt') 124 µs ± 2.18 µs per loop (mean ± std. dev. of 7 runs, 10000 loops each) ``` 62us to rename. 10us seems significant enough that we wouldn't want to favor the Path sugar suggestion. 16% speed decrease from adding the 10us. What do people think? I was hoping to get to use pathlib.Path here, but I suspect for this low level move, it should be as fast as possible, and 16% is not worth one line of sugary code to me. https://bugs.python.org/issue32689 Automerge-Triggered-By: @gvanrossum
This commit is contained in:
parent
e407013089
commit
cf57cabef8
|
@ -355,6 +355,9 @@ Directory and files operations
|
||||||
copy the file more efficiently. See
|
copy the file more efficiently. See
|
||||||
:ref:`shutil-platform-dependent-efficient-copy-operations` section.
|
:ref:`shutil-platform-dependent-efficient-copy-operations` section.
|
||||||
|
|
||||||
|
.. versionchanged:: 3.9
|
||||||
|
Accepts a :term:`path-like object` for both *src* and *dst*.
|
||||||
|
|
||||||
.. function:: disk_usage(path)
|
.. function:: disk_usage(path)
|
||||||
|
|
||||||
Return disk usage statistics about the given path as a :term:`named tuple`
|
Return disk usage statistics about the given path as a :term:`named tuple`
|
||||||
|
|
|
@ -730,8 +730,20 @@ def rmtree(path, ignore_errors=False, onerror=None):
|
||||||
rmtree.avoids_symlink_attacks = _use_fd_functions
|
rmtree.avoids_symlink_attacks = _use_fd_functions
|
||||||
|
|
||||||
def _basename(path):
|
def _basename(path):
|
||||||
# A basename() variant which first strips the trailing slash, if present.
|
"""A basename() variant which first strips the trailing slash, if present.
|
||||||
# Thus we always get the last component of the path, even for directories.
|
Thus we always get the last component of the path, even for directories.
|
||||||
|
|
||||||
|
path: Union[PathLike, str]
|
||||||
|
|
||||||
|
e.g.
|
||||||
|
>>> os.path.basename('/bar/foo')
|
||||||
|
'foo'
|
||||||
|
>>> os.path.basename('/bar/foo/')
|
||||||
|
''
|
||||||
|
>>> _basename('/bar/foo/')
|
||||||
|
'foo'
|
||||||
|
"""
|
||||||
|
path = os.fspath(path)
|
||||||
sep = os.path.sep + (os.path.altsep or '')
|
sep = os.path.sep + (os.path.altsep or '')
|
||||||
return os.path.basename(path.rstrip(sep))
|
return os.path.basename(path.rstrip(sep))
|
||||||
|
|
||||||
|
@ -769,7 +781,10 @@ def move(src, dst, copy_function=copy2):
|
||||||
os.rename(src, dst)
|
os.rename(src, dst)
|
||||||
return
|
return
|
||||||
|
|
||||||
|
# Using _basename instead of os.path.basename is important, as we must
|
||||||
|
# ignore any trailing slash to avoid the basename returning ''
|
||||||
real_dst = os.path.join(dst, _basename(src))
|
real_dst = os.path.join(dst, _basename(src))
|
||||||
|
|
||||||
if os.path.exists(real_dst):
|
if os.path.exists(real_dst):
|
||||||
raise Error("Destination path '%s' already exists" % real_dst)
|
raise Error("Destination path '%s' already exists" % real_dst)
|
||||||
try:
|
try:
|
||||||
|
|
|
@ -1835,6 +1835,16 @@ class TestMove(BaseTest, unittest.TestCase):
|
||||||
# Move a file inside an existing dir on the same filesystem.
|
# Move a file inside an existing dir on the same filesystem.
|
||||||
self._check_move_file(self.src_file, self.dst_dir, self.dst_file)
|
self._check_move_file(self.src_file, self.dst_dir, self.dst_file)
|
||||||
|
|
||||||
|
def test_move_file_to_dir_pathlike_src(self):
|
||||||
|
# Move a pathlike file to another location on the same filesystem.
|
||||||
|
src = pathlib.Path(self.src_file)
|
||||||
|
self._check_move_file(src, self.dst_dir, self.dst_file)
|
||||||
|
|
||||||
|
def test_move_file_to_dir_pathlike_dst(self):
|
||||||
|
# Move a file to another pathlike location on the same filesystem.
|
||||||
|
dst = pathlib.Path(self.dst_dir)
|
||||||
|
self._check_move_file(self.src_file, dst, self.dst_file)
|
||||||
|
|
||||||
@mock_rename
|
@mock_rename
|
||||||
def test_move_file_other_fs(self):
|
def test_move_file_other_fs(self):
|
||||||
# Move a file to an existing dir on another filesystem.
|
# Move a file to an existing dir on another filesystem.
|
||||||
|
|
|
@ -0,0 +1,2 @@
|
||||||
|
Update :func:`shutil.move` function to allow for Path objects to be used as
|
||||||
|
source argument. Patch by Emily Morehouse and Maxwell "5.13b" McKinnon.
|
Loading…
Reference in New Issue