elixir-cldr / cldr_units

Unit formatting (volume, area, length, ...) functions for the Common Locale Data Repository (CLDR)
Other
16 stars 13 forks source link

Cldr.Unit.Conversion does not deal with decimal values #8

Closed LostKobrakai closed 5 years ago

LostKobrakai commented 5 years ago

As cldr is already requiring the decimal library, couldn't Cldr.Unit normalize to just using Decimal internally?

test "decimal" do
    unit = Cldr.Unit.new(Decimal.new("300"), :minute)

    hours = Cldr.Unit.Conversion.convert(unit, :hour)

    assert hours.unit == :hour
    assert Decimal.equal?(5, Cldr.Unit.to_value(hours))
end
kipcole9 commented 5 years ago

Yes, it should support Decimal since all the Cldr libs should be consistent in this respect. Will add it to the weekend list to fix.

kipcole9 commented 5 years ago

Fixed, sorry it took so long. Published on hex as version 1.2.2

LostKobrakai commented 5 years ago

Can we make cldr_unit use only decimals internally? ex_cldr does depend on decimal anyways and results like the ones below just aren't nice to work with. (this is on 1.3.0)

iex(12)> Cldr.Unit.new!(Decimal.new(28800), :second) |> Cldr.Unit.convert(:minute)
#Unit<:minute, #Decimal<480.009600>>
kipcole9 commented 5 years ago

Actually its all decimal calculations for a Unit that is decimal-based.

But even using decimals, the current conversion strategy, which always has two steps: to a common unit, then to the desired unit, has issues. For example:

iex> to_microsecond = Decimal.new("1.0e-6")
#Decimal<0.0000010>
iex> to_minute = Decimal.new("1.6667e-8")  
#Decimal<1.6667E-8>
iex> Decimal.div(m, to_microsecond) |> Decimal.mult(to_minute)
#Decimal<480.00960>

So two thoughts:

  1. Add some additional direct conversion measures that are only a single calculation. Ie capture that seconds to minutes is 60.
  2. Apply rounding (which of course is already possible)
LostKobrakai commented 5 years ago

@kipcole9 Maybe we can just flip the numbers from representing fractions of the smallest unit to be how many of the smallest unit fits into the current one: https://gist.github.com/LostKobrakai/4419bd3c245272e2fc04ef857c3e0f4a