Skip to content

/ cpython

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

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

https://bugs.python.org/issue36785
@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

@pierreglaser @ogrisel
@pitrou pitrou force-pushed the pitrou:pickle5 branch from 8943edf to 25cbd5e May 3, 2019
@pitrou
pitrou reviewed May 3, 2019
View changes
Modules/_pickle.c Outdated Show resolved Hide resolved
@pitrou
pitrou reviewed May 3, 2019
View 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 */

This comment has been minimized.

Sign in to view
@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.

Sign in to view
@ogrisel

ogrisel May 4, 2019 •
edited
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.

Sign in to view
@pitrou

pitrou May 4, 2019
Author Member

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

This comment has been minimized.

Sign in to view
@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.

Sign in to view
@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.

Sign in to view
@pitrou

pitrou Feb 19, 2020
Author Member

Thanks for the ping. I'll reply on the issue.
@pierreglaser
pierreglaser reviewed May 7, 2019
View changes
Lib/pickle.py Outdated Show resolved Hide resolved
@pitrou
Copy link
Member Author

@pitrou pitrou commented May 8, 2019

@avassalotti @serhiy-storchaka
@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.
@terryjreedy
terryjreedy reviewed May 16, 2019
View changes
Copy link
Member

@terryjreedy terryjreedy left a comment

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

This comment has been minimized.

Sign in to view
@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.
@vstinner
vstinner reviewed May 17, 2019
View changes
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
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>`.

This comment has been minimized.

Sign in to view
@vstinner

vstinner May 17, 2019
Member

Maybe add a reference to your PEP?

This comment has been minimized.

Sign in to view
@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
@vstinner
vstinner approved these changes May 20, 2019
View changes
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 added 5 commits Apr 30, 2019
@pitrou
bpo-36785: PEP 574 implementation
4bf0b25
@pitrou
Remove special case for file-like objects without a readinto().
9d85ea6
@pitrou
Add documentation
5dd7f6a
@pitrou
Doc fixes
2a5a316
@pitrou
Address review comments
Loading status checks…
533d284
@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
@pitrou pitrou mentioned this pull request May 26, 2019
Merged
DinoV added 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

@ammaraskar ammaraskar

@pierreglaser pierreglaser

@terryjreedy terryjreedy

@vstinner vstinner

@python/windows-team windows-team

Assignees
No one assigned
Labels
CLA signed type-enhancement
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants
@pitrou @ogrisel @vstinner @ammaraskar @pierreglaser @terryjreedy @the-knights-who-say-ni @bedevere-bot
You can’t perform that action at this time.