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

bpo-36785: PEP 574 implementation #7076

Merged
merged 5 commits into from May 26, 2019
Merged

bpo-36785: PEP 574 implementation #7076

merged 5 commits into from May 26, 2019

Conversation

@pitrou
Copy link
Member

@pitrou pitrou commented May 23, 2018

@pitrou pitrou requested a review from python/windows-team as a code owner May 23, 2018
@pitrou pitrou force-pushed the pitrou:pickle5 branch 3 times, most recently from cef20d0 to 8943edf Apr 30, 2019
@pitrou pitrou removed the DO-NOT-MERGE label May 3, 2019
@pitrou pitrou changed the title [WIP] [DO NOT MERGE] PEP 574 implementation PEP 574 implementation May 3, 2019
@pitrou pitrou changed the title PEP 574 implementation bpo-36785: PEP 574 implementation May 3, 2019
@pitrou
Copy link
Member Author

@pitrou pitrou commented May 3, 2019

@pitrou pitrou force-pushed the pitrou:pickle5 branch from 8943edf to 25cbd5e May 3, 2019
Modules/_pickle.c Outdated Show resolved Hide resolved

if (self->readinto == NULL) {
/* Call read() and memcpy */
/* XXX do we want this fallback? pickle.py doesn't have it */

This comment has been minimized.

@pitrou

pitrou May 3, 2019
Author Member

@ogrisel Do you think we need to support "file-like" objects without a readinto() method?

This comment has been minimized.

@ogrisel

ogrisel May 4, 2019
Contributor

I don't have a strong opinion. Are there any such file objects in the standard library? The file-like objects that acts as wrappers for (de)compression algorithms such as GzipFile seem to support readinto.

This comment has been minimized.

@pitrou

pitrou May 4, 2019
Author Member

There doesn't seem to be any. I'll remove this snippet.

This comment has been minimized.

@pitrou

pitrou May 4, 2019
Author Member

Actually, the doc mentions that API contract for file is only to have read and readline, so I'm not sure it's ok to remove the fallback...

This comment has been minimized.

@ammaraskar

ammaraskar Feb 19, 2020
Member

Was the fallback ever added back? Looks like this came up as a real issue on https://bugs.python.org/issue39681

This comment has been minimized.

@pitrou

pitrou Feb 19, 2020
Author Member

Thanks for the ping. I'll reply on the issue.

Lib/pickle.py Outdated Show resolved Hide resolved
@pitrou
Copy link
Member Author

@pitrou pitrou commented May 8, 2019

@pitrou pitrou force-pushed the pitrou:pickle5 branch 2 times, most recently from c56f2dd to 745e7be May 8, 2019
@pitrou
Copy link
Member Author

@pitrou pitrou commented May 8, 2019

Rebased.

Copy link
Member

@terryjreedy terryjreedy left a comment

Doc comments

:class:`io.BytesIO` instance, or any other custom object that meets this
interface.
Arguments *file*, *protocol*, *fix_imports* and *buffer_callback* have
the same meaning as in :class:`Pickler`.

This comment has been minimized.

@terryjreedy

terryjreedy May 16, 2019
Member

I like having the redundancy removed, as it reduces cognitive load.

Doc/library/pickle.rst Outdated Show resolved Hide resolved
Doc/library/pickle.rst Outdated Show resolved Hide resolved
@pitrou pitrou force-pushed the pitrou:pickle5 branch from 745e7be to b27c96b May 17, 2019
@pitrou
Copy link
Member Author

@pitrou pitrou commented May 17, 2019

Thanks Terry! I've applied the suggested changes now.

Copy link
Member

@vstinner vstinner left a comment

Here is my review, enjoy :-) I don't know well the pickle module, so I mostly checked the doc and basic stuff.

By the way, I really like the overall change and the PEP: it's a great enhancement, thanks!

Doc/library/pickle.rst Outdated Show resolved Hide resolved
Doc/library/pickle.rst Show resolved Hide resolved

:class:`PickleBuffer` objects can only be serialized using pickle
protocol 5 or higher. They are eligible for
:ref:`out-of-band serialization <pickle-oob>`.

This comment has been minimized.

@vstinner

vstinner May 17, 2019
Member

Maybe add a reference to your PEP?

This comment has been minimized.

@pitrou

pitrou May 19, 2019
Author Member

It is referenced at the end of the "Out-of-band Buffers" section. Since it's quite specialized, I'm not sure it's useful to mention it also here.

Doc/library/pickle.rst Show resolved Hide resolved
Doc/library/pickle.rst Outdated Show resolved Hide resolved
Modules/_pickle.c Outdated Show resolved Hide resolved
Modules/_pickle.c Show resolved Hide resolved
Modules/_pickle.c Outdated Show resolved Hide resolved
Modules/_pickle.c Show resolved Hide resolved
Objects/object.c Show resolved Hide resolved
@pitrou pitrou force-pushed the pitrou:pickle5 branch from b27c96b to 6e8e93b May 19, 2019
@pitrou
Copy link
Member Author

@pitrou pitrou commented May 19, 2019

Thanks for the review @vstinner. I think I've addressed your comments now.

@pitrou pitrou force-pushed the pitrou:pickle5 branch from 6e8e93b to 08fa40b May 19, 2019
Copy link
Member

@vstinner vstinner left a comment

LGTM. I didn't see any major issue, but I didn't review carefully the C code. Nice PEP, nice implementation, thanks ;-)

@pitrou pitrou force-pushed the pitrou:pickle5 branch from 08fa40b to ade18e7 May 22, 2019
@pitrou
Copy link
Member Author

@pitrou pitrou commented May 26, 2019

I'm gonna merge soon since no other review came and 3.8b1 is due next Friday.

@pitrou pitrou force-pushed the pitrou:pickle5 branch from ade18e7 to 533d284 May 26, 2019
@pitrou pitrou merged commit 91f4380 into python:master May 26, 2019
5 checks passed
5 checks passed
Azure Pipelines PR #20190526.17 succeeded
Details
bedevere/issue-number Issue number 36785 found
Details
bedevere/news News entry found in Misc/NEWS.d
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@pitrou pitrou deleted the pitrou:pickle5 branch May 26, 2019
DinoV added a commit to DinoV/cpython that referenced this pull request Jan 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants
You can’t perform that action at this time.