Skip to content
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

Replace IndexedGzipFile.__reduce__ with IndexedGzipFile.__reduce_ex__ #126

Merged
merged 1 commit into from
Jul 12, 2023

Conversation

musicinmybrain
Copy link
Contributor

Restores “pickle-ability” of IndexedGzipFile in Python 3.12, which was broken due to python/cpython#101948.

Maybe there is a better way to do this – there are a lot of complex possibilities in https://docs.python.org/3.12/library/pickle.html – but simply renaming the method to give it priority over io.BufferedReader.__reduce_ex__, and ignoring the new protocol parameter, is sufficient to get the tests to pass (without nibabel for now) in Fedora Linux Rawhide.

Restores “pickle-ability” of IndexedGzipFile in Python 3.12, which was
broken due to python/cpython#101948.

Fixes pauldmccarthy#125.
@pauldmccarthy
Copy link
Owner

Hi @musicinmybrain, thanks for reporting and looking into this - I don't see a need for anything more complicated than your proposal of renaming __reduce__ to __reduce_ex__. I'm not sure which version of Python __reduce_ex__ was added, but it is present in Python 2.7 which is the oldest version that I care about (not particularly strongly, I might add). So I don't think there is any need to provide a __reduce__ implementation "for backwards compatibility" as suggested by the Python docs.

Thanks!

@pauldmccarthy pauldmccarthy merged commit 90cc615 into pauldmccarthy:master Jul 12, 2023
@musicinmybrain
Copy link
Contributor Author

So I don't think there is any need to provide a __reduce__ implementation "for backwards compatibility" as suggested by the Python docs.

I believe the docs are saying that __reduce_ex__ itself is for backwards compatibility: you can use features like out of band buffers, protocol 5 and higher, while still providing a “dumber” implementation when an older protocol version is in use, either on an older Python version or because it was explicitly requested, using the protocol parameter to decide which features to use.

Since the implementation in indexed_gzip is so straightforward, that extra flexibility doesn’t seem useful; the move to __reduce_ex__ is solely because it has priority over __reduce__.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants