Skip to content

Pickle 5 #3636

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
Merged
merged 5 commits into from
Jan 23, 2020
Merged

Pickle 5 #3636

merged 5 commits into from
Jan 23, 2020

Conversation

crusaderky
Copy link
Contributor

@crusaderky crusaderky commented Jan 20, 2020 •
edited
Loading

Closes #3635

@crusaderky crusaderky changed the title WIP: Pickle 5 Pickle 5 Jan 20, 2020
@crusaderky
Copy link
Contributor Author

The suite 'mypy self test' is failing with a very uninformative message OSError: cannot send to <Channel id=1 closed>; help would be appreciated

https://travis-ci.org/python/typeshed/jobs/639597167?utm_medium=notification&utm_source=github_status

hauntsaninja pushed a commit to hauntsaninja/typeshed that referenced this pull request Jan 21, 2020
retrigger CI build
43cb30a 
CI had some issues https://travis-ci.org/python/typeshed/jobs/639617045
This also affected python#3636
@hauntsaninja
Copy link
Collaborator

Try forcing another CI build? Things seem to be working now, eg, for #3634

@srittau
Copy link
Collaborator

srittau commented Jan 21, 2020

I restarted CI.

@crusaderky
Copy link
Contributor Author

Thank you - all green now. Ready for review.

hauntsaninja
hauntsaninja approved these changes Jan 22, 2020
View reviewed changes
Copy link
Collaborator

@hauntsaninja hauntsaninja left a comment

Choose a reason for hiding this comment

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

Lgtm.
bytes_object for pickle.loads should be named data, but that error precedes this PR.

One other weird note (that doesn't change anything) is that the docs say "If buffers is None (the default)" at https://docs.python.org/3/library/pickle.html#pickle.Unpickler, but for load and loads it looks like the default arg for buffers is empty tuple if it's using the version imported from _pickle.

srittau
srittau requested changes Jan 23, 2020
View reviewed changes
Copy link
Collaborator

@srittau srittau left a comment

Choose a reason for hiding this comment

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

Thanks, a few remarks below.

@crusaderky
Copy link
Contributor Author

All comments implemented, with the exception of the () default for buffers where I feel the cpython implementation should be corrected

@crusaderky
Copy link
Contributor Author

@hauntsaninja I opened https://bugs.python.org/issue39435

@srittau srittau merged commit 03c9faa into python:master Jan 23, 2020
moreati added a commit to moreati/typeshed that referenced this pull request Feb 22, 2020
@moreati
stdlib: Remove duplicated Pickler.reducer_override()
59b6b6f 
The declaration was present twice. One from python#3358 with the bulk Python
3.8 support, the other from python#3636 with Pickle protocol 5 support.
@moreati moreati mentioned this pull request Feb 22, 2020
Merged
moreati added a commit to moreati/typeshed that referenced this pull request Feb 22, 2020
@moreati
stdlib: Remove duplicated Pickler.reducer_override()
b45af5e 
The declaration was present twice. One from python#3358 with the bulk Python
3.8 support, the other from python#3636 with Pickle protocol 5 support.
@crusaderky crusaderky deleted the pickle5 branch October 14, 2020 14:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@srittau srittau srittau approved these changes

@hauntsaninja hauntsaninja hauntsaninja approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

New arguments for pickle 5
3 participants
@crusaderky @hauntsaninja @srittau