FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

GH-73991: Add `pathlib.Path.move()` by barneygale · Pull Request #122073 · python/cpython · GitHub

/ cpython Public

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

Merged
barneygale merged 27 commits into
python:mainfrom
barneygale:path-move
Aug 25, 2024
ConversationCommits (27)ChecksFiles changed
Merged

GH-73991: Add pathlib.Path.move()#122073
barneygale merged 27 commits into
python:mainfrom
barneygale:path-move

Conversation

barneygale commented Jul 21, 2024 •
edited
Loading

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.

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

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.
bedevere-app Bot mentioned this pull request Jul 21, 2024
Closed
barneygale marked this pull request as ready for review July 21, 2024 02:48
picnixz reviewed Jul 21, 2024
View reviewed changes
Comment thread Doc/library/pathlib.rst Outdated
Comment thread Lib/test/test_pathlib/test_pathlib_abc.py
barneygale marked this pull request as draft July 22, 2024 00:04
barneygale marked this pull request as ready for review July 22, 2024 04:41
picnixz reviewed Jul 22, 2024
View reviewed changes
Comment thread Lib/pathlib/_abc.py Outdated
Comment thread Lib/pathlib/_abc.py Outdated
Comment thread Lib/pathlib/_local.py Outdated
Comment thread Doc/library/pathlib.rst Outdated
Comment thread Lib/test/test_pathlib/test_pathlib_abc.py

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.

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

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

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

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

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

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?

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.

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

barneygale commented Aug 24, 2024

Copy link
Copy Markdown
Contributor Author

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

picnixz reviewed Aug 25, 2024
View reviewed changes

picnixz left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Comment thread Doc/library/pathlib.rst
Added return value, return the new :class:`!Path` instance.


.. method:: Path.move(target)

picnixz Aug 25, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

barneygale Aug 25, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

picnixz Aug 25, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

barneygale Aug 25, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Comment thread Lib/test/test_pathlib/test_pathlib_abc.py
Comment thread Lib/test/test_pathlib/test_pathlib_abc.py
Comment thread Lib/test/test_pathlib/test_pathlib_abc.py

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!

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

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

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

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!

picnixz self-requested a review August 25, 2024 15:47
picnixz approved these changes Aug 25, 2024
View reviewed changes

picnixz left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 commented Aug 25, 2024

Copy link
Copy Markdown
Contributor Author

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

barneygale merged commit 625d070 into python:main Aug 25, 2024
barneygale mentioned this pull request Aug 26, 2024
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

picnixz picnixz approved these changes

Assignees

No one assigned

Labels

topic-pathlib

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

Back | FazBrowse Home | New Git URL