-
-
Notifications
You must be signed in to change notification settings - Fork 31.1k
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
GH-73991: Disallow copying directory into itself via pathlib.Path.copy()
#122924
GH-73991: Disallow copying directory into itself via pathlib.Path.copy()
#122924
Conversation
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 my interrogations are answered. Do we actually need to have a full coverage? I think that what we are covering currently is A' -> A' and A -> A' with A' a symlink to A but I'm not sure if we need to do more per our discussion on the perverse example (for instance, A' -> A'' where both are symlinks actually).
Good shout! I've added tests for copying from one symlink to another. |
Hey @picnixz, do you think this is ready to merge? |
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.
Sometimes I request reviews for myself in order not to forget them... and then I still forget about them >.< From what we decided, I think we are fine with the tests, but you could also increase the coverage by testing the follow_symlinks=False
case. I'm fine with the current state of the PR though (by the way, any reason why you sometimes use a context manager for checking the error and sometimes not?)
No worries! Thanks tons for the review, it's been very helpful |
It was a fun moment as well! |
No news blurb because
pathlib.Path.copy()
is still new.