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

barneygale · 2024-07-21T02:23:03Z

Add a Path.move() method that moves a file or directory tree, and returns a new Path instance pointing to the target.

This method is similar to shutil.move(), except that it doesn't accept a copy_function argument, and it doesn't check whether the destination is an existing directory.

In pathlib's private ABCs, PathBase.move() uses the copy() and delete() methods to move files and directories.

Issue: Support moving across filesystems in pathlib.Path, as shutil.move() does #73991

📚 Documentation preview 📚: https://cpython-previews--122073.org.readthedocs.build/

Add a `Path.move()` method that moves a file or directory tree and returns a new `Path` instance. This method is similar to `shutil.move()`, except that it doesn't accept a *copy_function* argument, and it doesn't support copying into an existing directory.

Doc/library/pathlib.rst

Lib/test/test_pathlib/test_pathlib_abc.py

This reverts commit 29e51f5.

This reverts commit a98aed4.

Lib/pathlib/_abc.py

Lib/pathlib/_local.py

Doc/library/pathlib.rst

Lib/test/test_pathlib/test_pathlib_abc.py

barneygale · 2024-07-31T20:02:30Z

Marking this as a draft as it will need to be revised once #122368 and #122369 land.

barneygale · 2024-08-11T23:06:37Z

Should be ready to review again :-)

This isn't quite the last PR in this series. I'll log another to add copy_into() and move_into(), and then post to discuss.python.org about perhaps eliminating some/all of the ignore_errors, on_error and ignore arguments to delete() and copy().

barneygale · 2024-08-12T00:46:27Z

Sorry for flip-flopping. I've moved the copy() changes into their own PR (#122924) so I'll mark this as a draft once again (-:

picnixz · 2024-08-12T11:09:59Z

I've reviewed the other PR. Do you want me to review this one now or should I wait?

barneygale · 2024-08-16T17:26:02Z

I've reviewed the other PR. Do you want me to review this one now or should I wait?

Sorry for the slow reply - I think it would be best to wait for the other PR to land before reviewing this.

barneygale · 2024-08-24T17:50:37Z

Hey @picnixz, this is ready to review again FYI. Thanks in advance :)

picnixz

I think it would be good to also have assertions before calling move(). That way you really know how the state of the files change before/after the test (and that way, we are sure that we are in a correct test environment). I did not mark all tests that need this but any test that does self.assertTrue(target.exists()) after moving should do self.assertFalse(target.exists()) before moving (and if the target is to be overwritten, then it should check that it's content changed).

picnixz · 2024-08-25T00:25:51Z

Doc/library/pathlib.rst

@@ -1616,6 +1616,23 @@ Copying, renaming and deleting
      Added return value, return the new :class:`!Path` instance.


+.. method:: Path.move(target)


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)?

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

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.

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

Lib/test/test_pathlib/test_pathlib_abc.py

barneygale · 2024-08-25T14:30:13Z

I don't understand why we'd make assertions before calling move() - we already know the state of the test directory (self.base) because it's configured to our specification in setUp(). If we applied this logic consistently we'd need to adjust almost every test case in DummyPathTest and PathTest!

picnixz · 2024-08-25T14:36:44Z

I don't understand why we'd make assertions before calling move() - we already know the state of the test directory (self.base) because it's configured to our specification in setUp().

I shouldn't make review at 3 AM... Yes, why didn't I think about it. So yes, it's fine (I'm marking my comments as resolved). Sorry Barney for the time loss :(

barneygale · 2024-08-25T14:39:25Z

I shouldn't make review at 3 AM... Yes, why didn't I think about it. So yes, it's fine (I'm marking my comments as resolved). Sorry Barney for the time loss :(

Oh now, don't be silly! I really appreciate all your help with this PR and others, I apologise if I came off too strongly in my previous comment. I consider these sorts of discussions a great use of my time :)

Lib/test/test_pathlib/test_pathlib_abc.py

picnixz · 2024-08-25T14:49:20Z

I apologise if I came off too strongly in my previous comment. I consider these sorts of discussions a great use of my time :)

No your comment was definitely legitimate! I also appreciate your review and feedback on my fnmatch issues / PRs!

picnixz

All good for me! I'll leave the discussion on clubber opened so that you can create a separate issue from the comment if you want.

barneygale · 2024-08-25T15:49:36Z

Amazing, thanks so much! I'll get back to your fnmatch stuff shortly!

barneygale added the topic-pathlib label Jul 21, 2024

bedevere-app bot mentioned this pull request Jul 21, 2024

Support moving across filesystems in pathlib.Path, as shutil.move() does #73991

Closed

barneygale marked this pull request as ready for review July 21, 2024 02:48

bedevere-app bot added the awaiting core review label Jul 21, 2024

barneygale added 3 commits July 21, 2024 04:17

Simplify slightly

1dc0bfb

Test existing files/directories as destinations.

f488ff8

rename --> replace

ff7746f

picnixz reviewed Jul 21, 2024

View reviewed changes

Doc/library/pathlib.rst Outdated Show resolved Hide resolved

Lib/test/test_pathlib/test_pathlib_abc.py Show resolved Hide resolved

barneygale added 5 commits July 21, 2024 22:04

Windows test fixes

29e51f5

Revert "Windows test fixes"

36955ab

This reverts commit 29e51f5.

Handle existing targets more consistently

d595047

Use generic implementation on ERROR_ACCESS_DENIED.

5de963e

Expand test coverage, lean into the Windows differences a bit.

e1c0d9e

barneygale marked this pull request as draft July 22, 2024 00:04

bedevere-app bot removed the awaiting core review label Jul 22, 2024

barneygale added 9 commits July 22, 2024 01:33

Loosen tests for OSError types.

a98aed4

Revert "Loosen tests for OSError types."

0af6396

This reverts commit a98aed4.

Tweaks

348cabd

Relax expectations once again.

aca70d1

skip tests affected by apparent os.replace() inconsistency

aa27f48

More compatibility experiments.

04b115a

Cunningly avoid specifying the problematic cases.

be48d2a

Fix moving a file/directory to itself.

a5ee60a

Undo unnecessary change

f9219ee

barneygale marked this pull request as ready for review July 22, 2024 04:41

bedevere-app bot added the awaiting core review label Jul 22, 2024

picnixz reviewed Jul 22, 2024

View reviewed changes

Ensure we exit from copytree() when copying directory over itself.

5ad2c1d

Set filename and filename2 in error messages.

92cc785

barneygale marked this pull request as ready for review August 11, 2024 22:59

bedevere-app bot added the awaiting core review label Aug 11, 2024

barneygale requested a review from picnixz August 11, 2024 23:07

Simplify patch

b8372aa

barneygale marked this pull request as draft August 12, 2024 00:46

bedevere-app bot removed the awaiting core review label Aug 12, 2024

Undo changes from pythonGH-122924.

8199fd5

Merge branch 'main' into path-move

175d687

barneygale marked this pull request as ready for review August 23, 2024 19:34

bedevere-app bot added the awaiting core review label Aug 23, 2024

Merge branch 'main' into path-move

8182a88

picnixz reviewed Aug 25, 2024

View reviewed changes

picnixz self-requested a review August 25, 2024 14:36

picnixz reviewed Aug 25, 2024

View reviewed changes

Lib/test/test_pathlib/test_pathlib_abc.py Show resolved Hide resolved

picnixz self-requested a review August 25, 2024 15:47

picnixz approved these changes Aug 25, 2024

View reviewed changes

barneygale merged commit 625d070 into python:main Aug 25, 2024
34 checks passed

bedevere-app bot removed the awaiting core review label Aug 25, 2024

barneygale mentioned this pull request Aug 26, 2024

GH-73991: Add pathlib.Path.move that can handle rename across FS #30650

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

barneygale commented Jul 21, 2024 •

edited

Loading

barneygale commented Jul 31, 2024

barneygale commented Aug 11, 2024

barneygale commented Aug 12, 2024

picnixz commented Aug 12, 2024

barneygale commented Aug 16, 2024

barneygale commented Aug 24, 2024

picnixz left a comment

picnixz Aug 25, 2024

barneygale Aug 25, 2024

picnixz Aug 25, 2024

barneygale Aug 25, 2024

barneygale commented Aug 25, 2024

picnixz commented Aug 25, 2024

barneygale commented Aug 25, 2024

picnixz commented Aug 25, 2024

picnixz left a comment

barneygale commented Aug 25, 2024

		@@ -1616,6 +1616,23 @@ Copying, renaming and deleting
		Added return value, return the new :class:`!Path` instance.


		.. method:: Path.move(target)

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

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

Conversation

barneygale commented Jul 21, 2024 • edited Loading

barneygale commented Jul 31, 2024

barneygale commented Aug 11, 2024

barneygale commented Aug 12, 2024

picnixz commented Aug 12, 2024

barneygale commented Aug 16, 2024

barneygale commented Aug 24, 2024

picnixz left a comment

Choose a reason for hiding this comment

picnixz Aug 25, 2024

Choose a reason for hiding this comment

barneygale Aug 25, 2024

Choose a reason for hiding this comment

picnixz Aug 25, 2024

Choose a reason for hiding this comment

barneygale Aug 25, 2024

Choose a reason for hiding this comment

barneygale commented Aug 25, 2024

picnixz commented Aug 25, 2024

barneygale commented Aug 25, 2024

picnixz commented Aug 25, 2024

picnixz left a comment

Choose a reason for hiding this comment

barneygale commented Aug 25, 2024

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

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

barneygale commented Jul 21, 2024 •

edited

Loading