gh-85747: Active voice & suggested edits, 'running/stopping loop' & 'callbacks' subsections of asyncio-eventloop.rst #100270
Conversation
Loop run/stop section, and callbacks section.
bskinn
requested review from
1st1,
asvetlov,
gvanrossum and
kumaraditya303
as code owners
|
| Name | Link |
|---|---|
| 7c3fb58 | |
| https://app.netlify.com/sites/python-cpython-preview/deploys/639b3f604a5a2b000a14de44 | |
| https://deploy-preview-100270--python-cpython-preview.netlify.app | |
Toggle QR Code...Use your smartphone camera to open QR code link. |
To edit notification comments on pull requests, go to your Netlify site settings.
| Note that there is no need to call this function when | ||
| :func:`asyncio.run` is used. | ||
| Note that there is no need to call this function when :func:`asyncio.run` is | ||
| used, because that high-level function handles default executor shutdown |
There was a problem hiding this comment.
One spot where I think more explanation would be beneficial: explaining why "there is no need to call this function when asyncio.run() is used." Please correct my guess at the "why", if needed.
| Callbacks are called in the order in which they are registered. | ||
| Each callback will be called exactly once. | ||
| Callbacks are called in the order in which they are registered. Each callback | ||
| will be called exactly once, even if it is registered multiple times. |
There was a problem hiding this comment.
It wasn't clear to me why it was important to note that 'each callback will be called exactly once' -- if I register a callback once, I would expect it only to be run once.
My guess here is that a callback could be registered multiple times before execution loops back around to it, and in this case it's still called only once?
Alternatively -- I could see a design decision where a callback, once registered, is called repeatedly as the event loop cycles around, up until the point where it is de-registered by some mechanism... but that would be a more surprising design to me.
Regardless: I suggest more clarity here around this "will be called exactly once".
| the threads in the :class:`ThreadPoolExecutor`. After calling this method, | ||
| :meth:`loop.run_in_executor` will raise a :exc:`RuntimeError` if called |
There was a problem hiding this comment.
| the threads in the :class:`ThreadPoolExecutor`. After calling this method, | |
| :meth:`loop.run_in_executor` will raise a :exc:`RuntimeError` if called | |
| the threads in the :class:`~concurrent.futures.ThreadPoolExecutor`. | |
| After calling this method, | |
| :meth:`loop.run_in_executor` will raise a :exc:`RuntimeError` if called |
Fix broken reference to ThreadPoolExecutor (while still displaying it as such)
| The *timeout* parameter specifies the amount of time this method gives the | ||
| executor to finish joining. The default value is ``None``, which means the |
There was a problem hiding this comment.
| The *timeout* parameter specifies the amount of time this method gives the | |
| executor to finish joining. The default value is ``None``, which means the | |
| *timeout* specifies the duration in seconds (as a :class:`float`) | |
| this method gives the executor to finish joining. | |
| With the default, ``None``, the |
- Add key clarifying detail that timeout is in seconds as a
float, which is never stated, and I had to dig into the code to be sure. This leaves the reader to guess—Float seconds? Integer microseconds?datetimeobjects?timestructs? Per Diataxis, there should be no doubt or ambiguity in reference. - Be more concise, direct and to the point, as appropriate for reference docs
|
|
||
| If the timeout duration is reached, a warning is emitted and executor is | ||
| terminated without waiting for its threads to finish joining. | ||
| If the timeout duration is reached, this method emits a warning and |
There was a problem hiding this comment.
| If the timeout duration is reached, this method emits a warning and | |
| If the *timeout* is reached, this method emits a :exc:`RuntimeWarning` and |
Explicitly specify and cross-reference what warning is raised (extracted from the code), if users wanted to except it, filter it, check for it, log it, etc).
Also, format the parameter name accordingly
| Note that there is no need to call this function when :func:`asyncio.run` is | ||
| used, because that high-level function handles default executor shutdown | ||
| automatically. |
There was a problem hiding this comment.
| Note that there is no need to call this function when :func:`asyncio.run` is | |
| used, because that high-level function handles default executor shutdown | |
| automatically. | |
| .. note:: There is no need to call this method when :func:`asyncio.run` is | |
| used, because that high-level function handles default executor shutdown | |
| automatically. |
- Make this an actual note
- This is a method rather than a function, and calling it thus avoids repetition and potential confusion
| A thread-safe variant of :meth:`call_soon`. This method *must* be used to | ||
| schedule callbacks *from another thread*. |
There was a problem hiding this comment.
It was not totally clear to me whether this means to say that:
- This method must be used If the callback will be run in another thread than this method is called in,, which is what I think it is trying to say, or
- This method must be used from another thread to schedule callbacks, as I first read it, or
- Something else?
Here's a suggestion trying to clarify the first option:
| A thread-safe variant of :meth:`call_soon`. This method *must* be used to | |
| schedule callbacks *from another thread*. | |
| A thread-safe variant of :meth:`call_soon`. | |
| This method **must** be used instead of :meth:`!call_soon` to | |
| schedule callbacks if they are to be called from another thread. |
Also, used bold instead of italics to avoid confusion with the standard formatting of parameter names.
Another docs revision PR of
asyncio-eventloop.rstper #85747.There are a couple of spots here where I think more explanation than was already present will be useful. I've attempted explanations of my own, but they may be incorrect. I'll mark these with review comments shortly.
I didn't change much of the passive voice related to callback interactions, within the description text of methods that work with callbacks. All the attempts I made to cast those descriptions in active voice seemed to degrade readability.