-
-
Notifications
You must be signed in to change notification settings - Fork 30.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
gh-87390: Fix starred tuple equality and pickling (alt) #92337
gh-87390: Fix starred tuple equality and pickling (alt) #92337
Conversation
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, this is indeed a better solution.
Could you add tests for pickling starred aliases though? Your new tests only cover equality.
@@ -394,6 +396,7 @@ def test_pickle(self): | |||
self.assertEqual(loaded.__origin__, alias.__origin__) | |||
self.assertEqual(loaded.__args__, alias.__args__) | |||
self.assertEqual(loaded.__parameters__, alias.__parameters__) | |||
self.assertEqual(type(loaded), type(alias)) |
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.
maybe assertIs?
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.
It is the same for types. And I do not want to restart the CI testing for such minor change. 😉
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?) |
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. |
Additional tests in Mathew's PR are for the third parameter of GenericAlias. That change is not included in this PR. |
Thanks Serhiy! |
It is a simpler alternative of #92249.
#87390