Prevent StopIteration from being thrown into a Future by Rosuav · Pull Request #322 · python/asyncio (original) (raw)
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
[ Show hidden characters]({{ revealButtonHref }})
Raising StopIteration inside a Future will either result in a spurious
return-like action, or a RuntimeError, depending on the Python version.
Raising StopIteration inside a Future will either result in a spurious return-like action, or a RuntimeError, depending on the Python version.
| @@ -341,6 +341,8 @@ def set_exception(self, exception): |
|---|
| raise InvalidStateError('{}: {!r}'.format(self._state, self)) |
| if isinstance(exception, type): |
| exception = exception() |
| if isinstance(exception, StopIteration): |
| raise TypeError("StopException interacts badly with generators and cannot be raised into a Future") |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I afraid the line is much longer than allowed 79 characters.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you prefer that I shorten the message, or arbitrarily break it into two lines?
| @@ -341,6 +341,9 @@ def set_exception(self, exception): |
|---|
| raise InvalidStateError('{}: {!r}'.format(self._state, self)) |
| if isinstance(exception, type): |
| exception = exception() |
| if type(exception) is StopIteration: |
| raise TypeError("StopException interacts badly with generators " |
| "and cannot be raised into a Future") |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please align the second line
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got any more style issues to hit me with? So far, there's been not a single comment about the actual content of the patch, just fiddling with the styling.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, Guido suggested you the error message text, so I guess it's fine. One comment was about isinstance, and another about bad code style (pep8 and alignment as in the rest of asyncio codebase). I'm not sure why you have this attitude.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry. It's just that the breaking of the line sounded very much like what Raymond Hettinger said about line lengths - the most important reason to stick to 80-character lines isn't readability, nor tooling, it's to make sure you don't get whacked with the PEP 8 hammer.
| @@ -76,6 +76,9 @@ def test_exception(self): |
|---|
| f = asyncio.Future(loop=self.loop) |
| self.assertRaises(asyncio.InvalidStateError, f.exception) |
| # StopIteration cannot be raised into a Future - CPython issue26221 |
| self.assertRaises(TypeError, f.set_exception, StopIteration) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use with self.assertRaisesRegex and match the error message (at least partially).
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Huh, good call on that one. I hadn't noticed that the error text was wrong.
LGTM. I'm not sure the optimization is really worth it but I do see some timing difference -- a simple command-line test says the type() version takes 90 ns while the isinstance() takes 135 ns, both with an argument that's a RuntimeError instance.
1st1 added a commit that referenced this pull request
Patch by Chris Angelico (PR #322)
