Uh oh!
There was an error while loading. Please reload this page.
gh-81881: Raise a shutil.SpecialFileError when copying a Unix socket - #16575
gh-81881: Raise a shutil.SpecialFileError when copying a Unix socket#16575AWhetter wants to merge 6 commits into
Conversation
This used to raise an OSError with a platform dependent message. This change always raises a SpecialFileError with a consistent message no matter the platform.
Thanks for the PR @AWhetter! |
brandtbucher
left a comment
There was a problem hiding this comment.
This looks good to me! I would maybe just change the NEWS entry to be less wordy:
:func:`shutil.copyfile` always raises a :exc:`shutil.SpecialFileError` when trying to copy a Unix socket.brandtbucher
commented
Oct 4, 2019
AWhetter
commented
Oct 4, 2019
Good idea. I'll get that changed. |
brandtbucher
commented
Oct 4, 2019
I think it helps everyone follow the flow of the PR when things aren't force pushed and history isn't rewritten. It will get squashed in the end, anyways! I just break up my commits so that the individual diffs are easy to follow. So if I'm editing and moving a function, I'll do those in separate commits in order to preserve an easy-to-follow storyline. |
chrahunt
commented
Dec 12, 2019
Similar to the issue mentioned in bpo-37701, this raises |
| else: | ||
| self.fail("shutil.Error should have been raised") | ||
| finally: | ||
| shutil.rmtree(TESTFN2, ignore_errors=True) |
There was a problem hiding this comment.
This seems unnecessary as per:
self.addCleanup(shutil.rmtree, TESTFN, ignore_errors=True)
...some lines above.
| shutil.rmtree(TESTFN2, ignore_errors=True) | ||
| @unittest.skipUnless(hasattr(socket, 'AF_UNIX'), 'requires socket.AF_UNIX') | ||
| def test_copytree_socket(self): |
There was a problem hiding this comment.
IMO testing also copytree() is not very useful since internally it uses copyfile (which you are testing).
There was a problem hiding this comment.
I was copying what was done for named pipes for consistency. But having a test for copytree() makes sense to me because if in the future copytree() stopped using copyfile() for some reason, we would still want copytree() to raise an error?
| *dst* and return *dst* in the most efficient way possible. | ||
| *src* and *dst* are path-like objects or path names given as strings. | ||
| When *src* is a named pipe or a Unix socket, a :exc:`SpecialFileError` |
There was a problem hiding this comment.
- When *src* is a named pipe or a Unix socket, a :exc:`SpecialFileError`+ When *src* is a named pipe or a Unix socket, :exc:`SpecialFileError`bedevere-bot
commented
Dec 25, 2019
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
AWhetter
commented
May 18, 2020
I have made the requested changes; please review again |
bedevere-bot
commented
May 18, 2020
Thanks for making the requested changes! @giampaolo: please review the changes made to this pull request. |
AWhetter
commented
Jul 12, 2020
Is there anything else that I can do to help progress this review? |
oz123
commented
Mar 31, 2023
@giampaolo can this PR be unblocked? |
oz123
commented
Mar 31, 2023
@arhadthedev thanks for the quick response! |
|
This used to raise an OSError with a platform dependent message.
This change always raises a SpecialFileError with a consistent message
no matter the platform.
Tackling special devices wasn't part of the original issue, but it is still technically an issue. Although devices are technically copyable (at least they are with
cp), they will likely hang the same as a named pipe would even though a named pipe is technically copyable as well.https://bugs.python.org/issue37700