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.

Sign up for GitHub

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

Jump to bottom

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 •
edited
Loading

@pitrou pitrou requested a review from a team as a code owner May 23, 2018 18:31
@pitrou pitrou added DO-NOT-MERGE type-feature A feature request or enhancement and removed awaiting merge labels May 23, 2018
@pitrou pitrou force-pushed the pickle5 branch 3 times, most recently from cef20d0 to 8943edf Compare May 3, 2019 18:50
@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 commented May 3, 2019

@pierreglaser @ogrisel

pitrou
pitrou commented May 3, 2019
View reviewed changes
Modules/_pickle.c Outdated Show resolved Hide resolved
pitrou
pitrou commented May 3, 2019
View reviewed changes
Modules/_pickle.c Outdated

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@ogrisel ogrisel May 4, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

pierreglaser
pierreglaser reviewed May 7, 2019
View reviewed changes
Lib/pickle.py Outdated Show resolved Hide resolved
@pitrou
Copy link
Member Author

pitrou commented May 8, 2019

@avassalotti @serhiy-storchaka

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

pitrou commented May 8, 2019

Rebased.

terryjreedy
terryjreedy reviewed May 16, 2019
View reviewed changes
Copy link
Member

@terryjreedy terryjreedy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doc comments

Doc/library/pickle.rst Outdated
: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`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member Author

pitrou commented May 17, 2019

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

vstinner
vstinner reviewed May 17, 2019
View reviewed changes
Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Doc/library/pickle.rst

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add a reference to your PEP?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member Author

pitrou commented May 19, 2019

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

vstinner
vstinner approved these changes May 20, 2019
View reviewed changes
Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@pitrou
Copy link
Member Author

pitrou commented May 26, 2019

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

@pitrou pitrou merged commit 91f4380 into python:master May 26, 2019
@pitrou pitrou deleted the pickle5 branch May 26, 2019 15:10
@pitrou pitrou mentioned this pull request May 26, 2019
Merged
DinoV pushed a commit to DinoV/cpython that referenced this pull request Jan 14, 2020
@pitrou @DinoV
bpo-36785: PEP 574 implementation (pythonGH-7076)
7e3323c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ogrisel ogrisel ogrisel left review comments

@ammaraskar ammaraskar ammaraskar left review comments

@pierreglaser pierreglaser pierreglaser left review comments

@terryjreedy terryjreedy terryjreedy left review comments

@vstinner vstinner vstinner approved these changes

Assignees
No one assigned
Labels
type-feature A feature request or enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@pitrou @ogrisel @vstinner @ammaraskar @pierreglaser @terryjreedy @the-knights-who-say-ni @bedevere-bot