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: replace exchangerate.host -> open.er-api.com #2512

Merged
merged 1 commit into from
Oct 11, 2023

Conversation

dgw
Copy link
Member

@dgw dgw commented Oct 1, 2023

exchangerate.host was bought and changed to require an API key. The new owner also allegedly broke compatibility with the original endpoints, based on GitHub comments from other developers, so I didn't even bother signing up for a key and testing it. I just found a new keyless API.

ExchangeRate-API.com also provides key-based services, but there's an open endpoint that doesn't require any key we can use for now.

VCR file for currency example test has been updated, as well.

Note that this will be a "breaking" change for users who specify the currency.fiat_provider option in their settings as exchangerate.host since that's no longer a valid value. (Ask me how I know... I forgot to update the default at first and the test suite failed because of it. :D)

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 lint and make test)
    • Technically, my local lint reports a weird mypy error in sopel/tools/memories.py, but it's unrelated to this patch.
  • I have tested the functionality of the things this change touches
    • Both interactively and via updated VCR file for the new network request.

@dgw dgw added Tests Bugfix Generally, PRs that reference (and fix) one or more issue(s) Breaking Change Stuff that probably should be mentioned in a migration guide Housekeeping Code cleanup, removal of deprecated stuff, etc. labels Oct 1, 2023
@dgw dgw added this to the 8.0.0 milestone Oct 1, 2023
@dgw dgw requested a review from a team October 1, 2023 20:26
exchangerate.host was bought and changed to require an API key. The new
owner also allegedly broke compatibility with the original endpoints,
based on GitHub comments from other developers, so I didn't even bother
signing up for a key and testing it. I just found a new keyless API.

ExchangeRate-API.com also provides key-based services, but there's an
open endpoint that doesn't require any key we can use for now.

VCR file for `currency` example test has been updated, as well.

Note that this will be a "breaking" change for users who specify the
`currency.fiat_provider` option in their settings as `exchangerate.host`
since that's no longer a valid value. (Ask me how I know... I forgot to
update the default at first and the test suite failed because of it. :D)
@dgw
Copy link
Member Author

dgw commented Oct 1, 2023

OK, I'm at a bit of a loss. The bundled VCR cassettes work in our CI, but fail for me locally with a weird Brotli error:

_____________________________________________ test_example_exchange_cmd_0 ______________________________________________
/home/dgw/github/sopel/sopel/tests/pytest_plugin.py:148: in test
    ???
E   AssertionError: Output does not match the regex:
E   Pattern: 100 USD is [\d\.]+ BTC, [\d\.]+ CAD, [\d\.]+ EUR, \(unsupported: CAN, AUX\)
E   Output: Something went wrong while I was getting the exchange rate.
-------------------------------------------------- Captured log call ---------------------------------------------------
ERROR    sopel.modules.currency:currency.py:127 Error in GET request: ('Received response with content-encoding: br, but
failed to decode it.', error('BrotliDecoderDecompressStream failed while processing the stream'))

Guessing there's something wrong with my local environment, given that there's also a weird error from mypy when I run make lint (as mentioned in the PR description here) that doesn't happen in CI.

Whoever reviews this, if you have a moment to checkout and test locally, it's not part of the PR but I'd appreciate knowing whether this failure also happens for you locally.

Copy link
Contributor

@SnoopJ SnoopJ left a comment

Choose a reason for hiding this comment

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

Plugin change looks fine to me, I'm pretty impressed that the change is just the URL, but I guess it makes sense that providers of this information would naturally settle on similar response formats. And I trust that you did the right thing with the VCR changes as long as the associated tests pass.

Whoever reviews this, if you have a moment to checkout and test locally, it's not part of the PR but I'd appreciate knowing whether this failure also happens for you locally.

QA passes for me on 3.8-3.11 👍

@dgw
Copy link
Member Author

dgw commented Oct 10, 2023

Whoever reviews this, if you have a moment to checkout and test locally, it's not part of the PR but I'd appreciate knowing whether this failure also happens for you locally.

QA passes for me on 3.8-3.11 👍

I should probably just budget time for a nuke-and-rebuild of my local environment before we finalize 8.0. This plus the weird mypy check failure that nobody else sees (gone after #2514) makes me just not trust anything my terminal tells me.

Thanks for checking!

@dgw dgw merged commit 4c7b507 into master Oct 11, 2023
13 checks passed
@dgw dgw deleted the currency-api-changes branch October 11, 2023 21:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Breaking Change Stuff that probably should be mentioned in a migration guide Bugfix Generally, PRs that reference (and fix) one or more issue(s) Housekeeping Code cleanup, removal of deprecated stuff, etc. Tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants