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

A :duration vs :year_duration inconsistency #24

Closed
DaTrader opened this issue Dec 25, 2021 · 21 comments
Closed

A :duration vs :year_duration inconsistency #24

DaTrader opened this issue Dec 25, 2021 · 21 comments

Comments

@DaTrader
Copy link

DaTrader commented Dec 25, 2021

There's an inconsistency between the known unit categories and the unit category I get for a duration in years as show below:

iex(7)> Cldr.Unit.known_unit_categories()
[:acceleration, :angle, :area, :concentr, :consumption, :digital, :duration,
 :electric, :energy, :force, :frequency, :graphics, :length, :light, :mass,
 :power, :pressure, :speed, :temperature, :torque, :volume]
iex(8)> per = Cldr.Unit.parse!( "2y")
#Cldr.Unit<:year, 2>
iex(9)> { :ok, :duration} = Cldr.Unit.unit_category( per)
** (MatchError) no match of right hand side value: {:ok, :year_duration}

If this is returning :year_duration is the desired behavior here, how do I get to know all possible return values for my duration units so I can match against them?

@kipcole9
Copy link
Collaborator

This one is a little trickier, definitely a corner case. Will take a few hours to resolve properly.

@kipcole9
Copy link
Collaborator

kipcole9 commented Dec 25, 2021

Fixed on master. Following on from the discussion on ex_cldr_calendars, here is my recommendation on formatting days, weeks and quarters:

Days and Weeks (and Months, Years, Decades and Centuries)

These are all built-in units so no special handling is required.

iex> days = Cldr.Unit.new!(:day, 2)
#Cldr.Unit<:day, 2>
iex> MyApp.Cldr.Unit.to_string days
{:ok, "2 days"}

iex> weeks = Cldr.Unit.new!(:week, 2)
#Cldr.Unit<:week, 2>
iex> MyApp.Cldr.Unit.to_string weeks 
{:ok, "2 weeks"}
iex> MyApp.Cldr.Unit.to_string weeks, locale: "fr"
{:ok, "2 semaines"}
iex> MyApp.Cldr.Unit.to_string weeks, locale: "de"
{:ok, "2 Wochen"}
iex> MyApp.Cldr.Unit.to_string weeks, locale: "he"
{:ok, "שבועיים"}

Quarters

The unit quarter is not defined by CLDR but I have opened a ticket. In the mean time you can define a custom unit. Apply the following:

  1. Define the :quarter unit in config.exs:
config :ex_cldr_units, :additional_units,
  quarter: [
    base_unit: :year,
    factor: %{numerator: 1, denominator: 4},
    offset: 0,
    sort_before: :all,
  ]
  1. Define the localisations in your backend module (note the addition of use Cldr.Unit.Additonal at the top of the module):
defmodule MyApp.Cldr do
  use Cldr.Unit.Additional

  use Cldr,
    locales: ["en", "fr"],
    default_locale: "en",
    providers: [Cldr.Number, Cldr.Unit, Cldr.List]

  # Unit Quarter

  unit_localization(:quarter, "en", :long,
    nominative: %{
      one: "{0} quarter",
      other: "{0} quarters"
    },
    display_name: "quarters"
  )

  unit_localization(:quarter, "en", :short,
    nominative: %{
      one: "{0} qtr",
      other: "{0} qtrs"
    },
    display_name: "quarters"
  )

  unit_localization(:quarter, "en", :narrow,
    nominative: %{
      one: "{0} q",
      other: "{0} q"
    },
    display_name: "q"
  )


  unit_localization(:quarter, "fr", :long,
    nominative: %{
      one: "{0} quartier",
      other: "{0} quartiers"
    },
    display_name: "quartiers"
  )

  unit_localization(:quarter, "fr", :short,
    nominative: %{
      one: "{0} qtr",
      other: "{0} qtrs"
    },
    display_name: "quartiers"
  )

  unit_localization(:quarter, "fr", :narrow,
    nominative: %{
      one: "{0} q",
      other: "{0} q"
    },
    display_name: "q"
  )
end
  1. Unit creation and formatting as normal
iex> MyApp.Cldr.Unit.to_string Cldr.Unit.new!(:quarter, 1)
{:ok, "1 quarter"}
iex> MyApp.Cldr.Unit.to_string Cldr.Unit.new!(:quarter, 2)
{:ok, "2 quarters"}
iex> MyApp.Cldr.Unit.to_string Cldr.Unit.new!(:quarter, 2), locale: "fr"                      
{:ok, "2 quartiers"}

@kipcole9
Copy link
Collaborator

I have published ex_cldr_units version 3.9.2 which includes the fix for this issue.

@DaTrader
Copy link
Author

Great!

@DaTrader
Copy link
Author

Kip,

there's another practical concern I have regarding the narrow Cldr.Unit formats: they can and they do overlap sometimes when parsing and thus we come to another inconsistency i.e. the inability to do a roundtrip such as: unit -> to_string -> parse string -> unit.

