-
-
Notifications
You must be signed in to change notification settings - Fork 33.7k
bpo-28556: Minor fixes for typing module #6732
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
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 <[email protected]>
|
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 <[email protected]>
| "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.
| 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