-
-
Notifications
You must be signed in to change notification settings - Fork 343
New issue
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
Don't double-wrap exceptions in nurseries #2826
Don't double-wrap exceptions in nurseries #2826
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #2826 +/- ##
=======================================
Coverage 99.16% 99.16%
=======================================
Files 115 115
Lines 17482 17514 +32
Branches 3113 3125 +12
=======================================
+ Hits 17336 17368 +32
Misses 101 101
Partials 45 45
|
I think it's fine to pass through. For the tests, there are currently enough separate cases that I can imagine missing that something is backwards. Could we write something parameterized, such that it obviously does each possible set of operations, and then the asserts only depend on whether we're in strict mode? (I expect this to be harder to read individually, but the confidence in consistency is what I'm after) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great to me - the tests still take a moment of thought, but suffice to convince me that it's now correct in general 🎉
The CI failures look like one flake, and one mypy issue to fix - trio/_core/_tests/test_run.py:2563: Missing type parameters for generic type "ExceptionInfo" [type-arg]
, I think ExceptionInfo[BaseException]
would be easiest - and once that's done let's merge!
Oh just realized this should probably also contain a newsfragment? |
Co-authored-by: Thomas Grainger <tagrain@gmail.com>
Ah shoot, forgot the newsfragment. Opening a new PR for it |
Fixes #2611
I should probably remove the extraneous tests, and merge it into one of the existing test files, or at the very least rename the test file.
@Zac-HD suggested
and I suppose that's maybe slightly different behavior from what I implemented?
I added a test case for when the child case itself raises an
ExceptionGroup
, should the traceback be modified in any way in that case or is it fine to just pass it through?