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

Update asyncio.ensure_future() documentation #15347

Merged
merged 4 commits into from
Aug 21, 2019
Merged

Update asyncio.ensure_future() documentation #15347

merged 4 commits into from
Aug 21, 2019

Conversation

akindofyoga
Copy link
Contributor

Added back mention that ensure_future actually scheduled obj. This documentation just mentions what ensure_future returns, so I did not realize that ensure_future also schedules obj.

Added back mention that ensure_future actually scheduled obj. This documentation just mentions what ensure_future returns, so I did not realize that ensure_future also schedules obj.
@the-knights-who-say-ni
Copy link

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA).

Unfortunately we couldn't find an account corresponding to your GitHub username on bugs.python.org (b.p.o) to verify you have signed the CLA (this might be simply due to a missing "GitHub Name" entry in your b.p.o account settings). This is necessary for legal reasons before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

You can check yourself to see if the CLA has been received.

Thanks again for your contribution, we look forward to reviewing it!

Copy link
Contributor

@aeros aeros left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @rogerthat94 and welcome!

I have a few recommendations for you:

  1. Create an account on https://bugs.python.org/ and sign the CLA. Usually this takes around 24 hours and sometimes the status must be updated manually by checking the status for GitHub.

  2. Adjust the PR title to Update asyncio.ensure_future() documentation. The title you are using is definitely along the right lines, but it will make it easier for core developers knowledgeable with that specific function to find this PR.

Let me know if you have any questions and I'll do my best to answer them. (:

@@ -28,6 +28,9 @@ Future Functions

.. function:: ensure_future(obj, \*, loop=None)

Schedule the execution of a :ref:`coroutine object <coroutine>`: wrap it in
Copy link
Contributor

Choose a reason for hiding this comment

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

The use of the :term: Sphinx role links to Python's glossary directly. :ref: would not be incorrect in this case, but :term: is more specific. The glossary would be probably be the best place to direct users to if they are unfamiliar with coroutines.

Additionally, the extra : character after the Sphinx role isn't needed.

Suggested change
Schedule the execution of a :ref:`coroutine object <coroutine>`: wrap it in
Schedule the execution of a :term:`coroutine object <coroutine>` and wrap it in

After adding this change, it may be necessary to split this into an additional line. reST specifies that the maximum line width should be 80 characters, this change would place the total for this line at 82.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! I have signed the CLA, updated the title, and made the changes that you requested.

@akindofyoga akindofyoga changed the title Update asyncio-future documentation Update asyncio.ensure_future() documentation Aug 21, 2019
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.

I don't understand why this change is needed. The description you added isn't correct -- the function is quite complex and can do multiple things based on the type of its argument. The documentation already describes this all in full in a succinct form.

@bedevere-bot
Copy link

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@1st1
Copy link
Member

1st1 commented Aug 21, 2019

@rogerthat94 If you want we can do this the other way:

   * a :class:`Task` object wrapping *obj*, if *obj* is a
     coroutine (:func:`iscoroutine` is used for the test);
     in this case the coroutine will be scheduled by
    ``ensure_future()``.
     

@akindofyoga
Copy link
Contributor Author

The suggestion in your second comment looks fine to me.

The current documentation does not mention that asyncio.ensure_future() will schedule a coroutine. It just says what asyncio.ensure_future() returns. I think highlighting the fact that asyncio.ensure_future() can schedule a coroutine in addition to returning something is important.

@akindofyoga
Copy link
Contributor Author

I have made the requested changes; please review again

@bedevere-bot
Copy link

Thanks for making the requested changes!

@1st1: please review the changes made to this pull request.

@asvetlov
Copy link
Contributor

asvetlov commented Aug 21, 2019

Is there a thing like scheduled coroutine at all?
There is scheduled callback.
A task starts running just after creation, so there is no created-but-not-started task.

That's how asyncio works, nothing special for ensure_future(): is you have a task the object is either running or finished (successfully or by exception).

The scheduled async function looks incorrect.

Co-Authored-By: Yury Selivanov <yury@magic.io>
@akindofyoga
Copy link
Contributor Author

The old documentation for ensure_future says "Schedule the execution of a coroutine object: wrap it in a future. Return a Task object."

@1st1
Copy link
Member

1st1 commented Aug 21, 2019

I mean I say "schedule coroutine" all the time :) Maybe we do need a better terminology, but as it currently stands I guess this one is fine. :)

@miss-islington
Copy link
Contributor

Thanks @rogerthat94 for the PR, and @1st1 for merging it 🌮🎉.. I'm working now to backport this PR to: 3.7, 3.8.
🐍🍒⛏🤖

@miss-islington
Copy link
Contributor

I'm having trouble backporting to 3.8. Reason: 'Error 110 while writing to socket. Connection timed out.'. Please retry by removing and re-adding the needs backport to 3.8 label.

@bedevere-bot
Copy link

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

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Aug 21, 2019
Added back mention that ensure_future actually scheduled obj. This documentation just mentions what ensure_future returns, so I did not realize that ensure_future also schedules obj.
(cherry picked from commit 092911d)

Co-authored-by: Roger Iyengar <ri@rogeriyengar.com>
@1st1
Copy link
Member

1st1 commented Aug 21, 2019

Thank you, @rogerthat94!

@akindofyoga akindofyoga deleted the fix-future-doc branch August 21, 2019 16:01
@aeros
Copy link
Contributor

aeros commented Aug 21, 2019

Thanks for the PR @rogerthat94 and thanks for the review @1st1 and @asvetlov!

@aeros
Copy link
Contributor

aeros commented Aug 21, 2019

@1st1

Maybe we do need a better terminology, but as it currently stands I guess this one is fine. :)

If we do come up with a better word for it, be sure to add it to the glossary! This will help to ensure that readers have a clear understanding of what is being referred to. There's several async terms on there already.

1st1 pushed a commit that referenced this pull request Aug 21, 2019
Added back mention that ensure_future actually scheduled obj. This documentation just mentions what ensure_future returns, so I did not realize that ensure_future also schedules obj.
(cherry picked from commit 092911d)

Co-authored-by: Roger Iyengar <ri@rogeriyengar.com>
@miss-islington
Copy link
Contributor

Thanks @rogerthat94 for the PR, and @1st1 for merging it 🌮🎉.. I'm working now to backport this PR to: 3.8.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Aug 21, 2019
Added back mention that ensure_future actually scheduled obj. This documentation just mentions what ensure_future returns, so I did not realize that ensure_future also schedules obj.
(cherry picked from commit 092911d)

Co-authored-by: Roger Iyengar <ri@rogeriyengar.com>
@bedevere-bot
Copy link

GH-15364 is a backport of this pull request to the 3.8 branch.

1st1 pushed a commit that referenced this pull request Aug 21, 2019
Added back mention that ensure_future actually scheduled obj. This documentation just mentions what ensure_future returns, so I did not realize that ensure_future also schedules obj.
(cherry picked from commit 092911d)

Co-authored-by: Roger Iyengar <ri@rogeriyengar.com>
lisroach pushed a commit to lisroach/cpython that referenced this pull request Sep 10, 2019
Added back mention that ensure_future actually scheduled obj. This documentation just mentions what ensure_future returns, so I did not realize that ensure_future also schedules obj.
DinoV pushed a commit to DinoV/cpython that referenced this pull request Jan 14, 2020
Added back mention that ensure_future actually scheduled obj. This documentation just mentions what ensure_future returns, so I did not realize that ensure_future also schedules obj.
websurfer5 pushed a commit to websurfer5/cpython that referenced this pull request Jul 20, 2020
Added back mention that ensure_future actually scheduled obj. This documentation just mentions what ensure_future returns, so I did not realize that ensure_future also schedules obj.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants