FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

gh-87390: Fix starred tuple equality and pickling (alt) by serhiy-storchaka · Pull Request #92337 · python/cpython · GitHub

/ cpython Public

gh-87390: Fix starred tuple equality and pickling (alt) - #92337

Merged
serhiy-storchaka merged 2 commits into
python:mainfrom
serhiy-storchaka:unpacked-tuple-eq-pickle
May 5, 2022
ConversationCommits (2)ChecksFiles changed
Merged

gh-87390: Fix starred tuple equality and pickling (alt)#92337
serhiy-storchaka merged 2 commits into
python:mainfrom
serhiy-storchaka:unpacked-tuple-eq-pickle

Conversation

serhiy-storchaka commented May 5, 2022

Copy link
Copy Markdown
Member

It is a simpler alternative of #92249.

#87390

serhiy-storchaka added type-bug An unexpected behavior, bug, or error skip news topic-typing labels May 5, 2022
serhiy-storchaka mentioned this pull request May 5, 2022
Closed
JelleZijlstra reviewed May 5, 2022
View reviewed changes

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Thanks, this is indeed a better solution.

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

Comment thread Lib/test/test_genericalias.py
self.assertEqual(loaded.__origin__, alias.__origin__)
self.assertEqual(loaded.__args__, alias.__args__)
self.assertEqual(loaded.__parameters__, alias.__parameters__)
self.assertEqual(type(loaded), type(alias))

JelleZijlstra May 5, 2022

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

maybe assertIs?

serhiy-storchaka May 5, 2022

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Fidget-Spinner commented May 5, 2022

Copy link
Copy Markdown
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 commented May 5, 2022

Copy link
Copy Markdown
Member Author

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 commented May 5, 2022

Copy link
Copy Markdown
Member Author

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

serhiy-storchaka merged commit 1ed8d03 into python:main May 5, 2022
serhiy-storchaka deleted the unpacked-tuple-eq-pickle branch May 5, 2022 17:16

mrahtz commented May 7, 2022

Copy link
Copy Markdown
Contributor

Thanks Serhiy!

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

JelleZijlstra JelleZijlstra left review comments

Assignees

No one assigned

Labels

skip news topic-typing type-bug An unexpected behavior, bug, or error

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

Back | FazBrowse Home | New Git URL