elixir-cldr / cldr

Elixir implementation of CLDR/ICU
Other
447 stars 33 forks source link

Locale Name String => Atom Breaking Change #166

Closed maennchen closed 2 years ago

maennchen commented 2 years ago

The change leads to problems with cldr_languages.

Error

105) test Index lists all affiliations (HygeiaWeb.AffiliationLiveTest)
     test/hygeia_web/live/affiliation_live_test.exs:28
     ** (BadMapError) expected a map, got: {:error, {Cldr.UnknownLocaleError, "The locale \"en\" is not known."}}
     stacktrace:
       (stdlib 3.17) :maps.find("en", {:error, {Cldr.UnknownLocaleError, "The locale \"en\" is not known."}})
       (hygeia 1.44.0) lib/cldr/language.ex:169: HygeiaCldr.Language.to_string_by_locale/3
       (hygeia 1.44.0) lib/cldr/language.ex:159: HygeiaCldr.Language.to_string/2
       (hygeia 1.44.0) lib/hygeia_web/live/header.sface:185: anonymous fn/4 in HygeiaWeb.Header."render (overridable 1)"/1
       (phoenix_live_view 0.17.7) lib/phoenix_live_view/diff.ex:387: Phoenix.LiveView.Diff.traverse/7
       (phoenix_live_view 0.17.7) lib/phoenix_live_view/diff.ex:494: anonymous fn/4 in Phoenix.LiveView.Diff.traverse_dynamic/7
       (elixir 1.13.3) lib/enum.ex:2396: Enum."-reduce/3-lists^foldl/2-0-"/3
       (phoenix_live_view 0.17.7) lib/phoenix_live_view/diff.ex:387: Phoenix.LiveView.Diff.traverse/7
       (phoenix_live_view 0.17.7) lib/phoenix_live_view/diff.ex:494: anonymous fn/4 in Phoenix.LiveView.Diff.traverse_dynamic/7
       (elixir 1.13.3) lib/enum.ex:2396: Enum."-reduce/3-lists^foldl/2-0-"/3
       (phoenix_live_view 0.17.7) lib/phoenix_live_view/diff.ex:387: Phoenix.LiveView.Diff.traverse/7
       (phoenix_live_view 0.17.7) lib/phoenix_live_view/diff.ex:664: Phoenix.LiveView.Diff.render_component/9
       (phoenix_live_view 0.17.7) lib/phoenix_live_view/diff.ex:609: anonymous fn/5 in Phoenix.LiveView.Diff.render_pending_components/6
       (elixir 1.13.3) lib/enum.ex:2396: Enum."-reduce/3-lists^foldl/2-0-"/3
       (stdlib 3.17) maps.erl:410: :maps.fold_1/3
       (phoenix_live_view 0.17.7) lib/phoenix_live_view/diff.ex:582: Phoenix.LiveView.Diff.render_pending_components/6
       (phoenix_live_view 0.17.7) lib/phoenix_live_view/diff.ex:145: Phoenix.LiveView.Diff.render/3
       (phoenix_live_view 0.17.7) lib/phoenix_live_view/static.ex:244: Phoenix.LiveView.Static.to_rendered_content_tag/4
       (phoenix_live_view 0.17.7) lib/phoenix_live_view/static.ex:126: Phoenix.LiveView.Static.render/3
       (phoenix_live_view 0.17.7) lib/phoenix_live_view/controller.ex:39: Phoenix.LiveView.Controller.live_render/3

The error can be observed here: https://github.com/jshmrtn/hygeia/pull/1247 / https://github.com/jshmrtn/hygeia/runs/5272966546?check_suite_focus=true

Versions

$ mix deps | grep cldr
* cldr_utils 2.17.1 (Hex package) (mix)
  locked at 2.17.1 (cldr_utils) 052e0c2c
* ex_cldr 2.26.1 (Hex package) (mix)
  locked at 2.26.1 (ex_cldr) b666dd85
* ex_cldr_calendars 1.18.0 (Hex package) (mix)
  locked at 1.18.0 (ex_cldr_calendars) 5b47bf4e
* ex_cldr_currencies 2.13.0 (Hex package) (mix)
  locked at 2.13.0 (ex_cldr_currencies) 64731e49
* ex_cldr_dates_times 2.11.0 (Hex package) (mix)
  locked at 2.11.0 (ex_cldr_dates_times) 36b2dd6b
* ex_cldr_languages 0.3.1 (Hex package) (mix)
  locked at 0.3.1 (ex_cldr_languages) 9d9341bd
* ex_cldr_lists 2.10.0 (Hex package) (mix)
  locked at 2.10.0 (ex_cldr_lists) adc040cd
