← 返回首页
bpo-36785: PEP 574 implementation by pitrou · Pull Request #7076 · python/cpython · GitHub
Skip to content

Navigation Menu

Toggle navigation
Sign in
Appearance settings
Search or jump to...

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Include my email address so I can be contacted
Cancel Submit feedback

Saved searches

Use saved searches to filter your results more quickly

Cancel Create saved search
Appearance settings
Resetting focus

bpo-36785: PEP 574 implementation#7076

Merged
pitrou merged 5 commits into
python:masterfrom
pitrou:pickle5
May 26, 2019
Merged

bpo-36785: PEP 574 implementation#7076
pitrou merged 5 commits into
python:masterfrom
pitrou:pickle5

Conversation

pitrou commented May 23, 2018 •
edited
Loading

Copy link
Copy Markdown
Member

pitrou requested a review from a team as a code owner May 23, 2018 18:31
pitrou added DO-NOT-MERGE type-feature A feature request or enhancement and removed awaiting merge labels May 23, 2018
pitrou force-pushed the pickle5 branch 3 times, most recently from cef20d0 to 8943edf Compare May 3, 2019 18:50
pitrou changed the title [WIP] [DO NOT MERGE] PEP 574 implementation PEP 574 implementation May 3, 2019
pitrou changed the title PEP 574 implementation bpo-36785: PEP 574 implementation May 3, 2019

pitrou commented May 3, 2019

Copy link
Copy Markdown
Member Author

@pierreglaser @ogrisel

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

pitrou May 3, 2019

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality Hide comment

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

ogrisel May 4, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality Hide comment

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.

pitrou May 4, 2019

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality Hide comment

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

pitrou May 4, 2019

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality Hide comment

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

ammaraskar Feb 19, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality Hide comment

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

pitrou Feb 19, 2020

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality Hide comment

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

pierreglaser reviewed May 7, 2019
View reviewed changes
Comment thread Lib/pickle.py Outdated Show resolved Hide resolved

pitrou commented May 8, 2019

Copy link
Copy Markdown
Member Author

@avassalotti @serhiy-storchaka

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

pitrou commented May 8, 2019

Copy link
Copy Markdown
Member Author

Rebased.

terryjreedy reviewed May 16, 2019
View reviewed changes

terryjreedy left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality Hide comment

Doc comments

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

terryjreedy May 16, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality Hide comment

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

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

pitrou commented May 17, 2019

Copy link
Copy Markdown
Member Author

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

vstinner reviewed May 17, 2019
View reviewed changes

vstinner left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality Hide 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!

Comment thread Doc/library/pickle.rst Outdated Show resolved Hide resolved
Comment thread Doc/library/pickle.rst Show resolved Hide resolved
Comment thread 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>`.

vstinner May 17, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality Hide comment

Maybe add a reference to your PEP?

pitrou May 19, 2019

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality Hide comment

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.

Comment thread Doc/library/pickle.rst Show resolved Hide resolved
Comment thread Doc/library/pickle.rst Outdated Show resolved Hide resolved
12 hidden conversations Load more…
Comment thread Modules/_pickle.c Outdated Show resolved Hide resolved
Comment thread Modules/_pickle.c Outdated Show resolved Hide resolved
Comment thread Modules/_pickle.c Outdated Show resolved Hide resolved
Comment thread Modules/_pickle.c Outdated Show resolved Hide resolved
Comment thread Objects/object.c Outdated Show resolved Hide resolved

pitrou commented May 19, 2019

Copy link
Copy Markdown
Member Author

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

vstinner approved these changes May 20, 2019
View reviewed changes

vstinner left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality Hide comment

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

34 hidden items Load more…

pitrou commented May 26, 2019

Copy link
Copy Markdown
Member Author

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

pitrou merged commit 91f4380 into python:master May 26, 2019
pitrou deleted the pickle5 branch May 26, 2019 15:10
pitrou mentioned this pull request May 26, 2019
Merged
DinoV pushed a commit to DinoV/cpython that referenced this pull request Jan 14, 2020
bpo-36785: PEP 574 implementation (pythonGH-7076)
7e3323c
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

ammaraskar ammaraskar left review comments
terryjreedy terryjreedy left review comments
vstinner vstinner approved these changes
+2 more reviewers
ogrisel ogrisel left review comments
pierreglaser pierreglaser left review comments
Reviewers whose approvals may not affect merge requirements

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

Footer

© 2026 GitHub, Inc.