bpo-28369: Enhance transport socket check in add_reader/writer#4365
Conversation
|
@asvetlov Andrew, could you please take a look at this one? |
asvetlov
left a comment
There was a problem hiding this comment.
Thanks for PR.
Please fix notes.
I suggest installing codecov browser extension: https://docs.codecov.io/v4.3.6/docs/browser-extension
You'll see uncovered red lines just in Files changed tab on gihub.
| self.call_exception_handler(context) | ||
|
|
||
| def _ensure_fd_no_transport(self, fd): | ||
| fileno = fd |
There was a problem hiding this comment.
Please not make a new fileno local variable but reuse fd in the rest of function -- like selectors._fileobj_to_fd does.
There was a problem hiding this comment.
fd is used later in this function to format a better error message. This is not a copy/paste of selectors._fileobj_to_fd.
| try: | ||
| fileno = int(fileno.fileno()) | ||
| except (AttributeError, TypeError, ValueError): | ||
| # This code matches `selectors._fileobj_to_fd` function. |
There was a problem hiding this comment.
Please avoid backticks -- without them the comment is still pretty clean.
There was a problem hiding this comment.
Removed, but this is a very personal preference -- for me it was easier to read that comment with backticks/quotes.
| try: | ||
| fd = int(fd.fileno()) | ||
| except (AttributeError, TypeError, ValueError): | ||
| # This code matches `selectors._fileobj_to_fd` function. |
There was a problem hiding this comment.
@1st1 I hate any complex markup in comments, selectors._fileobj_to_fd without backticks is still pretty readable and understandable.
This is just my opinion.
But if you want to keep them -- I can live with it. Very minor thing about a taste.
| fileno = int(fileno.fileno()) | ||
| except (AttributeError, TypeError, ValueError): | ||
| # This code matches `selectors._fileobj_to_fd` function. | ||
| raise ValueError("Invalid file object: " |
There was a problem hiding this comment.
Added a functional test for this.
| def _ensure_fd_no_transport(self, fd): | ||
| if not isinstance(fd, int): | ||
| try: | ||
| fd = int(fd.fileno()) |
| fd = int(fd.fileno()) | ||
| except (AttributeError, TypeError, ValueError): | ||
| # This code matches `selectors._fileobj_to_fd` function. | ||
| raise ValueError("Invalid file object: " |
There was a problem hiding this comment.
This won't be covered as it's test_utils' test loop. There's no point in covering it.
There was a problem hiding this comment.
Maybe let's add # pragma: no cover comment for except clause?
It increases formal coverage level at least (:
|
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 |
|
I have made the requested changes; please review again |
|
Thanks for making the requested changes! @asvetlov: please review the changes made to this pull request. |
asvetlov
left a comment
There was a problem hiding this comment.
Sorry for my censoriousness.
|
Thanks, Andrew! |
https://bugs.python.org/issue28369
Original PR python/asyncio#420 missed the fact that
loop.add_readerandloop.add_writeraccept file-like objects in addition to int FDs.