bpo-37523: Raise ValueError for I/O operations on a closed zipfile.ZipExtFile#14658
Conversation
…pExtFile Raises `ValueError`when calling the following on a closed `zipfile.ZipExtFile`: - `read` - `readable` - `seek` - `seekable` - `tell`
|
I think the news can be skipped. |
|
Ok, I've removed the news but I'm unsure of how to apply the "skip news" label. Thanks for the review |
|
Unfortunately we can't because we don't have the right permissions on the repo. Anyway, after removing the news file, the tests that before this operation failed now are ok. Thank you :) |
|
Ah that explains why I couldn't find the add label button! Thanks again for the review :) |
The |
There was a problem hiding this comment.
@danifus Thanks for the contribution! I like the idea of adding tests to ensure that ValueErrors are appropriately raised for zipfiles, but I had some suggestions.
To make this change a bit more scale-able (in case other exceptions are added to the tests), I would recommend changing lines 575-582 to something like this:
with zipfile.ZipFile(TESTFN2, mode="r") as zipfp:
with zipfp.open(fname) as fid:
fid.close()
for function, *args in [fid.read, fid.tell, fid.readable, fid.seekable,
fid.extract, fid.testzip, fid.write, (fid.seek, 0)]:
self.assertRaises(ValueError, function, *args)Functionally, this operates the same, but is a bit more readable and easy to add additional functions to test simply by adding another one at the end of the list. Also, (https://docs.python.org/3.9/library/io.html#io.IOBase.seek).seek() already defaults to 0, so there's no need to specify it explicitly
Edit: Corrected seek() and added other functions, mostly as scale-ability example, not for specifically adding each of those functions to the PR.
Edit2: Remembered a way to include a variable number of arguments to unpack, added it to the example.
|
Thanks for the suggestions @aeros167. |
Edit: I've changed my stance on the testing guidelines since I first wrote this review several months ago. It's not much of an issue repeat 5 lines for the tests instead of using iteration, that comes down more to styling preference and prior precedence in the existing format of the other tests in the file. I added @serhiy-storchaka as a reviewer for this PR, since he's an active core dev with expertise in the |
…pExtFile. (pythonGH-14658) Raises ValueError when calling the following on a closed zipfile.ZipExtFile: read, readable, seek, seekable, tell.
…pExtFile. (pythonGH-14658) Raises ValueError when calling the following on a closed zipfile.ZipExtFile: read, readable, seek, seekable, tell.
7 participants
Raises
ValueErrorwhen calling the following on a closedzipfile.ZipExtFile:readreadableseekseekabletellhttps://bugs.python.org/issue37523