Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
|
@svelankar, thanks for your PR! By analyzing the history of the files in this pull request, we identified @ncoghlan, @berkerpeksag and @Yhg1s to be potential reviewers. |
| raise | ||
| if sys.exc_info()[1] is value: | ||
| return False | ||
| raise | ||
| else: |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
@ncoghlan suggested to drop this else: clause.
| @@ -152,6 +152,28 @@ def woohoo(): | |||
| else: | |||
| self.fail('StopIteration was suppressed') | |||
|
|
|||
| def test_contextmanager_do_not_unchain_non_stopiteration_exceptions(self): | |||
| code = """\ | |||
| from __future__ import generator_stop | |||
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Is this needed in 3.7?
| try: | ||
| yield | ||
| except Exception as exc: | ||
| raise RuntimeError from exc |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Add some argument to the raised RuntimeError for checking that the correct RuntimeError is caught.
| with test_issue29692(): | ||
| raise ZeroDivisionError | ||
| except Exception as ex: | ||
| self.assertIs(type(ex), RuntimeError) |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Check that this is a RuntimeError raised by test_issue29692, not some other RuntimeError. Check it's __cause__ (it should be ZeroDivisionError, right?).
|
|
||
| try: | ||
| with test_issue29692(): | ||
| raise ZeroDivisionError |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
What if raise StopIteration? I think this case should be tested too.
|
Please add an entry in |
ecf5deb
to
9538527
Compare
|
Implemented all changes, please review, thanks. |
| raise RuntimeError('issue29692:Chained') from exc | ||
| """ | ||
| locals = {} | ||
| exec(code, locals, locals) |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Since from __future__ import generator_stop no longer used, the trick with exec() no longer needed.
| except Exception as ex: | ||
| self.assertIs(type(ex), StopIteration) | ||
| self.assertEqual(ex.args[0], 'issue29692:Unchained') | ||
| self.assertIs(ex.__cause__, None) |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
assertIsNone()
| @@ -303,6 +303,10 @@ Extension Modules | |||
| Library | |||
| ------- | |||
|
|
|||
| - bpo-29692: Fixed arbitrary unchaining of RuntimeError exceptions in | |||
| contextlib.contextmanager | |||
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Add periods after sentence ends.
|
@serhiy-storchaka , thanks for the review. |
|
@ncoghlan could you please review the changes? Thanks |
|
Thanks for the fix, @svelankar! |
…ntimeError (pythonGH-949) contextlib._GeneratorContextManager.__exit__ includes a special case to deal with PEP 479 RuntimeErrors created when `StopIteration` is thrown into the context manager body. Previously this check was too permissive, and undid one level of chaining on *all* RuntimeError instances, not just those that wrapped a StopIteration instance. (cherry picked from commit 00c75e9)
…ntimeError (GH-949) (#1105) contextlib._GeneratorContextManager.__exit__ includes a special case to deal with PEP 479 RuntimeErrors created when `StopIteration` is thrown into the context manager body. Previously this check was too permissive, and undid one level of chaining on *all* RuntimeError instances, not just those that wrapped a StopIteration instance. (cherry picked from commit 00c75e9)
…ntimeError (pythonGH-949) contextlib._GeneratorContextManager.__exit__ includes a special case to deal with PEP 479 RuntimeErrors created when `StopIteration` is thrown into the context manager body. Previously this check was too permissive, and undid one level of chaining on *all* RuntimeError instances, not just those that wrapped a StopIteration instance. (cherry picked from commit 00c75e9)
…ntimeError (GH-949) (#1107) contextlib._GeneratorContextManager.__exit__ includes a special case to deal with PEP 479 RuntimeErrors created when `StopIteration` is thrown into the context manager body. Previously this check was too permissive, and undid one level of chaining on *all* RuntimeError instances, not just those that wrapped a StopIteration instance. (cherry picked from commit 00c75e9)
@ncoghlan @serhiy-storchaka @JelleZijlstra @brettcannon
I had to close PR 891 and open a new PR due to an incorrect rebase on my part.
All review comments made in PR 891 have been implemented and a test case added, please review, thanks.