Skip to content

Conversation

@serhiy-storchaka
Copy link
Member

It is a simpler alternative of #92249.

#87390

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, this is indeed a better solution.

Could you add tests for pickling starred aliases though? Your new tests only cover equality.

self.assertEqual(loaded.__origin__, alias.__origin__)
self.assertEqual(loaded.__args__, alias.__args__)
self.assertEqual(loaded.__parameters__, alias.__parameters__)
self.assertEqual(type(loaded), type(alias))
Copy link
Member

Choose a reason for hiding this comment

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

maybe assertIs?

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

It is the same for types. And I do not want to restart the CI testing for such minor change. 😉

@Fidget-Spinner
Copy link
Member

LGTM. But IMO this needs a news, and like Jelle mentioned more tests (maybe we can take the ones from Mathew's PR and credit him?)

@serhiy-storchaka
Copy link
MemberAuthor

The old tests already cover pickling. They were passed, because equality was broken in the same way as pickling. I fixed equality in #92335, but it made pickling tests failing.

Since it fixes not yet released code, there is no sense in adding a NEWS entry. It can only confuse readers.

@serhiy-storchaka
Copy link
MemberAuthor

Additional tests in Mathew's PR are for the third parameter of GenericAlias. That change is not included in this PR.

@serhiy-storchakaserhiy-storchaka merged commit 1ed8d03 into python:mainMay 5, 2022
@serhiy-storchakaserhiy-storchaka deleted the unpacked-tuple-eq-pickle branch May 5, 2022 17:16
@mrahtz
Copy link
Contributor

Thanks Serhiy!

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

Labels

skip newstopic-typingtype-bugAn unexpected behavior, bug, or error

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@serhiy-storchaka@Fidget-Spinner@mrahtz@JelleZijlstra@bedevere-bot