bpo-38314: Add is_reading() method to asyncio _UnixReadPipeTransport #17755
Conversation
Unix read pipe is missing method available for other read transports within the aynscio protocol, which checks that the transport read is not paused or the transport is not closing.
Co-Authored-By: Kyle Stanley <aeros167@gmail.com>
aeros
left a comment
aeros
left a comment
There was a problem hiding this comment.
Hey @CallumQuick, sorry for the delay on this PR. I think I can spend some time during the sprint reviewing it though and hopefully getting it merged (assuming you're also currently available and interested).
The implementation is perfectly fine. The test logic is also reasonable at a glance, but I do think that we should definitely group them in test_unix_events.py under something like the existing UnixReadPipeTransportTests (or its own class if needed) rather than in a separate test file. IMO, it doesn't make sense to separate out/add a new test file just for _UnixReadPipeTransport.is_reading() when the other existing unix pipe tests are in test_unix_events.
Is there a particular reason as to why you went with the current approach of having a separate test file?
|
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 |
@aeros It looks like this was in reponse to #17042 (comment)? |
|
Hi @aeros, yes this was in response to #17042 (comment), suggesting that splitting off the new style of functional tests would be preferable. I also added a simple mock-based test case to |
Oh sorry about that, I think I saw that comment from Andrew ~a year ago when I had made a suggestion for the news entry, but it must have slipped my mind when was looking over this PR. I'll take another pass over the tests. |
| """ | ||
| Verify that transports on Unix can facilitate reading and have access to | ||
| all state methods: verify using class internals | ||
| """ |
There was a problem hiding this comment.
Style nit: As per PEP 257, within the standard library, we use the following format for all docstrings:
"""..."""
or
"""...
...
"""
Rather than:
"""
...
"""
I was originally hesitant to point it out for a new test file that doesn't have an existing convention, but during this year's virtual core dev sprint (Oct. 19 - 23), I asked in the general chat and Guido commented that he was "unwaveringly in favor" of using the PEP 257 docstring conventions.
6 participants
Unix read pipe (from
Lib:asyncio.unix_events) is missingis_reading()method available for other read transports within the asyncio protocol, which checks that the transport read is notpaused or the transport is not closing.
This behaviour is documented in
Doc/library/asyncio-protocol.rst.Additional tests for the
Lib:asyncio.unix_eventsmodule which check against the state of the pipe and the behaviour ofis_reading(). New testing harness for functional testing of the UNIX pipe connection and associated methods, with an initial test for the newis_reading()implementation.This is a fresh rebase PR, based on changes made in the original PR (#17042).
https://bugs.python.org/issue38314