bpo-37523: Raise ValueError for I/O operations on a closed zipfile.ZipExtFile

[danifus]

Raises ValueErrorwhen calling the following on a closed zipfile.ZipExtFile:

• read
• readable
• seek
• seekable
• tell

https://bugs.python.org/issue37523

[mangrisano]

I think the news can be skipped.

[danifus]

Ok, I've removed the news but I'm unsure of how to apply the "skip news" label. Thanks for the review

[mangrisano]

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 :)

[mangrisano]

LGTM.

[danifus]

Ah that explains why I couldn't find the add label button!

Thanks again for the review :)

[aeros]

Ok, I've removed the news but I'm unsure of how to apply the "skip news" label. Thanks for the review

The skip news label can currently only be applied by the core devs. There's a proposal in place to allow for the triagers to also be able to do this, but contributors can't add those labels. If you don't think your particular PR needs the news item, just leave it out and a core dev will let you know whether or not it's needed when they review it.

[aeros]

@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, seek() already defaults to 0, so there's no need to specify it explicitly (https://docs.python.org/3.9/library/io.html#io.IOBase.seek).

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.

[danifus]

Thanks for the suggestions @aeros167. seek does require the offset parameter to be supplied (seek(offset[, whence])) and I prefer having them written out explicitly in this instance.

[aeros]

Thanks for the suggestions @aeros167. seek does require the offset parameter to be supplied (seek(offset[, whence])) and I prefer having them written out explicitly in this instance.

@danifus

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 zipfile module.