Doc: Clarify the return type of Event.wait when timeout is used. #104168
Conversation
|
All commit authors signed the Contributor License Agreement. |
a150707 to
81e3e82
Compare
Doc/library/threading.rst
Outdated
| @@ -989,7 +989,7 @@ method. The :meth:`~Event.wait` method blocks until the flag is true. | |||
|
|
|||
| When the timeout argument is present and not ``None``, it should be a | |||
| floating point number specifying a timeout for the operation in seconds | |||
| (or fractions thereof). | |||
| (or fractions thereof). When the timeout occurs, ``False`` will be returned. | |||
There was a problem hiding this comment.
It is already implicitly documented in the following paragraph. If it is not clear, it would be better to rewrite that paragraph.
There was a problem hiding this comment.
The paragraph you refer to currently states:
This method returns
Trueif and only if the internal flag has been set to
true, either before the wait call or after the wait starts, so it will
always returnTrueexcept if a timeout is given and the operation
times out.
It is certainly not clear enough IMO that False is the return of a timeout (rather than raising, for example). My rationale for putting it where I did was to keep the documentation of the timeout behaviour in a single paragraph, whereas the quoted paragraph, IMO, is less about timeout, and more about a general return value.
If you prefer, I can propose:
This method returns
Trueif and only if the internal flag has been set to
true, either before the wait call or after the wait starts, so it will
always returnTrueexcept if a timeout is given and the operation
times out. If a timeout occurs,Falsewill be returned.
There was a problem hiding this comment.
I think that this paragraph is more about the valid values and meaning of the timeout parameter (None or the time period in seconds, not necessary integer) than about the behavior.
I like your second idea more.
There was a problem hiding this comment.
I've added 2 commits. The first is as discussed. The second strips down some unnecessary and confusing description (removing the except clause from the sentence)
Doc/library/threading.rst
Outdated
| @@ -996,9 +996,8 @@ method. The :meth:`~Event.wait` method blocks until the flag is true. | |||
| (or fractions thereof). | |||
|
|
|||
| This method returns ``True`` if and only if the internal flag has been set to | |||
There was a problem hiding this comment.
"if the internal flag has been set" is vague. In practice, it means that the set() method was called, no?
There was a problem hiding this comment.
I agree that is is vague. I would also like to use that style of language, rather than talking about internal flags tbh.
I've re-written the docstring entirely based on the reviews, but haven't yet used the simpler language you suggested - the only hesitation is that we have to also mention clear() and the fact that set() may have been called before wait() if we want to be 100% precise (I'm not sure it helps with clarity to be so precise though).
There was a problem hiding this comment.
The wait entry immediately follows the set and clear entries, so I think it should be clear enough that those are the functions for manipulating the 'internal flag'.
Doc/library/threading.rst
Outdated
| always return ``True`` except if a timeout is given and the operation | ||
| times out. | ||
| true, either before the wait call or after the wait starts. | ||
| If a timeout is given and the operation times out, ``False`` will be returned. |
There was a problem hiding this comment.
I don't understand what is the expected return value if no timeout is given and set() was not called: the function just doesn't return and block until set() is called?
There was a problem hiding this comment.
Several paragraphs above:
Block until the internal flag is true. If the internal flag is true on
entry, return immediately. Otherwise, block until another thread calls
:meth:`.set` to set the flag to true, or until the optional timeout occurs.
There was a problem hiding this comment.
My rewrite combines paragraphs is a way that I think is clearer in less words. I think this ready to merge now.
There was a problem hiding this comment.
I think the first as well as the third paragraph should be rewritten together and condensed because together they currently jumble together blocking and returning in a confusing and overly wordy manner and because the first sentence is incorrect. (Correcting the incorrectness is part of the reason for needing too many words.) I suggest the following which first gives the two reasons for blocking and then defines the return value as indicating which block reason became false, triggering the return.
Block as long as the internal flag controlled by set and clear is false and the timeout, if any, has not expired. Return the reason for returning: True if returning because the internal flag was initially True or set True thereafter; False if a timeout expires.
Leave the 2nd sentence alone if correct, except that the final parentheses are not needed.
|
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 |
65c9ed2 to
f7f6623
Compare
|
I think this ready to merge now and will do it unless one of you two object or prefer to do so. |
|
LGTM. Thanks for your input 👍 |
|
Thanks @pelson for the PR, and @serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 3.11, 3.12. |
…onGH-104168) (cherry picked from commit 37f5d06) Co-authored-by: Phil Elson <pelson.pub@gmail.com>
|
GH-115938 is a backport of this pull request to the 3.12 branch. |
…onGH-104168) (cherry picked from commit 37f5d06) Co-authored-by: Phil Elson <pelson.pub@gmail.com>
|
GH-115939 is a backport of this pull request to the 3.11 branch. |
|
Thank you for your contribution @pelson and for your review @terryjreedy. |
Explicitly clarify the return type of
Event.wait(timeout)in the documentation.Without this, it takes some logic to figure out what will occur (or whether perhaps a
TimeoutErrormight be raised, for example). It would be easy to miss this, and not check the return type.📚 Documentation preview 📚: https://cpython-previews--104168.org.readthedocs.build/