bpo-30579: Allow TracebackType creation and tb_next mutation from Python #4793
Conversation
There was a problem hiding this comment.
The change itself looks good to me, but the remaining assert statements in the test case need to be converted to the appropriate unittest methods: https://docs.python.org/3/library/unittest.html#assert-methods
Lib/test/test_raise.py
Outdated
| except Exception as exc: | ||
| tb = exc.__traceback__ | ||
|
|
||
| assert isinstance(tb.tb_next, types.TracebackType) |
There was a problem hiding this comment.
These assertions all need to use the unittest assert methods, not assert statements.
|
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 |
|
Whoops, good catch, thanks. I have made the requested changes; please review again. |
|
Thanks for making the requested changes! @ncoghlan: please review the changes made to this pull request. |
There was a problem hiding this comment.
This mostly looks good to me now, just a minor comment relating to the possible use of assertIsInstance for slightly nicer failure messages.
Marking my review as approved anyway though, as I'll be offline until tomorrow, and I'm fine with someone else merging this in the meantime :)
Lib/test/test_raise.py
Outdated
| except Exception as exc: | ||
| tb = exc.__traceback__ | ||
|
|
||
| self.assertTrue(isinstance(tb.tb_next, types.TracebackType)) |
There was a problem hiding this comment.
There's a dedicated method for this one that gives a nicer message when it fails: https://docs.python.org/3/library/unittest.html#unittest.TestCase.assertIsInstance
This is admittedly an obscure use case, but currently there are projects like Jinja2 and Trio that are forced to work around this with horrible ctypes hacks: https://github.com/pallets/jinja/blob/fe3dadacdf4cf411d0a5b6bbd4d5234697a28af2/jinja2/debug.py#L345 https://github.com/python-trio/trio/blob/1e86b1aee8c0c759f6f239ae53a05d0d3963c629/trio/_core/_multierror.py#L296
|
Well that's embarrassing -- I even looked for |
This is admittedly an obscure use case, but currently there are
projects like Jinja2 and Trio that are forced to work around this with
horrible ctypes hacks:
https://github.com/pallets/jinja/blob/fe3dadacdf4cf411d0a5b6bbd4d5234697a28af2/jinja2/debug.py#L345
https://github.com/python-trio/trio/blob/1e86b1aee8c0c759f6f239ae53a05d0d3963c629/trio/_core/_multierror.py#L296
https://bugs.python.org/issue30579