currency: Moved to different API, support multiple target currencies, cache rates #1430
Conversation
Duckle29
changed the title
|
I added the option to configure the module with a fixer.io API key, as fixer.io has 167 currencies while exchangeratesapi.io has 32 additionally, the module can be configured to also respond directly to regex matches. If no configuration is done, the module defaults to exchangerates.io and no regex. |
dgw
changed the title
dgw
changed the title
|
Look at that title-edit mess I just made, ugh. 😁 @Duckle29 Are you OK with leaving your |
|
@dgw Yeah that's okay. If I have to work on something in the future I'll make sure to move to a different branch, but for now it works as it should :) |
There was a problem hiding this comment.
Looks like @HumorBaby already covered quite a bit before I got to this! I'm mostly pointing out stuff he couldn't know about (because it's only in my head so far).
Besides the line notes left by the two of us, you'll need to rebase this, re-add your new copyright line into the new file header, and sort out the imports: Get them sorted more or less alphabetically and grouped by standard (re and time), third-party (requests), and project (anything under sopel). As explained in one of those comments, we're going to enforce import ordering at some point, so I'm starting to keep an eye out for it already. (And I already fixed a lot of modules' imports in #1402, including this one, so I'm protecting work I already did. 😛)
There was a problem hiding this comment.
Preliminary testing with master merged in looks good so far! All previously requested changes have been addressed, and so my final note is: why not throw a test into the example for good measure? 😸
Approving since they are not required 🏅 Bah... accidentally submitted without approving!!! (so approved from my old review)
|
Just a heads up to re-clone my fork if testing it as I squashed a few commits together again. |
There was a problem hiding this comment.
I hate to do this, but I have one small request for this PR 😬 I will admit, however, that it was completely my fault due to a miscommunication in one of my review comments regarding the initialization of the rates_fiat_json dictionary 😭 (see comment). The other is a small style change.
Everything else LGMT 🏅
|
For the record, I'm perfectly happy to merge this as soon as @HumorBaby's small request is taken care of. We can continue tweaking the actual output format (when to use decimals, how many places, etc.) in a future PR, maybe for Sopel 7.1. |
Switched to exchangeratesapi.io with fixer.io support Supports multiple target currencies in one conversion Can be directly triggered on a regex match if enabled in the config Removed the separate BTC callable, as the main function handles BTC just fine. made it caches rates Co-authored-by: dgw <dgw@technobabbl.es> Co-authored-by: Humorous Baby <humorbaby@humorbaby.net>
|
I've been using it, and it works! 🚀 |
Hey there. Since I was moving from an old bot to sopel, I wanted to have it mimic what I and others were used to from my old bot. Because of this, I made it support multiple target currencies like:
.cur 100 usd in dkk eur usd aud cad btcadditionally, I moved to https://exchangeratesapi.io/ as the bank of Canada apparently doesn't offer rates for the Danish krona. Exchangeratesapi doesn't have a whole ton of currencies either, but the output is fixer.io compatible, so if it becomes a problem, a simple change would be to add a config section, allowing people to choose between API-key-less, exchangeratesapi.io or getting a free API key from fixer.io
A free API key should be more than enough as well, as the module caches rates for 24h in a global variable, and the free fixer.io plan offers 1000 requests / month