◐ Shell
reader mode source ↗
Skip to content

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

Merged
barneygale merged 27 commits into
python:mainfrom
barneygale:path-move
Aug 25, 2024
Merged

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

Conversation

@barneygale

@barneygale 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/

@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 label Jul 21, 2024
@bedevere-app bedevere-app Bot mentioned this pull request Jul 21, 2024
Closed
@barneygale barneygale marked this pull request as ready for review July 21, 2024 02:48
@barneygale barneygale marked this pull request as draft July 22, 2024 00:04
@barneygale barneygale marked this pull request as ready for review July 22, 2024 04:41
@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.

18 hidden items Load more…
@barneygale barneygale marked this pull request as ready for review August 11, 2024 22:59
@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().

@barneygale barneygale requested a review from picnixz August 11, 2024 23:07
@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 (-:

@barneygale barneygale marked this pull request as draft August 12, 2024 00:46
@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?

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

@barneygale barneygale marked this pull request as ready for review August 23, 2024 19:34
@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
picnixz reviewed Aug 25, 2024
View reviewed changes

@picnixz 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.

Hide comment

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

@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!

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

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

@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!

@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

Choose a reason for hiding this comment

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

Hide comment

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

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 barneygale merged commit 625d070 into python:main Aug 25, 2024
@barneygale barneygale mentioned this pull request Aug 26, 2024
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

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

@barneygale @picnixz