-
-
Notifications
You must be signed in to change notification settings - Fork 402
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
Conversation
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)
ce50449
to
d4686ff
Compare
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:
Guessing there's something wrong with my local environment, given that there's also a weird error from mypy when I run 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. |
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.
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 👍
I should probably just budget time for a nuke-and-rebuild of my local environment before we finalize 8.0. This plus the weird Thanks for checking! |
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 asexchangerate.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
make qa
(runsmake lint
andmake test
)sopel/tools/memories.py
, but it's unrelated to this patch.