Skip to content

GH-73991: Disallow copying directory into itself via pathlib.Path.copy() - #122924

Merged
barneygale merged 9 commits into
python:mainfrom
barneygale:gh-73991-forbid-copy-into-self
Aug 23, 2024
Merged

GH-73991: Disallow copying directory into itself via pathlib.Path.copy()#122924
barneygale merged 9 commits into
python:mainfrom
barneygale:gh-73991-forbid-copy-into-self

Conversation

@barneygale

@barneygalebarneygale commented Aug 12, 2024

Copy link
Copy Markdown
Contributor

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

Comment threadLib/pathlib/_abc.py
Comment threadLib/pathlib/_abc.py Outdated
Comment threadLib/pathlib/_abc.py
Comment threadLib/test/test_pathlib/test_pathlib_abc.py
Comment threadLib/pathlib/_abc.py Outdated
barneygale added a commit to barneygale/cpython that referenced this pull request Aug 13, 2024

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

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
Copy Markdown
ContributorAuthor

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
Copy Markdown
ContributorAuthor

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

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

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

Comment threadLib/test/test_pathlib/test_pathlib_abc.py
Comment threadLib/test/test_pathlib/test_pathlib_abc.py
Comment threadLib/test/test_pathlib/test_pathlib_abc.py
Comment threadLib/test/test_pathlib/test_pathlib_abc.py
Comment threadLib/test/test_pathlib/test_pathlib_abc.py
@barneygale

Copy link
Copy Markdown
ContributorAuthor

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

@barneygale
barneygale merged commit d7ae4dc into python:mainAug 23, 2024
@picnixz

Copy link
Copy Markdown
Member

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

It was a fun moment as well!

Sign up for freeto 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

@barneygale@picnixz