-
-
Notifications
You must be signed in to change notification settings - Fork 160
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
Treat exceptions as normal variables properly #150
base: master
Are you sure you want to change the base?
Conversation
try: | ||
a = RuntimeError("A") | ||
raise a | ||
except b.__class__ as e: |
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.
it's not entirely clear to me what this part is for, mind elaborate?
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.
It's using a merge_stack_items on an actual error class from raise_args and a stack item containing b. Merging two SymbolWithCustomValue's (will be) done in simple test, this one tests merging a SymbolWithCustomValue with a CustomValue.
The second except is unnecessary since it's similar to the outer except.
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.
Consider breaking this test down to smaller ones, ideally with one-except, and please add proper comments. In any case, I'd want to avoid tests from which readers cannot see the intent at first glance.
@@ -0,0 +1,84 @@ | |||
import pytest |
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.
Can we add some tests in which exceptions have sources? e.g.
b = ... # a string
a = ImportError(b)
some test errors seem to be related to pdm. maybe try updating its version and try again? |
class CustomException(Exception): | ||
pass | ||
|
||
a = ImportError("A") |
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.
I think ImportError("A")
is equivalent to ImportError()
in our case? In other words, "A"
and "B"
are merely distractions, I'd probably remove them from all tests.
Also, having print(a)
doesn't seem very useful either.
Fixes #147
Use
SymbolWithCustomValueStackItem
class to allow exceptions to be treated as normal variables.