GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — bpo-43224: Implement pickling of TypeVarTuples by mrahtz · Pull Request #32119 · python/cpython

Conversation

@mrahtz

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

Uh oh!

There was an error while loading. Please reload this page.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@mrahtz
Implement pickling of TypeVarTuples
5bdeab6
@mrahtz
mrahtz requested review from Fidget-Spinner, JelleZijlstra and gvanrossum as code owners March 25, 2022 22:11
@bedevere-bot bedevere-bot added the awaiting review label Mar 25, 2022
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Mar 25, 2022
Fidget-Spinner
Fidget-Spinner reviewed Mar 26, 2022
View reviewed changes

@Fidget-Spinner Fidget-Spinner left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this 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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread Lib/test/test_typing.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

JelleZijlstra
JelleZijlstra reviewed Mar 26, 2022
View reviewed changes

@JelleZijlstra JelleZijlstra left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this 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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread Lib/test/test_typing.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Lib/test/test_typing.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Lib/test/test_typing.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Lib/test/test_typing.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

mrahtz added 3 commits March 28, 2022 18:30
@mrahtz
Remove redundant tests
2ea4127
@mrahtz
Test all pickle protocol versions
f234214
@mrahtz
Switch other tests to use @all_pickle_protocols too
de68232
@Fidget-Spinner Fidget-Spinner added the skip news label Mar 29, 2022
@mrahtz
Revert "Switch other tests to use @all_pickle_protocols too"
69197c2
This reverts commit de68232.
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

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LGTM. Thanks!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting review labels Apr 1, 2022
@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

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

Why was the PR closed @mrahtz ?

Edit: Nevermind, I just read the bug report.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@mrahtz
Merge branch 'main' into pep646-pickling-tests
72e3db5
@mrahtz mrahtz mannequin mentioned this pull request Apr 11, 2022
Add support for PEP 646 #87390
Closed
JelleZijlstra
JelleZijlstra reviewed Apr 13, 2022
View reviewed changes
Comment thread Lib/typing.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Lib/test/test_typing.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Lib/test/test_typing.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Lib/test/test_typing.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Lib/test/test_typing.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

@AlexWaygood AlexWaygood added the topic-typing label Apr 13, 2022
mrahtz and others added 4 commits April 15, 2022 09:43
@mrahtz @JelleZijlstra
Tweak comment wording
9a38a91
Co-authored-by: Jelle Zijlstra <[email protected]>
@mrahtz
Remove test_pickling_then_unpickling_variadic_class_results_in_equality
8fd2b4f
@mrahtz
Remove parenthetic from comment
ea8d5fd
@mrahtz
Replace assertEqual with assertIs
d638d92
@mrahtz
Remove test_pickling_then_unpickling_variadic_func_results_in_equality
c5d8dc6
JelleZijlstra
JelleZijlstra approved these changes Apr 21, 2022
View reviewed changes
@JelleZijlstra
Merge branch 'main' into pep646-pickling-tests
e5501f4
@JelleZijlstra JelleZijlstra self-assigned this Apr 21, 2022
@JelleZijlstra
JelleZijlstra merged commit 5e130a8 into python:main Apr 22, 2022
@bedevere-bot bedevere-bot removed the awaiting merge label 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!

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@JelleZijlstra JelleZijlstra

Labels

skip news topic-typing

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

6 participants

@mrahtz @Fidget-Spinner @JelleZijlstra @the-knights-who-say-ni @bedevere-bot @AlexWaygood
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.