bpo-32847: Add DirectoryNotEmptyError subclass of OSError #15496
Conversation
|
@corona10 , I will fix that and other things on this PR. Thanks. |
Lib/test/test_exceptions.py
Outdated
| @@ -1170,6 +1170,12 @@ def inner(): | |||
| self.fail("RecursionError not raised") | |||
| self.assertEqual(wr(), None) | |||
|
|
|||
| def test_errno_ENOTEMPTY(self): | |||
| with self.assertRaises(OSError) as cm: | |||
There was a problem hiding this comment.
Wait, why not expecting DirectoryNotEmptyError?
There was a problem hiding this comment.
I tried but I can't assert DirectoryNotEmptyError here. Am I missing something? I will pick it later as a subclass of OSError.
There was a problem hiding this comment.
If the test fails, it sounds like there is a bug in your PR. Moreover, "with self.assertRaises(OSError) as cm:" should only surround "os.rmdir(dirname)".
There was a problem hiding this comment.
I will investigate.
Co-Authored-By: pppery <mapreader@olum.org>
|
I have done requested changes @vstinner PTAL. |
|
@pitrou: So, do you think that it's worth it to add DirectoryNotEmptyError for ENOTEMPTY? @nanjekyejoannah: Is ENOTEMPTY available on Windows, Linux, FreeBSD and macOS at least? |
I would like to echo this question. If ENOTEMPTY isn't available, or isn't actually returned in any non-exotic situation, then there's no need adding an Exception subclass for it. |
@vstinner @pitrou So AFAIK, ENOTEMPTY is available on Linux and MacOS (I confirmed with @ned-deily ). On windows am told by @zooba that they updated the exceptions mapping table in light of this issue to return this in Cpython. I think it is worth implementing this Exception subclass IMHO. Happy to hear any contrary opinions. |
There was a problem hiding this comment.
I'm not excited by adding a new builtin symbol: IMHO this module already exists too many symbols.
I'm not convinced that it's worth it. There is exactly 0 line of code in CPython code base which expects ENOTEMPTY errno. So it looks like an artifical use case.
I recall that when PEP 3151 was implemented, the implementation replaced a lot of code using "except OSError as exc: if exc.errno == XXX: ... else: raise" with "except : ..." which was neat.
But again, I don't see any usage of ENOTEMPTY in the Python stdlib.
Are you aware of 3rd party code expecting ENOTEMPTY? How many projects?
Adding a builtin symbol is a significant change, it should be well motived.
|
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 And if you don't make the requested changes, you will be put in the comfy chair! |
|
@vstinner made a comment on the bug tracker suggesting to the close this issue. Were there any additional opinions or should it be closed? |
Add
DirectoryNotEmptyErrorsubclass ofOSError.https://bugs.python.org/issue32847