elixir-cldr / cldr_numbers

CLDR Number localisation and formatting
Other
41 stars 21 forks source link

"bad argument in arithmetic expression" in short formatter with Decimal #34

Closed dbernheisel closed 1 year ago

dbernheisel commented 1 year ago

Seems there's some arithmetic that doesn't check the type before operating on it.

Mix.install([:decimal, :ex_cldr_numbers])

defmodule MyApp.Cldr do
  use Cldr, locales: [:en], providers: [Cldr.Number]
end

amount = Decimal.new("940038.00000000")
rounded = Decimal.round(amount, -2, :down)
integered = Decimal.to_integer(rounded)

{:ok, "940K"} = MyApp.Cldr.Number.to_string(amount, format: :short)
{:ok, "940K"} = MyApp.Cldr.Number.to_string(integered, format: :short)
MyApp.Cldr.Number.to_string(rounded, format: :short) # boom
** (ArithmeticError) bad argument in arithmetic expression
    (ex_cldr_numbers 2.29.0) lib/cldr/number/formatter/short_formatter.ex:305: Cldr.Number.Formatter.Short.pluralization_key/2
    (ex_cldr_numbers 2.29.0) lib/cldr/number/formatter/short_formatter.ex:198: Cldr.Number.Formatter.Short.choose_short_format/4
    (ex_cldr_numbers 2.29.0) lib/cldr/number/formatter/short_formatter.ex:104: Cldr.Number.Formatter.Short.short_format_string/6
    (ex_cldr_numbers 2.29.0) lib/cldr/number.ex:431: Cldr.Number.to_string/3
    iex:6: (file)
kipcole9 commented 1 year ago

Definitely a bug, thanks for the report. I am working on this lib over the weekend and will have a new version published by Sunday.

BTW, I also see:

iex> {:ok, "940K"} = MyApp.Cldr.Number.to_string(stringed, format: :short)
** (MatchError) no match of right hand side value: {:error, {ArgumentError, "Not a number: \"940000\". Long and short formats only support number or Decimal arguments"}}
    (stdlib 4.2) erl_eval.erl:496: :erl_eval.expr/6
    iex:6: (file)

Which I think is the correct response. Is that what you are also seeing?

kipcole9 commented 1 year ago

Fixed on my locale machine but its blended with some other uncommitted work-in-progress (my sloppiness) so not yet pushed to GitHub.

dbernheisel commented 1 year ago

sorry, you're right. I'll edit the correction. I did try the stringed version but as you say it doesn't accept that type. Since I'm rounding to integers, I ended up using Decimal.to_integer(rounded)

kipcole9 commented 1 year ago

@dbernheisel I believe this is now fixed in the currency branch. If you have a moment would you consider testing with that branch? ie {:ex_cldr_numbers, github: "elixir-cldr/cldr_numbers", branch: "currency"} in your mix.exs. Of course please also let me know if you see any other issues.

dbernheisel commented 1 year ago

A quick validation showed that it's fixed, but it's difficult for me to anticipate the numbers that fall into this scenario, and for me they change frequently throughout the day, so I'm going to let it run all day on my app and I'll let you know tomorrow if I still see the issue.

I really appreciate the quick bugfix!

kipcole9 commented 1 year ago

I've merged the commits into the master branch so just {:ex_cldr_numbers, github: "elixir-cldr/cldr_numbers"} is required now. Ready to publish when you're ok after your test runs.

dbernheisel commented 1 year ago

So far it's good! No issues anymore 👍

kipcole9 commented 1 year ago

Thanks for the feedback and thanks for your patience. This was an easy fix - but I made the mistake of blending it into some other work that took longer. I'll publish to hex now and close the issue.