Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 34k
GH-73991: Add pathlib.Path.move()#122073
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
barneygale commented Jul 21, 2024 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
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.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
This reverts commit a98aed4.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
barneygale commented Jul 31, 2024
barneygale commented Aug 11, 2024
Should be ready to review again :-) This isn't quite the last PR in this series. I'll log another to add |
barneygale commented Aug 12, 2024
Sorry for flip-flopping. I've moved the |
picnixz commented Aug 12, 2024
I've reviewed the other PR. Do you want me to review this one now or should I wait? |
barneygale commented Aug 16, 2024
Sorry for the slow reply - I think it would be best to wait for the other PR to land before reviewing this. |
barneygale commented Aug 24, 2024
Hey @picnixz, this is ready to review again FYI. Thanks in advance :) |
picnixz left a comment
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).
| Added return value, return the new :class:`!Path` instance. | ||
| .. method:: Path.move(target) |
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)?
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.
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.
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.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
barneygale commented Aug 25, 2024
I don't understand why we'd make assertions before calling |
picnixz commented Aug 25, 2024
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 commented Aug 25, 2024
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 :) |
Uh oh!
There was an error while loading. Please reload this page.
picnixz commented Aug 25, 2024
No your comment was definitely legitimate! I also appreciate your review and feedback on my |
picnixz left a comment
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.
barneygale commented Aug 25, 2024
Amazing, thanks so much! I'll get back to your |
Add a
Path.move()method that moves a file or directory tree, and returns a newPathinstance 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 thecopy()anddelete()methods to move files and directories.📚 Documentation preview 📚: https://cpython-previews--122073.org.readthedocs.build/