* ex_cldr_messages 0.13.2 (Hex package) (mix)
  locked at 0.13.2 (ex_cldr_messages) 8c722bdc
* ex_cldr_numbers 2.25.0 (Hex package) (mix)
  locked at 2.25.0 (ex_cldr_numbers) 0ffe6648
* ex_cldr_units 3.12.0 (Hex package) (mix)
  locked at 3.12.0 (ex_cldr_units) 2a252abb
$ elixir --version
Erlang/OTP 24 [erts-12.2.1] [source] [64-bit] [smp:8:8] [ds:8:8:10] [async-threads:1] [jit]

Elixir 1.13.3 (compiled with Erlang/OTP 24)

Triggering Code

https://github.com/jshmrtn/hygeia/blob/4d929ef882f6f81e720b1791279e3ed01143648a/lib/hygeia_web/live/header.sface#L184

HygeiaCldr.get_locale().language()
|> (&HygeiaCldr.Language.to_string(&1, locale: &1)).()
|> elem(1)
alaadahmed commented 2 years ago

after updating deps, got this also =>

The call 'Elixir.Cldr.DateTime.Formatter':literal
         (_@2 :: #{'hour' := _, 'minute' := _, _ => _},
          <<45>>,
          _@1 ::
              #{'__struct__' := 'Elixir.Cldr.LanguageTag',
                'cldr_locale_name' := 'und',
                _ => _},
          'Elixir.AhramSchoolWeb.Cldr',
          _@5 :: any()) breaks the contract 
          (any(),
          'Elixir.String':t(),
          locale(),
          'Elixir.Cldr':backend(),
          map()) ->
             'Elixir.String':t()
kipcole9 commented 2 years ago

@maennchen sorry about this, I think the issue relates to ex_cldr_languages. I have just now sent a PR to @LostKobrakai to fix the compatibility issue. Hopefully with his agreement a new version can be released soon.

@alaadahmed Can you please open an issue on ex_cldr_dates_times with an example of how that error is being returned? It looks like a dialyzer error?

alaadahmed commented 2 years ago

yes it is dialyzer errors, 6 to be specific. That's why I didn't open issue.

Screen Shot 2022-02-21 at 21 05 10

.

kipcole9 commented 2 years ago

Closing this issue since both reports are for different libraries (ex_cldr_languages and ex_cldr_dates_times). Both issues are being worked on.

LostKobrakai commented 2 years ago

@kipcole9 @maennchen I just released a new version of ex_cldr_languages. I want to make sure that while the string -> atom locale switch caused the error mentioned in the issue, it was ultimately caused by incorrect usage:

HygeiaCldr.get_locale().language()
|> (&HygeiaCldr.Language.to_string(&1, locale: &1)).()
|> elem(1)

Using the language as the locale only worked by accident given both languages and locales were identified by strings. Locales changes to be identified by atoms, languages are still strings, so the code should be like the following:

locale = HygeiaCldr.get_locale()
{:ok, lang} = HygeiaCldr.Language.to_string(locale.language, locale: locale)
lang

What should've worked before and was fixed in this release is that the first parameter could also be a LanguageTag. So this works as well:

language_tag = HygeiaCldr.get_locale()
{:ok, lang} = HygeiaCldr.Language.to_string(language_tag, locale: language_tag)
lang
maennchen commented 2 years ago

@LostKobrakai Thanks for the update :)

@kipcole9 It seems that various user facing things changed:

https://github.com/jshmrtn/hygeia/runs/5389221417?check_suite_focus=true

Is there a function where I can provide a locale name as a string and if it exists, I get either the atom or the language tag back?

kipcole9 commented 2 years ago

@maennchen, the canonical approach is to use MyApp.Cldr.validate_locale/1. This takes an atom, string or Cldr.LanguageTag.t and returns the Cldr.LanguageTag.t. The language tag element :cldr_locale_name then holds the CLDR locale being used.

It was not my intention to introduce breaking changes (except for ex_cldr libraries, which have been updated) so I will check your link and see where I can improve.

maennchen commented 2 years ago

@kipcole9 I did not know about validate_locale, that mostly did the trick.

I'm left with only one dialyzer error:

lib/hygeia_cldr.ex:1:invalid_contract
The @spec for the function does not match the success typing of the function.

Function:
HygeiaCldr.Language.to_string_by_locale/3

Success typing:
@spec to_string_by_locale(_, _, %{:fallback => boolean(), :locale => _, :style => :short | :standard}) ::
  :error | {:error, {atom(), binary()}} | {:ok, _}
LostKobrakai commented 2 years ago

@maennchen I just published another version, which should fix the dialyzer error.