Clarify documentation around close, flush, and isopen
#58020
+48
−12
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The previous documentation was unclear about what "closing" something actually meant, what you could expect from a closed resource, and whether it was usable in generic code. It also mixed up closing IO object, timers and events, where the semantics are different. In this commit: * The docstring of `close` and `flush` has been expanded. * `close` has been given a generic implementation * The documentation details specific to `IOStream`, `Timer` and `Asyncevent` has been moved to specific docstrings, such that the *generic* behaviour of these functions aren't mixed up with the specifics of e.g. `IOStream`.
jakobnissen
added
io
fingolfin
reviewed
May 8, 2025
| @@ -9,7 +9,8 @@ Create a async condition that wakes up tasks waiting for it | |||
| (by calling [`wait`](@ref) on the object) | |||
| when notified from C by a call to `uv_async_send`. | |||
| Waiting tasks are woken with an error when the object is closed (by [`close`](@ref)). | |||
| Use [`isopen`](@ref) to check whether it is still active. | |||
| Use [`isopen`](@ref) to check whether it is still active. A closed condition will | |||
There was a problem hiding this comment.
Maybe this would be better? Yes, the function is called isopen but the text says "active", not "open?
Suggested change
| Use [`isopen`](@ref) to check whether it is still active. A closed condition will | |
| Use [`isopen`](@ref) to check whether it is still active. An inactive condition will |
There was a problem hiding this comment.
OK I see elsewhere the text already talks about "closed". I find this use of two different terms meaning the same thing adds a layer of potential confusion. So perhaps like this:
Suggested change
| Use [`isopen`](@ref) to check whether it is still active. A closed condition will | |
| Use [`isopen`](@ref) to check whether it is still active. A closed condition is inactive and will |
fingolfin
reviewed
May 8, 2025
| @@ -74,7 +75,8 @@ Create a timer that wakes up tasks waiting for it (by calling [`wait`](@ref) on | |||
| Waiting tasks are woken after an initial delay of at least `delay` seconds, and then repeating after | |||
| at least `interval` seconds again elapse. If `interval` is equal to `0`, the timer is only triggered | |||
| once. When the timer is closed (by [`close`](@ref)) waiting tasks are woken with an error. Use | |||
| [`isopen`](@ref) to check whether a timer is still active. Use `t.timeout` and `t.interval` to read | |||
| [`isopen`](@ref) to check whether a timer is still active. A closed timer will not fire. | |||
There was a problem hiding this comment.
Suggested change
| [`isopen`](@ref) to check whether a timer is still active. A closed timer will not fire. | |
| [`isopen`](@ref) to check whether a timer is still active. An inactive timer will not fire. |
fingolfin
reviewed
May 8, 2025
| """ | ||
| close(t::Union{Timer, AsyncCondition}) | ||
|
|
||
| Close an object `t`. Once a timer or condition is closed, it will not produce |
There was a problem hiding this comment.
Continuing from above, how about
Suggested change
| Close an object `t`. Once a timer or condition is closed, it will not produce | |
| Close an object `t` and thus mark it as inactive. Once a timer or condition is inactive, it will not produce |
fingolfin
reviewed
May 8, 2025
| However, since a closed stream may still have data to read in its buffer, | ||
| use [`eof`](@ref) to check for the ability to read data. | ||
| Use the `FileWatching` package to be notified when a stream might be writable or readable. | ||
| Determine whether an object, such as an IO or timer, is not yet closed. |
There was a problem hiding this comment.
Suggested change
| Determine whether an object, such as an IO or timer, is not yet closed. | |
| Determine whether an object, such as an IO or timer, is still open and hence active. |
fingolfin
reviewed
May 8, 2025
|
|
||
| Close an I/O stream. Performs a [`flush`](@ref) first. | ||
| This function is generically defined to only `flush` the io. That allows | ||
| wrapping IOs to close its underlying IO. |
There was a problem hiding this comment.
Suggested change
| wrapping IOs to close its underlying IO. | |
| wrapping IOs to close their underlying IOs. |
fingolfin
reviewed
May 8, 2025
Comment on lines
+81
to
+84
| function close(io::IO) | ||
| flush(io) | ||
| nothing | ||
| end |
There was a problem hiding this comment.
Isn't this a change in behavior and thus goes beyond what the title of this PR suggests? I feel a bit uncomfortable with this -- I'd immediately agree to merge the docstring changes, but I don't feel qualified to judge whether this change is OK.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The previous documentation was unclear about what "closing" something actually meant, what you could expect from a closed resource, and whether it was usable in generic code.
It also mixed up closing IO object, timers and events, where the semantics are different.
In this commit:
closeandflushhas been expanded.closehas been given a generic implementationIOStream,TimerandAsynceventhas been moved to specific docstrings, such that the generic behaviour of these functions aren't mixed up with the specifics of e.g.IOStream.Closes #57944