GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — GH-73991: Add `pathlib.Path.move()` by barneygale · Pull Request #122073 · python/cpython

Conversation

@barneygale

@barneygale barneygale commented Jul 21, 2024 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

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/

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
nineteendo reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@barneygale
pythonGH-73991: Add pathlib.Path.move()
f6d09c0 
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.
@barneygale barneygale added the topic-pathlib label Jul 21, 2024
@bedevere-app bedevere-app Bot mentioned this pull request Jul 21, 2024
Support moving across filesystems in pathlib.Path, as shutil.move() does #73991
Closed
@barneygale
barneygale marked this pull request as ready for review July 21, 2024 02:48
@bedevere-app bedevere-app Bot added the awaiting core review label Jul 21, 2024
barneygale added 3 commits July 21, 2024 04:17
@barneygale
Simplify slightly
1dc0bfb
@barneygale
Test existing files/directories as destinations.
f488ff8
@barneygale
rename --> replace
ff7746f
picnixz
picnixz reviewed Jul 21, 2024
View reviewed changes
Comment thread Doc/library/pathlib.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Lib/test/test_pathlib/test_pathlib_abc.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
barneygale added 5 commits July 21, 2024 22:04
@barneygale
Windows test fixes
29e51f5
@barneygale
Revert "Windows test fixes"
36955ab 
This reverts commit 29e51f5.
@barneygale
Handle existing targets more consistently
d595047
@barneygale
Use generic implementation on ERROR_ACCESS_DENIED.
5de963e
@barneygale
Expand test coverage, lean into the Windows differences a bit.
e1c0d9e
@barneygale
barneygale marked this pull request as draft July 22, 2024 00:04
@bedevere-app bedevere-app Bot removed the awaiting core review label Jul 22, 2024
barneygale added 9 commits July 22, 2024 01:33
@barneygale
Loosen tests for OSError types.
a98aed4
@barneygale
Revert "Loosen tests for OSError types."
0af6396 
This reverts commit a98aed4.
@barneygale
Tweaks
348cabd
@barneygale
Relax expectations once again.
aca70d1
@barneygale
skip tests affected by apparent os.replace() inconsistency
aa27f48
@barneygale
More compatibility experiments.
04b115a
@barneygale
Cunningly avoid specifying the problematic cases.
be48d2a
@barneygale
Fix moving a file/directory to itself.
a5ee60a
@barneygale
Undo unnecessary change
f9219ee
@barneygale
barneygale marked this pull request as ready for review July 22, 2024 04:41
@bedevere-app bedevere-app Bot added the awaiting core review label Jul 22, 2024
picnixz
picnixz reviewed Jul 22, 2024
View reviewed changes
Comment thread Lib/pathlib/_abc.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Lib/pathlib/_abc.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Lib/pathlib/_local.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Doc/library/pathlib.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Lib/test/test_pathlib/test_pathlib_abc.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
@barneygale
Ensure we exit from copytree() when copying directory over itself.
5ad2c1d
@barneygale

barneygale commented Jul 31, 2024

Copy link
Copy Markdown
Contributor Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@barneygale
Set filename and filename2 in error messages.
92cc785
@barneygale
barneygale marked this pull request as ready for review August 11, 2024 22:59
@bedevere-app bedevere-app Bot added the awaiting core review label Aug 11, 2024
@barneygale

barneygale commented Aug 11, 2024

Copy link
Copy Markdown
Contributor Author

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().
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@barneygale
barneygale requested a review from picnixz August 11, 2024 23:07
@barneygale
Simplify patch
b8372aa
@barneygale

barneygale commented Aug 12, 2024

Copy link
Copy Markdown
Contributor Author

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 (-:
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@barneygale
barneygale marked this pull request as draft August 12, 2024 00:46
@bedevere-app bedevere-app Bot removed the awaiting core review label Aug 12, 2024
@picnixz

picnixz commented Aug 12, 2024

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@barneygale
Undo changes from pythonGH-122924.
8199fd5
@barneygale

barneygale commented Aug 16, 2024

Copy link
Copy Markdown
Contributor Author

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@barneygale
Merge branch 'main' into path-move
175d687
@barneygale
barneygale marked this pull request as ready for review August 23, 2024 19:34
@bedevere-app bedevere-app Bot added the awaiting core review label Aug 23, 2024
@barneygale
Merge branch 'main' into path-move
8182a88
@barneygale

barneygale commented Aug 24, 2024

Copy link
Copy Markdown
Contributor Author

Hey @picnixz, this is ready to review again FYI. Thanks in advance :)
picnixz reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
picnixz
picnixz reviewed Aug 25, 2024
View reviewed changes

@picnixz picnixz left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Doc/library/pathlib.rst
Added return value, return the new :class:`!Path` instance.


.. method:: Path.move(target)

@picnixz picnixz Aug 25, 2024

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@barneygale barneygale Aug 25, 2024

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@picnixz picnixz Aug 25, 2024

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@barneygale barneygale Aug 25, 2024

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/test/test_pathlib/test_pathlib_abc.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Lib/test/test_pathlib/test_pathlib_abc.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Lib/test/test_pathlib/test_pathlib_abc.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
@barneygale

barneygale commented Aug 25, 2024

Copy link
Copy Markdown
Contributor Author

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!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@picnixz

picnixz commented Aug 25, 2024

Copy link
Copy Markdown
Member

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 :(
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@picnixz
picnixz self-requested a review August 25, 2024 14:36
@barneygale

barneygale commented Aug 25, 2024

Copy link
Copy Markdown
Contributor Author

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 :)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
picnixz
picnixz reviewed Aug 25, 2024
View reviewed changes
Comment thread Lib/test/test_pathlib/test_pathlib_abc.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
@picnixz

picnixz commented Aug 25, 2024

Copy link
Copy Markdown
Member

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!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@picnixz
picnixz self-requested a review August 25, 2024 15:47
picnixz
picnixz approved these changes Aug 25, 2024
View reviewed changes

@picnixz picnixz left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
barneygale reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@barneygale

barneygale commented Aug 25, 2024

Copy link
Copy Markdown
Contributor Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@barneygale
barneygale merged commit 625d070 into python:main Aug 25, 2024
@bedevere-app bedevere-app Bot removed the awaiting core review label Aug 25, 2024
@barneygale barneygale mentioned this pull request Aug 26, 2024
GH-73991: Add pathlib.Path.move that can handle rename across FS #30650
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@picnixz picnixz picnixz approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

topic-pathlib

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

2 participants

@barneygale @picnixz
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.