-
-
Notifications
You must be signed in to change notification settings - Fork 31.4k
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
bpo-28556: Minor fixes for typing module #6732
Conversation
Note this will not backport to 3.6 cleanly. I will make a manual backport. |
I approved, but tests are failing...
|
It looks like all failures are unrelated except for a useful warning about bad |
Hm, all tests passed now. So either they were flakes, or just they got fixed in the meantime. I think it is safe to merge now. |
Thanks @ilevkivskyi for the PR 🌮🎉.. I'm working now to backport this PR to: 3.7. |
This also fixes https://bugs.python.org/issue33420 (cherry picked from commit 43d12a6) Co-authored-by: Ivan Levkivskyi <levkivskyi@gmail.com>
GH-6735 is a backport of this pull request to the 3.7 branch. |
This also fixes https://bugs.python.org/issue33420 (cherry picked from commit 43d12a6) Co-authored-by: Ivan Levkivskyi <levkivskyi@gmail.com>
@@ -1385,6 +1390,7 @@ def __new__(cls, typename, bases, ns): | |||
"follow default field(s) {default_names}" | |||
.format(field_name=field_name, | |||
default_names=', '.join(defaults_dict.keys()))) | |||
nm_tpl.__new__.__annotations__ = collections.OrderedDict(types) |
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.
Why OrderedDict? dict
is ordered.
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.
To make code closer to the backported version (plus there is another occurrence above). At some point however we might want to clean-up all this, but I propose to make it a separate PR.
@@ -835,7 +836,11 @@ def __new__(cls, *args, **kwds): | |||
if cls is Generic: | |||
raise TypeError("Type Generic cannot be instantiated; " | |||
"it can be used only as a base class") | |||
return super().__new__(cls) | |||
if super().__new__ is object.__new__: |
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.
I think and cls.__init__ is not object.__init__
is needed here.
The test:
class A(Generic[T]):
pass
with self.assertRaises(TypeError):
A('foo')
And would extract the subexpression super().__new__
into a variable.
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.
Hm, indeed, in the original PR in typing repo we were concerned that it may fail when it shouldn't, but forgot that it also should fail when it should. @gvanrossum what do you think?
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.
Yes, @serhiy-storchaka is right. This is definitely a wart -- object.__init__
and object.__new__
each ignore extra arguments only when the other is overridden (and they themselves are not).
This also fixes https://bugs.python.org/issue33420
https://bugs.python.org/issue28556