-
-
Notifications
You must be signed in to change notification settings - Fork 522
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
Avoid recursion error in inquirer #8341
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## bugfixes #8341 /- ##
============================================
Coverage 53.80% 53.81% 0.01%
============================================
Files 1707 1708 1
Lines 168704 168797 93
Branches 13956 13980 24
============================================
Hits 90774 90846 72
- Misses 75505 75526 21
Partials 2425 2425
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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.
Some comments but also I would suggest catching somewhere in the root of the function callls the possible exceptions and logging an error and exiting gracefully there too.
So catch RecursionError
and then perhaps the other ones that were raised during our tests?
rotkehlchen/inquirer.py
Outdated
): | ||
Inquirer._msg_aggregator.add_error( | ||
f'Token {asset} has itself as underlying token. Please edit the ' | ||
'asset to fix it.', |
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.
'asset to fix it.', | |
'asset to fix it. Price queries will not work until this is done.', |
d053cd2
to
65adad2
Compare
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.
Left some comments. Also wouldn't it make sense for this to be a decorator? It's repeating for everything.
e44edc4
to
06b1dd6
Compare
652c0cf
to
074c1ea
Compare
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.
LGTM
Closes #8169
Checklist