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

add AsyncGenerator #346

Merged
merged 3 commits into from
Jan 18, 2017
Merged

add AsyncGenerator #346

merged 3 commits into from
Jan 18, 2017

Conversation

JelleZijlstra
Copy link
Member

Looks like this was inadvertently omitted from 3.6 as released.

Should I submit a separate patch to CPython directly to add documentation?

@gvanrossum
Copy link
Member

Thanks! I will merge this after 3.5.3 has been released.

@gvanrossum
Copy link
Member

And yes, please send a separate doc patch.

JelleZijlstra added a commit to JelleZijlstra/typeshed that referenced this pull request Jan 7, 2017
JelleZijlstra added a commit to JelleZijlstra/typeshed that referenced this pull request Jan 7, 2017
@JelleZijlstra
Copy link
Member Author

extra=_AG_base):
__slots__ = ()

def __new__(cls, *args, **kwds):
Copy link
Member

Choose a reason for hiding this comment

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

Since you always inherit from collections.abc there is no need to override __new__. It will be anyway not possible to instantiate typing.AsyncGenerator because collections.abc.AsyncGenerator has abstract methods.

Copy link
Member Author

Choose a reason for hiding this comment

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

This was mostly copied from the Generator implementation just above. It is possible to instantiate a subclass of AsyncGenerator, which would use this __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 don't think I understand your point. The only way to instantiate a subclass of an ABC is to implement all abstract methods. Moreover, there is no way a subclass could "use" this __new__, it only exists everywhere in typing to prohibit direct instantiation of generic version of concrete collections and similar things.

__new__ for Generator is necessary, since it could inherit from types.GeneratorType.

@@ -1320,6 +1320,7 @@ def blah():


ASYNCIO = sys.version_info[:2] >= (3, 5)
ASYNC_GENERATOR = sys.version_info[:2] >= (3, 6)
Copy link
Member

Choose a reason for hiding this comment

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

Why is this line needed? The variable is not used AFAICT.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, looks like I didn't end up needing it. I'll remove it.

@gvanrossum
Copy link
Member

@ilevkivskyi Are you happy with this code now? Then I can merge it. (Honestly I want to give you commit privs here if you don't already have them.)

@ilevkivskyi
Copy link
Member

Are you happy with this code now?

There are two very minor points: the variable _AG_base is used only once, and the last test self.assertNotIsInstance(type(g), G) is not very useful, I would add self.assertNotIsInstance(g, G). Otherwise I am happy.

Honestly I want to give you commit privs here if you don't already have them.

I am glad that you trust me. I don't have the privileges. I think it indeed may simplify things.

@gvanrossum
Copy link
Member

I invited you. Go ahead and commit this PR when you are happy with it.

@ilevkivskyi
Copy link
Member

Probably the best way is to merge this now and I will add those minor changes myself, so that #355 will be "unblocked" very soon.

@ilevkivskyi ilevkivskyi merged commit 56abd8f into python:master Jan 18, 2017
JelleZijlstra added a commit to JelleZijlstra/typeshed that referenced this pull request Jan 18, 2017
gvanrossum pushed a commit to python/typeshed that referenced this pull request Jan 18, 2017
@JelleZijlstra JelleZijlstra deleted the asyncgen branch January 25, 2017 03:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants