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

info USD (or any alias of USD) doesn't work until a currency calculation is performed #635

Open
rben01 opened this issue Oct 24, 2024 · 3 comments
Labels
bug Something isn't working

Comments

@rben01
Copy link
Contributor

rben01 commented Oct 24, 2024

>>> info USD
  Not found
>>> 1USD

  1 dollar

    = 1 $    [Money]

>>> info USD
  Unit: US dollar (https://en.wikipedia.org/wiki/United_States_dollar)
  Aliases: dollar, dollars, USD, usd, $
  A unit of: Money
  
  1 dollar = 0.92584 €

I assume that this is related to the lazy loading of currencies. Should currencies be loaded if info triggers a use?

@sharkdp
Copy link
Owner

sharkdp commented Dec 27, 2024

Thank you very much for reporting this. This lazy loading of currencies really causes a lot of issues. We've been recently discussing a general lazy evaluation strategy that would potentially solve this alltogether.

Should currencies be loaded if info triggers a use?

Yes. A similar problem exists with running list. I think we should probably trigger a load of the currency module whenever a command is run (assuming that we are not in no-prelude mode). Basically the same that we do here:

numbat/numbat/src/lib.rs

Lines 809 to 833 in 77decee

let mut no_print_settings = InterpreterSettings {
print_fn: Box::new(
move |_: &m::Markup| { // ignore any print statements when loading this module asynchronously
},
),
};
// We also call this from a thread at program startup, so if a user only starts
// to use currencies later on, this will already be available and return immediately.
// Otherwise, we fetch it now and make sure to block on this call.
{
let erc = ExchangeRatesCache::fetch();
if erc.is_none() {
return Err(Box::new(NumbatError::RuntimeError(
RuntimeError::CouldNotLoadExchangeRates,
)));
}
}
let _ = self.interpret_with_settings(
&mut no_print_settings,
"use units::currencies",
CodeSource::Internal,
)?;

@sharkdp sharkdp added the bug Something isn't working label Dec 27, 2024
@rben01
Copy link
Contributor Author

rben01 commented Dec 28, 2024

In theory we don't have to eagerly load the currencies on the first command — if it's info, we can look at the argument and then use the same logic as in Context::interpret_with_settings to decide to load the currencies. Same goes for list and list units (but not list functions). But no need to load currencies for save, help, and especially quit.

Separately, looking at the comment in that function

// TODO: maybe we can somehow load this list of identifiers from units::currencies?

Tackling this seems easy enough with a build script that parses units/currencies.nbt and emits a Rust array to a file, and then const CURRENCY_IDENTIFIERS: &[&str] = include!(...).

@sharkdp
Copy link
Owner

sharkdp commented Dec 28, 2024

Tackling this seems easy enough with a build script that parses units/currencies.nbt and emits a Rust array to a file, and then const CURRENCY_IDENTIFIERS: &[&str] = include!(...).

Yes. Let's maybe wait for the results of the "should Numbat do lazy evaluation by default?" discussion before investing time here though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants