Skip to content

Conversation

@barneygale
Copy link
Contributor

@barneygalebarneygale commented Aug 12, 2024

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

barneygale added a commit to barneygale/cpython that referenced this pull request Aug 13, 2024
Copy link
Member

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

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

@picnixzpicnixz self-requested a review August 19, 2024 07:28
@barneygale
Copy link
ContributorAuthor

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

Copy link
Member

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

@barneygale
Copy link
ContributorAuthor

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

@barneygalebarneygale merged commit d7ae4dc into python:mainAug 23, 2024
@picnixz
Copy link
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