bpo-36785: PEP 574 implementation#7076
Conversation
pitrou
added
DO-NOT-MERGE
type-feature
pitrou
force-pushed
the
pickle5
branch
3 times, most recently
from
cef20d0 to
8943edf
Compare
pitrou
changed the title
Modules/_pickle.c
Outdated
There was a problem hiding this comment.
@ogrisel Do you think we need to support "file-like" objects without a readinto() method?
There was a problem hiding this 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.
There was a problem hiding this comment.
There doesn't seem to be any. I'll remove this snippet.
There was a problem hiding this 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...
There was a problem hiding this comment.
Was the fallback ever added back? Looks like this came up as a real issue on https://bugs.python.org/issue39681
There was a problem hiding this comment.
Thanks for the ping. I'll reply on the issue.
pitrou
force-pushed
the
pickle5
branch
2 times, most recently
from
c56f2dd to
745e7be
Compare
|
Rebased. |
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`. |
There was a problem hiding this comment.
I like having the redundancy removed, as it reduces cognitive load.
|
Thanks Terry! I've applied the suggested changes now. |
vstinner
left a comment
vstinner
left a comment
There was a problem hiding this 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!
|
|
||
| :class:`PickleBuffer` objects can only be serialized using pickle | ||
| protocol 5 or higher. They are eligible for | ||
| :ref:`out-of-band serialization <pickle-oob>`. |
There was a problem hiding this comment.
Maybe add a reference to your PEP?
There was a problem hiding this 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.
|
Thanks for the review @vstinner. I think I've addressed your comments now. |
vstinner
left a comment
vstinner
left a comment
There was a problem hiding this comment.
LGTM. I didn't see any major issue, but I didn't review carefully the C code. Nice PEP, nice implementation, thanks ;-)
|
I'm gonna merge soon since no other review came and 3.8b1 is due next Friday. |
8 participants
https://bugs.python.org/issue36785