GH-73991: Disallow copying directory into itself via `pathlib.Path.copy()` by barneygale · Pull Request #122924 · python/cpython
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?
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
No worries! Thanks tons for the review, it's been very helpful
It was a fun moment as well!