I am talking about weeks here e.g. the string output for #Cldr.Unit< :week, 3> is "3w" which is correct, but it requires "3wk" to parse it into weeks and not the watts.

Would it be CLDR compliant to add an optional hint as to which unit category to parse into, eg:

Cldr.Unit.parse!( "3w", category: :duration)

I for one have no need whatsoever to parse watts or most other unit categories, so in my case (as in pretty much most other applications), the one or two unit categories in use are known well upfront. This would also help or even speed up the parsing process so that it spends less time parsing and eventually fails instead of returning false positives given the more limited scope.

My 2c,

Damir

@kipcole9
Copy link
Collaborator

More like 25c I think :-) :-)

I don't think round-trip is necessarily a CLDR design goal, but I think what you ask is possible without compatibility issues. Will think on how best to do this and revert.

@DaTrader
Copy link
Author

You may be right. I should account for the central bank induced inflation. 25c it is then :)

@DaTrader
Copy link
Author

Another thing, what do you suggest how to go about encoding/decoding the units?
Make my own Ecto.Type or simply encode them as JSON arrays of two?
Any library already available for this?

@kipcole9
Copy link
Collaborator

ex_cldr_units_sql should do the trick.

@DaTrader
Copy link
Author

Thanks. Maybe it'd be useful to have this library added to the list on the main ex_cldr documentation page?

@DaTrader
Copy link
Author

Ah, I now see you it in the bottom of the ex_cldr_units page. My bad.

@kipcole9
Copy link
Collaborator

I have published ex_cldr_units version 3.10.0 with the following changelog entry. You can see some examples of the :only and :except usage in the tests and the documentation for Cldr.Unit.parse/2.

Bug Fixes

  • Further refinement to Cldr.Unit.unit_category/1 to return a result in a broader range of cases.

Enhancements

  • Adds :only and :except options to Cldr.Unit.parse/2. These options provide a mechanism to disambiguate the unit when a unit string could refer to more than one unit. For example, "2w" could refer to either "2 weeks" or "2 watts". If neither option is provided then the result is the same as in prior releases: the unit with the lexically shorter and alphabetically earlier unit is returned.

@DaTrader
Copy link
Author

Yes!!! Thanks!

@DaTrader
Copy link
Author

Kip,

just to let you know that upgrading ex_units from 3.9.2 to 3.10.0 results in the following:

iex(4)> Cldr.Unit.parse( "52m", only: :duration)
{:error,
 {Cldr.Unit.AmbiguousUnitError,
  "The string \"m\" ambiguously resolves to [:month, :minute]"}}

Minutes should be probably left as min, and not m :)

My 25c (the inflation!),

Damir

@kipcole9
Copy link
Collaborator

hmmm, tricky. in the CLDR data both minute and month have a "narrow" string of m. And I really don't want to "special case" the data - that's like to be unmaintainable and also drive me mad. Other than saying to the user "keep typing ..." I'm not sure what else to do.

@DaTrader
Copy link
Author

DaTrader commented Jan 1, 2022

First, Happy New Year, Kip!

hmmm, tricky. in the CLDR data both minute and month have a "narrow" string of m. And I really don't want to "special case" the data - that's like to be unmaintainable and also drive me mad. Other than saying to the user "keep typing ..." I'm not sure what else to do.

In that case, the only reasonable option IMO is to allow for expanding on the hint specification i.e.:

Cldr.Unit.parse( "52m", only: [ :day, :week, :month, :year])
This would be my parsing case here for I am only interested in the day+ durations. Also worth noting that in this particular case I don't even support the :quarter duration for a reason (unlike in reporting), so specifying a list of desirable outcomes is the most convenient way to go.

@kipcole9
Copy link
Collaborator

kipcole9 commented Jan 5, 2022

Will add this over the next couple of days, sorry for the delay. I want to provide an option for compile-time validation of the units/categories as well as the current runtime variety.

@DaTrader
Copy link
Author

DaTrader commented Jan 5, 2022

No biggie. I am still on 3.9.2 and there it works with just "m" for months.

@kipcole9
Copy link
Collaborator

kipcole9 commented Jan 5, 2022

I have published ex_cldr_units version 3.11.0 with the following changelog:

Bug Fixes

  • Fix canonical base unit calculation when the unit is a per per form like candela per lux.

Enhancements

  • Add unit filters for Cldr.Unit.parse/2. This means that the options :only and :except can comprise both unit categories and unit names as part of the filter.

Note that the filters are validated at runtime, not compile time. If this becomes a performance issue then I will consider alternative approaches. I think validating the filter arguments is important since otherwise it could lead to hard-to-debug use cases.

@kipcole9
Copy link
Collaborator

kipcole9 commented Jan 5, 2022

Thanks. Maybe it'd be useful to have this library added to the list on the main ex_cldr documentation page?

Its listed here, is there some other place you think it should be listed?

@DaTrader
Copy link
Author

DaTrader commented Jan 6, 2022

No, it's ok. I saw it later in the bottom of the ex_cldr_units page.

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

No branches or pull requests

2 participants