◐ Shell
reader mode source ↗
Skip to content

bpo-43224: Implement pickling of TypeVarTuples#32119

Merged
JelleZijlstra merged 12 commits into
python:mainfrom
mrahtz:pep646-pickling-tests
Apr 22, 2022
Merged

bpo-43224: Implement pickling of TypeVarTuples#32119
JelleZijlstra merged 12 commits into
python:mainfrom
mrahtz:pep646-pickling-tests

Conversation

@mrahtz

@mrahtz mrahtz commented Mar 25, 2022 •
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Contributor

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

Fidget-Spinner
Fidget-Spinner reviewed Mar 26, 2022
View reviewed changes

@Fidget-Spinner Fidget-Spinner 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.

Hide comment

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.

JelleZijlstra
JelleZijlstra reviewed Mar 26, 2022
View reviewed changes

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

Hide comment

Thanks, I think these tests are a bit repetitive.

Fidget-Spinner
Fidget-Spinner approved these changes Apr 1, 2022
View reviewed changes

@Fidget-Spinner Fidget-Spinner 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.

Hide comment

LGTM. Thanks!

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

Fidget-Spinner commented Apr 6, 2022 •
edited
Loading

Copy link
Copy Markdown
Member

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

mrahtz commented Apr 6, 2022

Copy link
Copy Markdown
Contributor Author

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
Closed
@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

mrahtz commented Apr 22, 2022

Copy link
Copy Markdown
Contributor Author

Thanks, Jelle!

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

Reviewers

@JelleZijlstra JelleZijlstra JelleZijlstra approved these changes

@Fidget-Spinner Fidget-Spinner Fidget-Spinner approved these changes

@gvanrossum gvanrossum Awaiting requested review from gvanrossum

Assignees

@JelleZijlstra JelleZijlstra

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@mrahtz @Fidget-Spinner @JelleZijlstra @the-knights-who-say-ni @bedevere-bot @AlexWaygood