Skip to content

Conversation

@mrahtz
Copy link
Contributor

@mrahtzmrahtz 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-SpinnerFidget-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.

Copy link
Member

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

Copy link
Member

@Fidget-SpinnerFidget-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!

@mrahtzmrahtz closed this Apr 5, 2022
@mrahtzmrahtz 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.

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

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

@mrahtzmrahtzmannequin mentioned this pull request Apr 11, 2022
@JelleZijlstraJelleZijlstra self-assigned this Apr 21, 2022
@JelleZijlstraJelleZijlstra merged commit 5e130a8 into python:mainApr 22, 2022
@mrahtzmrahtz deleted the pep646-pickling-tests branch April 22, 2022 18:06
@mrahtz
Copy link
ContributorAuthor

Thanks, Jelle!

Sign up for freeto 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

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