New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
zipfile: change RuntimeError to more appropriate exception type #68881
Comments
RuntimeError is raised in the zipfile module in many cases where more appropriate exception type is expected. Proposed patch changes a number of RuntimeErrors to one of BadZipFile, NotImplementedError, or ValueError. Only changing to NotImplementedError is backward compatible (NotImplementedError is subclass of RuntimeError), other changes are not. |
I have hardly used the zipfile module, but here are my thoughts on some of the exceptions anyway :) Some of these exceptions are documented, so the documentation would need updating. BadZipFile for corrupted field seems reasonable. The purpose of RuntimeError is not clearly documented, but I have the feeling it gets used mainly for programmer errors, rather than errors caused by external data. So I tend to agree a couple of the changes away from RuntimeError, such as the invalid password case. For programmer errors, I doubt the exception type matters much for compatibility. RuntimeError seems fine to me, but I accept that ValueError is more consistent with open(..., mode="invalid"), operations on closed files, etc. |
Updated patch includes changed tests and the documentation. It includes changes for new exceptions added in bpo-26039. Perhaps not all changes should be accepted, but I have included them for demonstrating. RuntimeError was used in following cases:
I'm going to push changes for cases 3 and 4, this looks safe. Cases 5 and 6 are more discussable. There are reasons for RuntimeError in cases 7, 8 and 9. And I think RuntimeError is appropriate in cases 1 and 2. |
New changeset 22112359abcf by Serhiy Storchaka in branch 'default': |
Changed cases 3, 4, 5, 7, and 8. |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: