Skip to content
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

Merged
merged 1 commit into from
Jul 30, 2024
Merged

Conversation

yabirgb
Copy link
Member

@yabirgb yabirgb commented Jul 29, 2024

Closes #8169

Checklist

  • The PR modified the frontend, and updated the user guide to reflect the changes.

Copy link

codecov bot commented Jul 29, 2024

Codecov Report

Attention: Patch coverage is 94.11765% with 2 lines in your changes missing coverage. Please review.

Project coverage is 53.81%. Comparing base (ab9e478) to head (b4db049).
Report is 13 commits behind head on bugfixes.

Files Patch % Lines
rotkehlchen/inquirer.py 93.54% 1 Missing and 1 partial ⚠️
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              
Flag Coverage Δ
backend 80.89% <94.11%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@yabirgb yabirgb added the ready for final review Backend PR ready to be reviewed by great Lefteris label Jul 29, 2024
Copy link
Member

@LefterisJP LefterisJP left a 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 Show resolved Hide resolved
):
Inquirer._msg_aggregator.add_error(
f'Token {asset} has itself as underlying token. Please edit the '
'asset to fix it.',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
'asset to fix it.',
'asset to fix it. Price queries will not work until this is done.',

rotkehlchen/inquirer.py Outdated Show resolved Hide resolved
rotkehlchen/inquirer.py Outdated Show resolved Hide resolved
@yabirgb yabirgb force-pushed the inquirer branch 2 times, most recently from d053cd2 to 65adad2 Compare July 29, 2024 16:57
Copy link
Member

@LefterisJP LefterisJP left a 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.

rotkehlchen/inquirer.py Outdated Show resolved Hide resolved
rotkehlchen/inquirer.py Outdated Show resolved Hide resolved
rotkehlchen/inquirer.py Outdated Show resolved Hide resolved
rotkehlchen/inquirer.py Outdated Show resolved Hide resolved
rotkehlchen/inquirer.py Outdated Show resolved Hide resolved
rotkehlchen/inquirer.py Outdated Show resolved Hide resolved
@yabirgb yabirgb force-pushed the inquirer branch 4 times, most recently from e44edc4 to 06b1dd6 Compare July 29, 2024 18:29
rotkehlchen/inquirer.py Outdated Show resolved Hide resolved
rotkehlchen/inquirer.py Outdated Show resolved Hide resolved
docs/changelog.rst Outdated Show resolved Hide resolved
@yabirgb yabirgb force-pushed the inquirer branch 5 times, most recently from 652c0cf to 074c1ea Compare July 30, 2024 13:40
docs/changelog.rst Outdated Show resolved Hide resolved
rotkehlchen/globaldb/manual_price_oracles.py Outdated Show resolved Hide resolved
rotkehlchen/inquirer.py Outdated Show resolved Hide resolved
rotkehlchen/inquirer.py Show resolved Hide resolved
Copy link
Member

@LefterisJP LefterisJP left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@LefterisJP LefterisJP merged commit 5cd0e8e into rotki:bugfixes Jul 30, 2024
14 checks passed
@yabirgb yabirgb deleted the inquirer branch September 27, 2024 11:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready for final review Backend PR ready to be reviewed by great Lefteris
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants