Skip to content

Conversation

@ambv
Copy link
Contributor

@ambv ambv commented Apr 2, 2018

This makes performance better and produces shorter pickles. This change is backwards compatible up to the oldest currently supported version of Python (3.4).

https://bugs.python.org/issue23403

Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

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

Please update docstrings, the module documentation, and What's New.

Copy link
Member

Choose a reason for hiding this comment

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

Since this is a reStructuredText, we can add links: :mod:`pickle`, :pep:`3154`.

@ambv
Copy link
Contributor Author

ambv commented Apr 3, 2018

Lib/test/pickletester.py:AbstractPicklerUnpicklerObjectTests.test_clear_pickler_memo is failing for CPicklerUnpicklerObjectTests only. This seems like a bug to me since it passes on the same protocol in the pure-Python implementation (PyPicklerUnpicklerObjectTests).

@pitrou Could you take a look?

Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

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

There is other mention of the default protocol 3 in the module documentation.

Regenerate Argument Clinic files and fix tests.

@ambv
Copy link
Contributor Author

ambv commented Apr 3, 2018

@serhiy-storchaka, updated the remaining comment on default protocol 3, regenerated Argument Clinic. For the failing test, see my comment above, I'm not sure what the fix should be here.

@serhiy-storchaka
Copy link
Member

I think there is a bug in C implementation. Opened bpo-33209.

Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

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

And maybe add a versionchanged directive somewhere in pickle.rst?

Copy link
Member

Choose a reason for hiding this comment

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

Tiny nit: it is preferable to use an en-dash for the version range.

Python 3.0--3.7

This makes performance better and produces shorter pickles. This change is backwards compatible up to the oldest currently supported version of Python (3.4).
@ambv
Copy link
Contributor Author

ambv commented Apr 3, 2018

Responded to remaining nits and rebased to include GH-6363. All tests pass now.

@ambv ambv merged commit c51d8c9 into python:master Apr 4, 2018
@bedevere-bot
Copy link

@ambv: Please replace # with GH- in the commit message next time. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants