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

currency: improve error handling #2053

Merged
merged 2 commits into from
May 3, 2021
Merged

currency: improve error handling #2053

merged 2 commits into from
May 3, 2021

Conversation

dgw
Copy link
Member

@dgw dgw commented Apr 9, 2021

Description

This fixes two gaps in the currency plugin's error handling:

  1. KeyError was not handled. Such an exception could result if the exchange rate API returns valid JSON that doesn't contain the expected keys (perhaps an error reply).
  2. rates_updated was refreshed before executing all code that could prematurely terminate the update operation, leading to some (but not all) of the data being stale for at least 24 hours until the next full update_rates() run.

Additionally, catching KeyError will allow the bot to continue converting the input as normal if it has rates cached.

Checklist

  • I have read CONTRIBUTING.md
  • I can and do license this contribution under the EFLv2
  • No issues are reported by make qa (runs make quality and make test)
  • I have tested the functionality of the things this change touches
    • Theoretically, at least. I don't have a good way to generate API errors at will. A good future enhancement would be writing a test suite for this plugin in test/modules/test_modules_currency.py that explicitly feeds it bad JSON (invalid, missing expected keys, etc.) to verify that the error handling works as expected.

Notes

Discovered point 1 above when I ran the .cur command for the first time in a while and it presumably tried to update the exchange rates, but couldn't find rates_fiat['rates']. Running the command again succeeded, hence my decision to add "Try again later." as part of the revised error message.

Possible future enhancement: adding a note to the conversion output if the cached rates are older than 2-3x the refresh interval.

@dgw dgw added the Bugfix Generally, PRs that reference (and fix) one or more issue(s) label Apr 9, 2021
@dgw dgw added this to the 7.1.0 milestone Apr 9, 2021
@dgw dgw requested a review from a team April 9, 2021 05:18
@dgw dgw merged commit a033cb1 into master May 3, 2021
@dgw dgw deleted the currency-KeyError branch May 3, 2021 01:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bugfix Generally, PRs that reference (and fix) one or more issue(s)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants