Skip to content

bpo-36921: Deprecate @coroutine for sake of async def #13346

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

Merged
merged 15 commits into from
May 16, 2019

Conversation

asvetlov
Copy link
Contributor

@asvetlov asvetlov commented May 15, 2019

The second attempt. Now deprecate @coroutine only, keep yield from fut as is.

https://bugs.python.org/issue36921

@asyncio.coroutine
def __anext__(self) -> T_a:
data = yield from self.value
async def __anext__(self) -> T_a:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The same functionality but without @coroutine

Copy link
Member

@ilevkivskyi ilevkivskyi left a comment

Choose a reason for hiding this comment

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

The typing part LGTM.

@gvanrossum gvanrossum removed their request for review May 15, 2019 19:45
Co-Authored-By: Matthias Bussonnier <[email protected]>
@Carreau
Copy link
Contributor

Carreau commented May 15, 2019

want to update asyncio-task.rst to use the .. deprecated-removed:: 3.8 3.10 directive ?

@Carreau
Copy link
Contributor

Carreau commented May 15, 2019

Also side-question (and maybe this is better suited for bpo , should asyncio.iscoroutine and asyncio.iscoroutinefunction be deprecated at the same time ?

Because presumably once @coroutine is removed, the two other have quasy no chance of encountering a coroutine that would not be properly detected by inspect.iscoroutine.

I'm thinking they are still useful from inter-library compatibility and across python version; so maybe deprecate as well but remove even later ?

Copy link
Member

@1st1 1st1 left a comment

Choose a reason for hiding this comment

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

Overall LGTM. Please fix the few nits I commented on.

@1st1
Copy link
Member

1st1 commented May 15, 2019

Also side-question (and maybe this is better suited for bpo , should asyncio.iscoroutine and asyncio.iscoroutinefunction be deprecated at the same time ?

We can deprecate them when we remove @asyncio.coroutine (in 3.9?)

asvetlov and others added 2 commits May 16, 2019 09:20
@asvetlov
Copy link
Contributor Author

@caavery the funny thing that @coroutine is already deprecated in docs. Now we have the support of this statement explicitly in code.
Removal is scheduled for 3.10, that's fine.
I agree with @1st1 that asyncio.iscoroutine and asyncio.iscoroutinefunction should be deprecated later.
As the maintainer of several asyncio based libraries, I want a smooth transition path.
My libraries don't use @coroutine already but sometimes accept async callbacks and calls asyncio checkers for them. I want to support user-written old-style coros in my libraries as long as possible (but discourage them).
Basically the same as asyncio itself does.
Deprecation checkers after removal the root functionality absolutely makes sense to me. We can consider deprecation in 3.9 if you want but I pretty sure we shouldn't do it in 3.8.

This decorator should not be used for :keyword:`async def`
coroutines.

.. deprecated-removed:: 3.8 3.10
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Replace free-form deprecation text with more standardized one.

@asvetlov
Copy link
Contributor Author

asvetlov commented May 16, 2019

The last call for a review.
I have a plan to merge the PR in a day.

@1st1
Copy link
Member

1st1 commented May 16, 2019

The last call for a review.

I think it's ready.

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.

7 participants