-
-
Notifications
You must be signed in to change notification settings - Fork 796
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
fix[lang]: show user error in error map #4286
base: master
Are you sure you want to change the base?
fix[lang]: show user error in error map #4286
Conversation
raise "some error" | ||
""" | ||
error_map = compile_code(code, output_formats=["source_map"])["source_map"]["error_map"] | ||
assert "user revert with reason" in list(error_map.values()) |
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.
why can't we check in error_map.values()
?
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.
We can. test_error_map
also wraps the values in list - should I change it there too?
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 a dict view, i think we should. @charles-cooper do you remember the reason you did it this way?
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 a dict view, i think we should. @charles-cooper do you remember the reason you did it this way?
no, just habit i think
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #4286 +/- ##
==========================================
- Coverage 91.40% 90.24% -1.17%
==========================================
Files 112 112
Lines 15927 15935 +8
Branches 2694 2696 +2
==========================================
- Hits 14558 14380 -178
- Misses 935 1087 +152
- Partials 434 468 +34 ☔ View full report in Codecov by Sentry. |
return IRnode.from_list(ir_node, error_msg="user revert with reason") | ||
|
||
ir_node = IRnode.from_list(ir_node) | ||
ir_node.set_error_msg("user revert with reason") |
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.
what if some of the arguments have their own error message? Then it gets overwritten although it might be more specific.
eg in the case of raise self.bar(5 % i)
the mod is annotated with a safemod
error msg, which will be replaced with user revert with reason
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.
Hmm I did implement set_error_msg to override, but that could be changed. @sandbubbles could you change that in this PR to not override? And add @cyberthirst's test case raise self.foo(5%i)
to the test suite.
What I did
Fix #4199
How I did it
Moved error message to "revert"
How to verify it
Commit message
Commit message for the final, squashed PR. (Optional, but reviewers will appreciate it! Please see our commit message style guide for what we would ideally like to see in a commit message.)
Description for the changelog
Cute Animal Picture