Skip to content
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

Merged
merged 9 commits into from
Aug 23, 2024

Conversation

barneygale
Copy link
Contributor

@barneygale barneygale commented Aug 12, 2024

No news blurb because pathlib.Path.copy() is still new.

Lib/pathlib/_abc.py Show resolved Hide resolved
Lib/pathlib/_abc.py Outdated Show resolved Hide resolved
Lib/pathlib/_abc.py Show resolved Hide resolved
Lib/test/test_pathlib/test_pathlib_abc.py Show resolved Hide resolved
Lib/pathlib/_abc.py Outdated Show resolved Hide resolved
barneygale added a commit to barneygale/cpython that referenced this pull request Aug 13, 2024
@barneygale barneygale requested a review from picnixz August 18, 2024 15:11
Copy link
Member

@picnixz picnixz left a 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).

@barneygale
Copy link
Contributor Author

Good shout! I've added tests for copying from one symlink to another.

@picnixz picnixz self-requested a review August 19, 2024 07:28
@barneygale
Copy link
Contributor Author

Hey @picnixz, do you think this is ready to merge?

Copy link
Member

@picnixz picnixz left a 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?)

Lib/test/test_pathlib/test_pathlib_abc.py Show resolved Hide resolved
Lib/test/test_pathlib/test_pathlib_abc.py Show resolved Hide resolved
Lib/test/test_pathlib/test_pathlib_abc.py Show resolved Hide resolved
Lib/test/test_pathlib/test_pathlib_abc.py Show resolved Hide resolved
Lib/test/test_pathlib/test_pathlib_abc.py Show resolved Hide resolved
@barneygale
Copy link
Contributor Author

No worries! Thanks tons for the review, it's been very helpful

@barneygale barneygale merged commit d7ae4dc into python:main Aug 23, 2024
32 checks passed
@picnixz
Copy link
Member

picnixz commented Aug 23, 2024

No worries! Thanks tons for the review, it's been very helpful

It was a fun moment as well!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants