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

Added Python 3.5 “async for” compatibility #542

Merged
merged 1 commit into from
Oct 18, 2015

Conversation

flying-sheep
Copy link
Contributor

Instead of

while True:
    msg = await dataqueue.read()
    ...

do:

async for msg in dataqueue:
    ...

TODO: how to handle EofStream?

@asvetlov
Copy link
Member

asvetlov commented Oct 3, 2015

EofStream should be converted to StopAsyncIterator.
Tests should be added.
Documentation -- updated.
What's about iterating over chunks also (.read(), .readexactly() and .readeany() should be supported too I believe).

@flying-sheep
Copy link
Contributor Author

OK! but i don’t get the last point.

what do you mean by “supported”? we can only add one __aiter__ and one __anext__ function here.

we could of course add explicit asnc generator functions, e.g. aiter_any, which would crate an async iterator that loops data as available instead of lines.

readexactly is parameterized, so it makes no sense to make __anext__ defer to it. read and readany are basically the same when called unparameterized, so it makes no sense to distinguish.

so:

  1. what should __anext__ do for the stream readers? readany or readline?
  2. how to support the other? aiter_lines()/aiter_any()/aiter_exactly(n) methods? which of those?

@asvetlov
Copy link
Member

asvetlov commented Oct 3, 2015

Well, .readexactly() doesn't make sense.
My initial intention was adding a way by both iterating over fixed size chunks and any present data.

Something like

async for chunk in resp.content.iter_chunked(1024):
    ...

async for slice in resp.content.iter_any():
    ...

Does it make sense?

Default iteration for StreamReader should be line-mode, sure -- like iteration over opened text file.

@flying-sheep
Copy link
Contributor Author

OK, then we’re on the same page 😄

we now only need a way to indicate in the docs that those methods are only available on python 3.5. is there a sphinx directive for that?

maybe we should also version-gate them?

class StreamReader(...):

    def __init__(self, ...):
        ...

    if python_version > (3, 5):
        def iter_any(self):
            ...

@asvetlov
Copy link
Member

asvetlov commented Oct 3, 2015

Yes, lease use check like https://github.com/KeepSafe/aiohttp/blob/master/aiohttp/client.py#L447

I believe just versionadded directive with text like "available for Python 3.5+ only" should be enough.

We dropped Py3.3, I'm converting the doc to use async/await syntax so Py3.5 is preferable version for next release.

@flying-sheep
Copy link
Contributor Author

OK. final thing for now: do i only need to document the iter_* functions? because __aiter__ is documented in the stdlib already.

@asvetlov
Copy link
Member

asvetlov commented Oct 3, 2015

Magic methods should not be documented itself but I would see usage example in class section, e.g. http://aiohttp.readthedocs.org/en/latest/client_reference.html#aiohttp.ClientResponse

@flying-sheep
Copy link
Contributor Author

OK!

@flying-sheep
Copy link
Contributor Author

hmm, i have this AsyncStreamReaderMixin which is used by StreamReader and EmptyStreamReader, but that results in the iter_chunked and iter_any methods not appearing in the docs.

can i force autodoc to treat the methods of AsyncStreamReaderMixin as if they were in StreamReader?

/edit: got it!

@asvetlov
Copy link
Member

asvetlov commented Oct 3, 2015

I believe we should document StreamReader manually, without autodoc.
Eventually we will drop autodoc at all.

@flying-sheep
Copy link
Contributor Author

maybe after this is done?

@asvetlov
Copy link
Member

asvetlov commented Oct 3, 2015

ok, you can use autodoc for now if you want.

@flying-sheep flying-sheep force-pushed the patch-2 branch 2 times, most recently from 29fa81b to 3c8ef3d Compare October 3, 2015 18:08
@flying-sheep
Copy link
Contributor Author

done. this got more complex than i thought due to readexactly leaving this in a state where 0 bytes are left but EOF isn’t actually hit.

tests pass, flake8 also (why the hell does it complain about “unused variable _”? every other tool recognizes that as placeholder)

@@ -0,0 +1,5 @@
"""
Copy link
Member

Choose a reason for hiding this comment

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

Just drop the file.
test_py35 is not a package.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i explained that in the file:

This file allows files in here to be called the same as other test files.

if there is e.g. /tests/test_streams and /tests/test_py35/test_streams, then /tests/test_py35/__init__.py has to exist due to the module cache. try it if you don’t believe me 😄

@asvetlov
Copy link
Member

asvetlov commented Oct 3, 2015

this got more complex than i thought due to readexactly leaving this in a state where 0 bytes are left but EOF isn’t actually hit.

Hmm. Looks like a bug. I think .readexcaltly should raise EofStream after .feed_eof() call.
Anyway .iter_chunked should use .read not .readexactly I believe.

Instead of

while True:
	msg = await dataqueue.read()
	...

do:

async for msg in dataqueue:
	...
@flying-sheep
Copy link
Contributor Author

done!

@flying-sheep
Copy link
Contributor Author

@asvetlov good to merge?

@flying-sheep
Copy link
Contributor Author

@asvetlov ping

@asvetlov asvetlov mentioned this pull request Oct 12, 2015
@asvetlov asvetlov merged commit 7e8b10c into aio-libs:master Oct 18, 2015
@asvetlov
Copy link
Member

Thanks!

@flying-sheep flying-sheep deleted the patch-2 branch October 18, 2015 13:17
@lock
Copy link

lock bot commented Oct 29, 2019

This thread has been automatically locked since there has not been
any recent activity after it was closed. Please open a new issue for
related bugs.

If you feel like there's important points made in this discussion,
please include those exceprts into that new issue.

@lock lock bot added the outdated label Oct 29, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 29, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants