elixir-cldr / cldr_territories

Territory formatting functions for the Common Locale Data Repository (CLDR) package https://github.com/elixir-cldr/cldr
Other
23 stars 11 forks source link

"no case clause matching", when calling to_currency_code/1 #47

Closed Stroemgren closed 4 days ago

Stroemgren commented 2 weeks ago

I needed a list of all currency codes and wasn't at the time aware of the ex_cldr_currencies package, so I took the approach of just getting all known country codes and mapping over to_currency_code/1. This led to an error where the map would run for most of the country codes and then break on :CQ, specifically.

It's reproducible with:

defmodule Test.Cldr do
  use Cldr,
    locales: [:en],
    default_locale: "en",
    providers: [Cldr.Territory]
end

Test.Cldr.Territory.to_currency_code(:CQ)

Stacktrace:

** (CaseClauseError) no case clause matching: {:ok, []}
    (ex_cldr_territories 2.9.0) lib/cldr/territory.ex:822: Cldr.Territory.to_currency_code/2

I just ran it in isolation on a Livebook with the following deps.

Mix.install([
  {:ex_cldr, "~> 2.40"},
  {:ex_cldr_territories, "~> 2.9"},
  {:jason, "~> 1.4"}
])
Schultzer commented 5 days ago

Thanks, ~looks like we need to handle the case of territories with no currencies in this case statement https://github.com/elixir-cldr/cldr_territories/blob/f1639c38cba9905d55789052502f606c66c5f95f/lib/cldr/territory.ex#L822.~

if you are up for it then a PR is welcomed 🙏

The country code is the Island of Sark which is under the UK, there might be and subdivision alias we can use to resolve the proper currency code.

Stroemgren commented 5 days ago

I'm up for giving it a go, but I will probably need some direction. At least it depends on the direction.

So either we should return a proper error for this case or we resolve the currency through subdivisions. Sark Island doesn't have any subdivisions though :)

Stroemgren commented 5 days ago

https://github.com/elixir-cldr/cldr_territories/pull/48

This will return {:ok, nil} and nil for the bang version.

Let me know if you'd rather solve it differently.

Schultzer commented 5 days ago

I'm up for giving it a go, but I will probably need some direction. At least it depends on the direction.

So either we should return a proper error for this case or we resolve the currency through subdivisions. Sark Island doesn't have any subdivisions though :)

I believe that Sark Island would be under UK, so it might be possible to resolve it by looking up the parent, or maybe the UK subdivision aliases can resolve CQ.

We’re getting the currencies from the info function on https://github.com/elixir-cldr/cldr_territories/blob/f1639c38cba9905d55789052502f606c66c5f95f/lib/cldr/territory.ex#L886

I believe that is where we want to place the logic.

I have not looked at the data available for CQ or the UK so I can’t give you exact instructions.

Regardless, since these things are political and changes over time, then I’m still not sure if we want to error or resolve. @kipcole9 what do you think.

For what is worth, Wikipedia list their currency as GBP https://en.m.wikipedia.org/wiki/Sark

kipcole9 commented 5 days ago

Not all territories have a known currency. Antartica would be one case. CLDR returns a currency XXX in that case:

iex> Cldr.Currency.territory_currencies(:AQ)
{:ok, %{XXX: %{tender: false}}}

XXX or similar is used in various places in CLDR to denote "Unknown".

In other cases, CLDR doesn't have a currency definition as you observed. In this case ex_cldr_currencies returns an error:

iex> Cldr.Currency.territory_currencies(:CQ)
{:error, {Cldr.UnknownCurrencyError, "No currencies for :CQ were found"}}

I think for consistency purposes, this would be the appropriate thing to do in ex_cldr_territories.

BTW, according to Sark's website the currency is the Guernsey Pound - albeit thats a local issue of the GBP.

kipcole9 commented 5 days ago

And I should probably also return {:error, {Cldr.UnknownCurrencyError, "No currencies for :__ were found"}} when CLDR would return XXX too, like for the :AQ example.

Stroemgren commented 4 days ago

Updated the PR.