-
-
Notifications
You must be signed in to change notification settings - Fork 32k
bpo-38260: Add Docs on asyncio.run #16337
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
Conversation
Add docs about return and raise exception on asyncio.run
Doc/library/asyncio-task.rst
Outdated
@@ -225,6 +225,19 @@ Running an asyncio Program | |||
the end. It should be used as a main entry point for asyncio | |||
programs, and should ideally only be called once. | |||
|
|||
Return the `base_events.run_until_complete()` that this return the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we don't need to mention a future and run_until_complete()
here.
asyncio.run()
returns a result of coro execution, that's it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @asvetlov Thanks for the review. I make the changes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left some comments for you.
Please take a look
Doc/library/asyncio-task.rst
Outdated
@@ -225,6 +225,18 @@ Running an asyncio Program | |||
the end. It should be used as a main entry point for asyncio | |||
programs, and should ideally only be called once. | |||
|
|||
Return a result of *coro* execution, or raise a RuntimeError if |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RuntimeError should be
:exc:`RuntimeError`
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks I change it
Doc/library/asyncio-task.rst
Outdated
@@ -225,6 +225,18 @@ Running an asyncio Program | |||
the end. It should be used as a main entry point for asyncio | |||
programs, and should ideally only be called once. | |||
|
|||
Return a result of *coro* execution, or raise a RuntimeError if | |||
``asyncio.run()`` is called from a running event loop, or a | |||
ValueError if *coro* is not a courutine. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This also should be
:exc:`ValueError`
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change it too. thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
@eamanu: Status check is done, and it's a success ✅ . |
Thanks @eamanu for the PR 🌮🎉.. I'm working now to backport this PR to: 3.7, 3.8. |
GH-16379 is a backport of this pull request to the 3.8 branch. |
GH-16380 is a backport of this pull request to the 3.7 branch. |
Add docs about return and raise exception on asyncio.run https://bugs.python.org/issue38260 Automerge-Triggered-By: @asvetlov (cherry picked from commit 17deb16) Co-authored-by: Emmanuel Arias <emmanuelarias30@gmail.com>
Add docs about return and raise exception on asyncio.run https://bugs.python.org/issue38260 Automerge-Triggered-By: @asvetlov (cherry picked from commit 17deb16) Co-authored-by: Emmanuel Arias <emmanuelarias30@gmail.com>
Add docs about return and raise exception on asyncio.run https://bugs.python.org/issue38260 Automerge-Triggered-By: @asvetlov (cherry picked from commit 17deb16) Co-authored-by: Emmanuel Arias <emmanuelarias30@gmail.com>
|
@@ -225,6 +225,18 @@ Running an asyncio Program | |||
the end. It should be used as a main entry point for asyncio | |||
programs, and should ideally only be called once. | |||
|
|||
Return a result of *coro* execution, or raise a :exc:`RuntimeError` | |||
if ``asyncio.run()`` is called from a running event loop, or a | |||
:exc:`ValueError` if *coro* is not a courutine. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the adding the additional information @eamanu, but I noticed a couple of issues with the section added.
The more apparent one was a spelling typo, "courutine" should be "coroutine" (misspelled here and in the docstring). Also, it's helpful to link to the glossary using:
:term:`coroutine`
Also, while this section is helpful (particularly with specifying the RuntimeError
and ValueError
scenarios), I think it could be phrased significantly better. I would suggest the following changes:
Within the new event loop, *coro* is executed, returning the result. If
``asyncio.run()`` is called within a running event loop, a :exc:`RuntimeError`
is raised. If *coro* is not a coroutine, a :exc:`ValueError` is raised.
Personally, I think it's especially important to phrase this section of the documentation as well as possible, since it's likely one of the most read sections for the entire asyncio module.
Let me know what you think @asvetlov and @1st1. If you approve of the suggestions, I'll open a new PR that makes the changes I mentioned above in the docs for ascynio.run()
and in the docstring as well (since this one was already merged and backported).
Grammar tip: When chaining together multiple instances of "or", each of the items should be separated by a comma, but only the last one is supposed to have the "or". For example: "item1, item2, or item3" instead of "item1, or item2, or item3". The section added to the documentation in this PR used the latter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I follow "Within the new event loop, coro is executed, returning the result." -- I can't parse it. I'd keep the original one "Return the result of coro execution."
IMO it's not necessary to document RuntimeError and ValueError; "This function cannot be called when another asyncio event loop is running in the same thread." already covers that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also in case it wasn't obvious, the test_venv failure is completely unrelated to the PR. This failure has been happening intermittently for the buildbots recently. I believe a similar failure with test_venv recently happened in a PR from @corona10 and he created a bpo issue for it. |
PR with improvement is welcome! @aeros167 your proposals make sense |
Sounds good! I'll open one soon. Edit: The changes are applied in #16403. |
Add docs about return and raise exception on asyncio.run https://bugs.python.org/issue38260 Automerge-Triggered-By: @asvetlov
Add docs about return and raise exception on asyncio.run
https://bugs.python.org/issue38260
Automerge-Triggered-By: @asvetlov