Prevent StopIteration from being thrown into a Future #322
Conversation
Raising StopIteration inside a Future will either result in a spurious return-like action, or a RuntimeError, depending on the Python version.
asyncio/futures.py
Outdated
| if isinstance(exception, type): | ||
| exception = exception() | ||
| if isinstance(exception, StopIteration): | ||
| raise TypeError("StopException interacts badly with generators and cannot be raised into a Future") |
There was a problem hiding this comment.
I afraid the line is much longer than allowed 79 characters.
There was a problem hiding this comment.
Would you prefer that I shorten the message, or arbitrarily break it into two lines?
asyncio/futures.py
Outdated
| exception = exception() | ||
| if type(exception) is StopIteration: | ||
| raise TypeError("StopException interacts badly with generators " | ||
| "and cannot be raised into a Future") |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
tests/test_futures.py
Outdated
| self.assertRaises(asyncio.InvalidStateError, f.exception) | ||
|
|
||
| # StopIteration cannot be raised into a Future - CPython issue26221 | ||
| self.assertRaises(TypeError, f.set_exception, StopIteration) |
There was a problem hiding this comment.
Please use with self.assertRaisesRegex and match the error message (at least partially).
There was a problem hiding this comment.
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. |
|
Merged. Thanks, Chris! |
Raising StopIteration inside a Future will either result in a spurious
return-like action, or a RuntimeError, depending on the Python version.