-
-
Notifications
You must be signed in to change notification settings - Fork 31k
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
__aiter__ should return async iterator instead of awaitable #71430
Comments
There is a small flaw in PEP-492 design -- __aiter__ should not return an awaitable object that resolves to an asynchronous iterator. It should return an asynchronous iterator directly. Let me explain this by showing some examples. I've discovered this while working on a new asynchronous generators PEP. Let's pretend that we have them already: if we have a 'yield' expression in an 'async def' function, the function becomes an "asynchronous generator function": async def foo():
await bar()
yield 1
await baz()
yield 2 foo -- is an
|
While I agree this needs to be fixed, one key piece of documentation needed will be to cover how to write an __aiter__ method that does the right thing on both 3.5.1 and 3.5.2+ (and also avoids the deprecation warning in the latter case). (I've also added Larry to the cc list as release manager) |
Would it be easier to handle for everyone if this did not vary between |
Since the old behaviour is only deprecated with Yury's changes, rather than disallowed entirely, I think it makes sense to provide a 3.5.x version that also supports the new behaviour. In addition to my docs comment above, the other thing I noticed in reviewing the patch is that the proposed tests don't currently check that we *don't* emit a deprecation warning for the newly permitted forward compatible cases that return an asynchronous iterator (rather than an awaitable) directly from __aiter__. There are definitely some tests that exercise that path, so we could rely on the general principle of "the test suite shouldn't emit deprecation warnings", but we could also add some tests that specifically check that no warning is emitted in that case. |
As RM my default position is naturally "don't change behavior in point releases". I'm willing to be overruled by the BDFL, less so by anybody else. |
Oh, this is tough. How about we do this:
It's a rather long & conservative process, but it will be |
It's a rather long & conservative process, but it will be easier for people to migrate their code. |
Unless Nick disagrees (or unless we can't figure out how to implement it) I |
Okay. I'm hoping to not delay 3.5.2 RC1, and the schedule calls for me to tag the release Saturday afternoon. Can you guys get this solid and checked in before then? |
Will do my best. I'll update the patch soon with some code comments and docs. |
Updated patch (fix_aiter2.patch) |
Yury's proposal sounds good to me - I'll have time to do a proper review tomorrow (at a quick glance, my one suggestion is to move the if statement outside the example decorator, so the decorator is defined differently based on the Python version, rather than checking the version when called) |
Nick, Please see the updated patch. Do you think it's ready to be merged? I want buildbots to have some time with it before RC. |
+1 from me - my only comments were on the docs updates and one of the explanatory comments in the code. |
New changeset 93ad47d63b87 by Yury Selivanov in branch '3.5': New changeset 9ff95c30a38e by Yury Selivanov in branch 'default': |
Thanks a lot, Nick! I've merged the patch. |
I've also updated PEP-492: https://hg.python.org/peps/rev/fef4b9969b9d Please feel free to post to this issue if you think that I should have covered it differently or in more detail. |
Test suite emits a new warning, and fails under python -Werror: ====================================================================== Traceback (most recent call last):
File "/media/disk/home/proj/python/cpython/Lib/test/test_asyncio/test_pep492.py", line 89, in test_readline
data = self.loop.run_until_complete(reader())
File "/media/disk/home/proj/python/cpython/Lib/asyncio/base_events.py", line 387, in run_until_complete
return future.result()
File "/media/disk/home/proj/python/cpython/Lib/asyncio/futures.py", line 274, in result
raise self._exception
File "/media/disk/home/proj/python/cpython/Lib/asyncio/tasks.py", line 239, in _step
result = coro.send(None)
File "/media/disk/home/proj/python/cpython/Lib/test/test_asyncio/test_pep492.py", line 85, in reader
async for line in stream:
PendingDeprecationWarning: 'StreamReader' implements legacy __aiter__ protocol; __aiter__ should return an asynchronous iterator, not awaitable |
This is because sys.version_info is 3.5.1 (not 3.5.2 yet) in the "3.5" branch. Once 3.5.2 RC is tagged the warning will disappear. https://github.com/python/cpython/blob/master/Lib/asyncio/streams.py#L693 |
I didn’t realize, sorry for the noise |
Actually thanks for reporting this, Martin. I didn't realize that sys.version_info was 3.5.1 in 3.5 branch. |
New changeset a0d50aad7b02 by Yury Selivanov in branch '3.6': New changeset 9550f0d22d27 by Yury Selivanov in branch 'default': |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: