Skip to content

Conversation

ZackerySpytz
Copy link
Contributor

@ZackerySpytz ZackerySpytz commented May 20, 2020

Copy link
Contributor

@YoSTEALTH YoSTEALTH left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doc/library/exceptions.rst needs to be updated as well

Copy link
Contributor

@YoSTEALTH YoSTEALTH left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good.

@csabella csabella requested a review from ericvsmith May 23, 2020 16:34
Copy link
Member

@ericvsmith ericvsmith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to add a test for this? I realize it might be difficult, so it's not a show stopper, but it would be good to test newly added code. There are a few ETIMEDOUT tests, so maybe they could be used as a guide?

Updated blurb to reflect that it's an OSError that's being mapped to a TimeoutError.
@@ -65,7 +65,7 @@ def test_select_error(self):
+-- NotADirectoryError ENOTDIR
+-- PermissionError EACCES, EPERM
+-- ProcessLookupError ESRCH
+-- TimeoutError ETIMEDOUT
+-- TimeoutError ETIME, ETIMEDOUT
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this test is as good as we're going to get.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be honest, I'm having second thoughts w.r.t. this PR (especially after the discussion
on python-dev). I don't know a lot about ETIME. I'm not sure if I should have submitted this PR.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ZackerySpytz No need to worry, its only being added to future version of python. With more and more async style coding being adapted ETIME will be handy to have as TimeoutError. It should have been adapted a long time ago but i suppose no one wanted to bother submitting a PR for it.

@erlend-aasland erlend-aasland changed the title bpo-39673: Map errno==ETIME to TimeoutError gh-83854: Map errno==ETIME to TimeoutError Jan 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants