Skip to content
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

bpo-43224: Implement pickling of TypeVarTuples #32119

Merged
merged 12 commits into from
Apr 22, 2022

Conversation

mrahtz
Copy link
Contributor

@mrahtz mrahtz commented Mar 25, 2022

Guess who learned a whole bunch about how pickling works :)

Notes:

  • I was a bit horrified to realise that the class I'd renamed to _BoundVarianceMixin also contains a key ingredient for pickling of things that use it! I've refactored its __reduce__ into a separate mixin for clarity.
  • I'm not entirely sure whether these tests make sense - is assertEqual a reasonable way of testing this functionality? Am I correct in thinking we do need to test both assertEqual and assertIs for TypeVarTuples and unpacked TypeVarTuples?

This PR doesn't include implementation of pickling support for unpacked native tuple; I'll do that in a future PR. (And I guess we'll also need to add tests for copy?)

https://bugs.python.org/issue43224

Copy link
Member

@Fidget-Spinner Fidget-Spinner left a comment

Choose a reason for hiding this comment

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

correct in thinking we do need to test both assertEqual and assertIs for TypeVarTuples and unpacked TypeVarTuples?

IIRC, assertIs for pickle/unpickling isn't guaranteed, so we don't need to test for that.

Other than the tests, the code in typing.py LGTM.

Lib/test/test_typing.py Show resolved Hide resolved
Copy link
Member

@JelleZijlstra JelleZijlstra left a comment

Choose a reason for hiding this comment

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

Thanks, I think these tests are a bit repetitive.

Lib/test/test_typing.py Outdated Show resolved Hide resolved
Lib/test/test_typing.py Outdated Show resolved Hide resolved
Lib/test/test_typing.py Outdated Show resolved Hide resolved
Lib/test/test_typing.py Outdated Show resolved Hide resolved
Copy link
Member

@Fidget-Spinner Fidget-Spinner left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@mrahtz mrahtz closed this Apr 5, 2022
@mrahtz mrahtz deleted the pep646-pickling-tests branch April 5, 2022 18:34
@Fidget-Spinner
Copy link
Member

Fidget-Spinner commented Apr 6, 2022

Why was the PR closed @mrahtz ?

Edit: Nevermind, I just read the bug report.

@mrahtz mrahtz restored the pep646-pickling-tests branch April 6, 2022 08:18
@mrahtz mrahtz reopened this Apr 6, 2022
@mrahtz
Copy link
Contributor Author

mrahtz commented Apr 6, 2022

Lol, my bad - I was cleaning up some old branches, and forgotten this hadn't yet been merged 😅

@mrahtz mrahtz mannequin mentioned this pull request Apr 11, 2022
Lib/typing.py Outdated Show resolved Hide resolved
Lib/test/test_typing.py Outdated Show resolved Hide resolved
Lib/test/test_typing.py Outdated Show resolved Hide resolved
Lib/test/test_typing.py Outdated Show resolved Hide resolved
Lib/test/test_typing.py Outdated Show resolved Hide resolved
@JelleZijlstra JelleZijlstra self-assigned this Apr 21, 2022
@JelleZijlstra JelleZijlstra merged commit 5e130a8 into python:main Apr 22, 2022
@mrahtz mrahtz deleted the pep646-pickling-tests branch April 22, 2022 18:06
@mrahtz
Copy link
Contributor Author

mrahtz commented Apr 22, 2022

Thanks, Jelle!

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

Successfully merging this pull request may close these issues.

6 participants