Mismatch between spec and doctests for DateTime.Format functions

[jarrodmoldrich]

Noticed dialyzer errors around some format functions I was using:

      {:ok, %{medium: format_dt}} = Public.Cldr.DateTime.Format.date_time_formats("en")
      {:ok, %{medium: format_d}} = Public.Cldr.DateTime.Format.date_formats("en")
      {:ok, %{short: format_t}} = Public.Cldr.DateTime.Format.time_formats("en")

Type specs have union type of Cldr.LanguageTag | atom():

@spec date_formats(
  Cldr.Locale.locale_name() | Cldr.LanguageTag.t(),
  Cldr.Calendar.calendar(),
  Cldr.backend()
) :: {:ok, standard_formats()} | {:error, {atom(), String.t()}}

Doctests use string:

Cldr.DateTime.Format.date_formats "en", :gregorian, MyApp.Cldr
{:ok, %Cldr.Date.Styles{
  full: "EEEE, MMMM d, y",
  long: "MMMM d, y",
  medium: "MMM d, y",
  short: "M/d/yy"
}}

Not a huge deal, but thought I'd raise an issue.

[kipcole9]

I have published ex_cldr_dates_times version 2.14.1 with the following changelog entry:

Bug Fixes

• Fix typespecs for various functions that allow a string-based locale identifier. Thanks to @jarrodmoldrich for the report. Closes #39.

[jarrodmoldrich]

Thank you @kipcole9 ! I think Cldr.DateTime.Format.date_time_interval_fallback may also have the same issue. Is it best practice to use something like :en?

[kipcole9]

You are right, I missed that function. Have fixed the @spec and added more example code for dialyzer to analyse. Sorry about missing that (there were a couple of others in the backend module too. I’m on a plane and they block gitthub for some reason so I’ll publish a new bug fix release when I land in a few hours.

Best practise would be to use Cldr.LanguageTag.t types. That’s the canonical format. You can derive that with either Cldr.validate_locale/1 which is used on pretty much every function in the whole ex_cldr domain. It’s inexpensive when the locale is exactly the same as a CLDR locale name because I precompute all the Cldr.LanguageTag.t for those. Therefore it’s just a map lookup. You can also get a Cldr.LanguageTag.t with Sigil_l which is obtained by import Cldr.LanguageTag.Sigil.

I have been back and forward on the typing and function signatures on this for years trying to find a good balance between clear semantics and developer experience. I don’t think I’ve got it quite right and if I ever do a “ex_cldr 3.x` it’s high on the list to formalise better.

In generaal, throughout the system, I use atoms where the cardinality is fixed. That’s true for CLDR locale names, hence atom is preferred over a string. And it’s more efficient because the atom is directly looked up in the precomputed map. A string is first converted to an existing atom (if possible) and then looked up.

Feedback on this topic is always welcome.

On 23 Sep 2023, at 4:10 pm, Jarrod Moldrich @.***> wrote:

Thank you @kipcole9 https://github.com/kipcole9 ! I think Cldr.DateTime.Format.date_time_interval_fallback may also have the same issue. Is it best practice to use something like :en?

— Reply to this email directly, view it on GitHub https://github.com/elixir-cldr/cldr_dates_times/issues/39#issuecomment-1732429205, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAD4F7QHZMGLHRM22DSZATX35T4TANCNFSM6AAAAAA5EF66ZU. You are receiving this because you were mentioned.

[jarrodmoldrich]

Thanks for all the detail. I think I understand the types much better. As for feedback, my preferences with Elixir libraries fall with correctness, ergonomics, and then performance, in that order. Sometimes the last two swap around for performance critical functions, and time conversion, parsing, and formatting, may count.

If the functions can handle LanguageTag.t or atom() or string() then it doesn't seem to be a problem. If there's some significant performance benefit to removing the translation step, or if handling errors from the translation step causes complexity to splinter across the codebase, maybe in 3.x you could remove the union with atom() or string() and leave it to the caller to either explicitly translate them to a canonical language tag. That way they can handle the errors as a separate concern, and redundant look-ups would be avoided. It might irk some to have the extra line for every request, though some others might like that they use a canonical verified locale type around their code instead of unvalidated strings.

[jarrodmoldrich]

Only other suggestion I'd make is a validate_locale! function. I want to set a module attribute with the result without having to handle :ok/:error

[kipcole9]

Good call and it’s long overdue.  I’m in the dev cycle for CLDR 44 that will be out in October and I’ll add it for that if not before.Sent from my iPhoneOn 23 Sep 2023, at 22:30, Jarrod Moldrich @.***> wrote: Only other suggestion I'd make is a validate_locale! function. I want to set a module attribute with the result without having to handle :ok/:error

—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: @.***>