Skip to content
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

Merged
merged 4 commits into from
May 9, 2018

Conversation

ilevkivskyi
Copy link
Member

@ilevkivskyi ilevkivskyi commented May 8, 2018

@ilevkivskyi
Copy link
Member Author

Note this will not backport to 3.6 cleanly. I will make a manual backport.

@gvanrossum
Copy link
Member

gvanrossum commented May 8, 2018 via email

@ilevkivskyi
Copy link
Member Author

It looks like all failures are unrelated except for a useful warning about bad .rst formatting. Let us see tomorrow how it goes.

@ilevkivskyi
Copy link
Member Author

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.

@ilevkivskyi ilevkivskyi merged commit 43d12a6 into python:master May 9, 2018
@miss-islington
Copy link
Contributor

Thanks @ilevkivskyi for the PR 🌮🎉.. I'm working now to backport this PR to: 3.7.
🐍🍒⛏🤖

@ilevkivskyi ilevkivskyi deleted the sync-typing branch May 9, 2018 01:23
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request May 9, 2018
This also fixes https://bugs.python.org/issue33420
(cherry picked from commit 43d12a6)

Co-authored-by: Ivan Levkivskyi <levkivskyi@gmail.com>
@bedevere-bot
Copy link

GH-6735 is a backport of this pull request to the 3.7 branch.

miss-islington added a commit that referenced this pull request May 9, 2018
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)
Copy link
Member

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.

Copy link
Member Author

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__:
Copy link
Member

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.

Copy link
Member Author

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?

Copy link
Member

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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants