elixir-cldr / cldr_dates_times

Date & times formatting functions for the Common Locale Data Repository (CLDR) package https://github.com/elixir-cldr/cldr
Other
69 stars 13 forks source link

Date Interval formatting crossing a year boundary #40

Closed matt-glover closed 1 year ago

matt-glover commented 1 year ago

Hi,

First off thanks for the work on this great library!

I have some code to display date intervals using this library and leveraging the short format with the month_and_day style. I discovered that this returns an error whenever the date range crosses a year boundary. For example:

iex(1)> Cldr.Date.Interval.to_string(~D[2023-12-31], ~D[2024-01-02], MyApp.Cldr, [format: :short, style: :month_and_day])
{:error,
 {Cldr.DateTime.UnresolvedFormat,
  "The interval format :y is invalid. Valid formats are [:short, :long, :medium] or an interval format string."}}

I can see why/how this triggers (explained below). But my core question is whether I should treat this as a special case and handle it myself using a different format/custom logic or if I should consider this a bug somewhere in the Elixir CLDR stack and try to issue a patch.

I dug into the formatting rules and saw that short format and month_and_day style give me Md. In the default "en" locale with a default Gregorian calendar I get Md: %{d: ["M/d – ", "M/d"], M: ["M/d – ", "M/d"]},. The greatest difference check for date intervals spits out y (year) as the difference here and then obviously lacking a y option in the Md map it tells me the interval format is invalid.

I hoped to stick with the shortcut options like short and long to get support out-of-the-box for various locales. Seems like that might work okay here if I ditched the style parameter?

I'm a little out of my depth so I paused here before identifying whether the Md mapping is from the Unicode CLDR project itself vs something in one of the suite of Elixir libraries involved. Any advice is appreciated.

kipcole9 commented 1 year ago

Matt, thanks for the kind words. Time is hard! I understand what you're trying to do. Let me think on it a bit today (UTC+11) and see what can be done.

kipcole9 commented 1 year ago

Matt, I've pushed a commit which I believe does what you expect. Already the code will fallback to "greatest difference :year" from the greatest difference month and day if those formats don't exist. So it's reasonable that greatest difference :year should also try month and day formats if a year format doesn't exist.

Would you considering configuring ex_cldr_dates_times from GitHub and let me know if it works for your requirements?

With this commit the code now does the following:

  defp greatest_difference_format(format, requested_format, requested_style, :y = difference) do
    resolved_format = Map.get(format, difference) || Map.get(format, :M) || Map.get(format, :d) || :error

    case resolved_format do
      :error -> {:error, unavailable_format_error(requested_format, requested_style, difference)}
      success -> {:ok, success}
    end
  end

I also improved the error message if there is no resolvable format (it wasn't a very good one before).

Lastly, I did a proof read of the various Interval modules to make them a bit better.

Let me know if this now works as you expect - and of course re-open if not. I will publish to Hex an update when you say it's ok for you.

matt-glover commented 1 year ago

I’ll take a look Monday (UTC-4) when I get in to work to be sure. But glancing at the PR I’m pretty sure the change is exactly what I need.

matt-glover commented 1 year ago

That change is producing the expected output for me. I had to update various other CLDR dependencies and I'm getting plenty of other test failures I have to work through where we asserted on CLDR output already. But I get those failures with the latest ex_cldr* packages including the latest released version of this package. I suspect my issues are tied to CLDR 43 which we weren't using yet. Seeing a mix of no longer valid formats + changes around the type of space or hyphen/dash/etc being used to delimit things. So I'll be debugging those before I can cut over to this latest version. But the fix looks good to me.

Thanks for all the help!

kipcole9 commented 1 year ago

Yes, CLDR 43 did change quite a bit of white space in formats. A pain for updating tests I agree, but a better longer-term outcome.

I've published ex_cldr_dates_times version 2.14.3 with the following changelog entry:

Bug Fixes