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

Cache exchange rates #403

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

P1n3appl3
Copy link
Contributor

I use lots of short invocations of numbat with fetching-policy = "on-first-use" which means it re-fetches the rates every time I open it to do a quick currency conversion. The on-startup fetching policy makes it a bit better, but the delay is still noticeable if the first thing I do uses currency, and it just feels wasteful to download them every time when they're only updated daily. This change reuses the fetched rates for 24 hours to alleviate that delay.

This could make the rates up to a day out of date (if you happened to fetch them the first time right before the European Central Bank releases the next day's rates). In my mind this isn't hugely important as anyone who needs super up-to-date rates is probably using a different source in the first place, but the max age could be changed to something shorter to make it fetch more aggressively. If users want even more control, max-age could be specified as a config option in the [exchange-rates] section.

@sharkdp
Copy link
Owner

sharkdp commented Mar 19, 2024

Thank you for looking into this. I believe we also have a ticket for this.

I only had a quick glance at the code. I think the caching should rather be moved to the application layer, not to the numbat-exchange-rates library. I realize that this is a bit harder to implement (as we need to serialize/deserialize), but I think that would be cleaner.

@P1n3appl3
Copy link
Contributor Author

When you say it should be in the "application layer" do you mean that numbat-cli should specify how writing and loading the cache file works? For that to work with on-demand currency loading there would need to be some sort of callback when the rates get fetched. I've roughed out that change in this commit but it a bit of a mess. Let me know if that's the direction you were thinking of and if so I'll try to clean it up and add it here.

Also I see #315 but that's kind of the opposite issue, was there another ticket you were thinking of?

@sharkdp
Copy link
Owner

sharkdp commented Mar 25, 2024

I don't fully understand why we need this setup with two closures and additional locks.

Could we add a (possibly atomic?) filesystem_caching_enabled flag to the ExchangeRates struct, set that according to the config when starting the application, and then internally have two branches for the caching/non-caching case?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants