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

Allow customizing behavior of exchange rate fetching #139

Closed
triallax opened this issue Aug 29, 2023 · 8 comments · Fixed by #227
Closed

Allow customizing behavior of exchange rate fetching #139

triallax opened this issue Aug 29, 2023 · 8 comments · Fixed by #227
Labels

Comments

@triallax
Copy link
Contributor

triallax commented Aug 29, 2023

Not everybody needs exchange rate information, or uses it often; some people will also take issue with Numbat using the network without their permission or awareness. Therefore, I propose adding the following options (or at least an appropriate subset):

  1. Fetching the rates on startup (the current behavior, adding a configurable time-out is also a good idea)
  2. Fetching them on first currency conversion, or something similar (I'd personally use this)
  3. Any other possible behaviors?
@sharkdp
Copy link
Owner

sharkdp commented Aug 29, 2023

see also: #97

Fetching them on first currency conversion, or something similar (I'd personally use this)

I thought about that but it really doesn't fit well into the overall architecture. We would have to pre-define units like USD before fetching the exchange rates (so that users can type 13 USD -> EUR without getting a unknown-identifier error). Without having the exchange rates, we would have to give those units a dummy conversion factor.

And then we would have to have special handling in the interpreter to detect the usage of those units in order to schedule the exchange rate update.

Any other possible behaviors?

I thought about a simple on/off setting. Either you fetch them at startup, or you don't get currency units.

@triallax
Copy link
Contributor Author

I thought about that but it really doesn't fit well into the overall architecture. We would have to pre-define units like USD before fetching the exchange rates (so that users can type 13 USD -> EUR without getting a unknown-identifier error). Without having the exchange rates, we would have to give those units a dummy conversion factor.

Fair point.

I thought about a simple on/off setting. Either you fetch them at startup, or you don't get currency units.

Is it too difficult to make that setting possible to change at runtime too?

@sharkdp
Copy link
Owner

sharkdp commented Sep 28, 2023

I found a way to implement on-demand fetching now: see #107. This also solves the error handling part.

What is still left open is to disable prefetching (which we currently still do by default, to have units readily available once the user needs them)

@triallax
Copy link
Contributor Author

Looks good to me, thanks!

@triallax
Copy link
Contributor Author

What is still left open is to disable prefetching (which we currently still do by default, to have units readily available once the user needs them)

I think I'm gonna work on this, do you have any syntax in mind? Is it gonna be something like #55, or a line like set exchange_rate_fetching off in init.nbt?

@sharkdp
Copy link
Owner

sharkdp commented Oct 26, 2023

I think this will be a CLI-application-only thing, so that should go into the (not yet existing) config file. See #97 for details. I will add some more thoughts there.

@triallax triallax mentioned this issue Nov 4, 2023
1 task
@triallax
Copy link
Contributor Author

triallax commented Nov 6, 2023

Another thing I didn't consider when initially opening the issue: exchange rates get out of date fairly quickly, how would Numbat handle that? Would it reload the exchange rates after e.g. 1 day? Should the period be configurable? How would this play with the fetching behavior setting proposed in this issue?

It might be a good idea to see how other software (e.g. Qalculate) handles this and other issues that exchange rate fetching brings up.

@triallax
Copy link
Contributor Author

triallax commented Nov 6, 2023

Relevant discussion on IRC:

11:10 <triallax> hmm, i'm wondering if it might be a good idea to allow disabling currency fetching altogether
11:11 <triallax> in that case, we could either not allow conversion between currencies, or just remove currency units
13:24 <achin> we should still keep the units, so that someone can use them in their own custom functions
13:25 <triallax> yeah but then each unit would have to have its own "dimension"
13:25 <triallax> which is kind of weird
13:25 <triallax> and probably not very fun to implement
13:31 <achin> why would each unit need its own dimension?
13:31 <triallax> well, because we don't have exchange rates
13:32 <triallax> but if they're all the same dimension, we need to have some kind of conversion factor
13:32 <triallax> factors*
13:54 <sharkdp> You can also have multiple 'base' units of the same dimension, without conversion factors (dimension D; unit a: D; unit b: D). In that case, you will get a runtime error if you attempt any conversions (unit a can not be converted to b). But I'm not sure if I want to keep that behavior forever
13:54 <triallax> interesting
13:54 <triallax> why do you think you don't want to keep that behavior?
13:55 <triallax> it seems useful in this case at least
13:55 <sharkdp> I think it would be a reasonable first version to just show a runtime error in case exchange-rate-download is deactivated (and someone wants to use currency units). Similar to what we do if there are any network errors
13:57 <sharkdp> Because it feels to me like an inconsistent unit system. And maybe it would be better to show users an error if they attempt to create a second base unit for a given dimension.
13:57 <triallax> fair enough
13:57 <triallax> so we have three possible behaviors
13:57 <triallax> prefetching enabled, fetch on first use, and no fetching at all
13:57 <triallax> does that make sense?
14:17 <sharkdp> yes, thank you
14:20 <triallax> right, and with the last one we don't have currency units at all
14:34 <sharkdp> yes. and with the second one we expect a short delay when using currencies in the REPL
14:34 <triallax> yeah
14:34 <triallax> i did try it with my pr, it's not too bad
14:34 <triallax> but my internet is fairly fast
14:34 <sharkdp> when doing something like `numbat -e "USD -> EUR"`, we always expect a delay, of course.
14:35 <triallax> of course yeah
14:36 <sharkdp> I regularly work behind a company proxy where the delay is noticeable. And sometimes I also get failures if proxy settings are not correct, for example.
14:36 <triallax> yeah, that's why i want to allow prefetching
14:36 <triallax> the next question to bikeshed over is, what should be put as the default
14:40 <sharkdp> One request is 1.5 KiB. If you open Numbat 10x per day, every day - that's 5 MiB in a full year. I think that is acceptable :-)
14:41 <sharkdp> i.e. I'm voting for prefetching as a default
14:41 <triallax> perhaps, but not everybody likes it when software phones somewhere without their knowledge/consent
14:42 <triallax> oh, and what about when the data becomes outdated
14:42 <triallax> like, if somebody has a long-running numbat session, over one week for instance
14:42 <triallax> god this currency thing opened up a can of worms
14:42 <sharkdp> that is something I have not considered
14:42 <triallax> i'll open an issue for it
14:43 <triallax> but it might be a good idea to hold back on any currency conversion-related changes until we think through all of this
14:43 <sharkdp> yes, regarding the can of worms. I would really like to add time/date handling to Numbat. But this will be an even bigger rabbit hole, I'm afraid.
14:43 <triallax> yeah i would love that, but it's probably not gonna be easy
14:44 <triallax> every second i deal with dates and times is 1% of my sanity being lost
14:48 <sharkdp> Maybe we can just expose part of the Rust timestamp handling in a nice way. Without trying to reimplement stuff
14:51 <triallax> it might be worth looking at how qalculate handles exchange rates
14:52 <sharkdp> I did :-)
14:52 <triallax> iirc it was a lot more sophisticated than what we do
14:52 <triallax> but that's qalculate's style, it brings the kitchen sink along with it lol
14:53 <sharkdp> It also uses ECB exchange rates (and maybe additional sources), but keeps a local cache of the downloaded file. But then they ask the user if they want to update the file once it's older than a week or so. I personally don't like the interactiveness of this.
14:54 <triallax> so you want it to be handled transparently in the background?
14:54 <sharkdp> Yes
14:54 <triallax> i agree
14:54 <triallax> but that brings up the issue of what if the fetching fails
14:54 <triallax> i would definitely like to be informed if that happens
14:54 <triallax> rather than risk using stale exchange rate information
14:54 <sharkdp> exactly
14:55 <sharkdp> With Insect and Numbat I always followed the approach: our users want their computations to be correct. So we rather show an error message than trying to patch up things (by guessing).

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

Successfully merging a pull request may close this issue.

2 participants