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

Inspect implementation of %Cldr.LanguageTag{} is unhelpful and doesn't align with the docs #234

Open
nietaki opened this issue Jul 1, 2024 · 4 comments

Comments

@nietaki
Copy link

nietaki commented Jul 1, 2024

I used ex_cldr before, which is why I wanted to use it to fetch some information for a number of locales.

When I add it to a project (a real one or a repro one), most of the functions I run return MyApp.Cldr.Locale.new!("en-US") for whatever reason.

$ iex -S mix
Erlang/OTP 25 [erts-13.2] [source] [64-bit] [smp:10:10] [ds:10:10:10] [async-threads:1] [jit]

Compiling 1 file (.ex)
Generating Repro.Cldr for 97 locales named [:af, :am, :ar, :as, :az, ...] with a default locale named :en
Compiling lib/repro/cldr.ex (it's taking more than 10s)
Interactive Elixir (1.14.4) - press Ctrl+C to exit (type h() ENTER for help)
iex(1)> Repro.Cldr.validate_locale "en"
{:ok, Repro.Cldr.Locale.new!("en-US")}
iex(2)> Repro.Cldr.default_locale()
Repro.Cldr.Locale.new!("en-US")
iex(3)> Repro.Cldr.Locale.new!("en-US")
Repro.Cldr.Locale.new!("en-US")
iex(4)> Repro.Cldr.Locale.new!(:fr)
Repro.Cldr.Locale.new!("fr-FR")
iex(5)> Enum.count(Repro.Cldr.known_locale_names())
96
iex(6)> Enum.filter(Repro.Cldr.known_locale_names(), fn ln -> String.contains?("#{ln}", "en") end)
[:en]
iex(7)> Repro.Cldr.Locale.new!(:en)
Repro.Cldr.Locale.new!("en-US")

I created a repo with the repro. I tried to keep the configuration minimal:

defmodule Repro.Cldr do
  use Cldr,
    otp_app: :repro,
    default_locale: "en",
    locales:
    ["ja", "br", "be", "hr", "da", "hu", "oc", "ms", "sn", "sk", "sd", "ka", "no", "pa", "yi", "ur",
 "ro", "fi", "el", "es", "hy", "sr", "pt", "kk", "ha", "kn", "eu", "haw", "af", "sa",
 "et", "ln", "bg", "si", "ml", "ne", "fa", "my", "fo", "th", "sq", "pl", "en", "ar", "ca", "bo",
 "cs", "mk", "tg", "it", "as", "tt", "sl", "lv", "gl", "nl", "ko", "cy", "sw", "ba", "sv", "mt",
 "te", "tr", "mg", "lt", "he", "id", "lo", "is", "fr", "am", "vi", "so", "la", "km", "su", "de",
 "bs", "lb", "az", "gu", "zh", "uz", "ta", "tk", "ps", "uk", "mn", "nn", "bn", "mi",
 "ru", "hi", "mr", "yo"],
    providers: []
end
import Config

config :ex_cldr,
  default_backend: Repro.Cldr,
  json_library: Jason

I re-read the installation/configuration docs a couple of times and fiddled with the config and I'm still getting the same results.

Any advice? I assume (hope) this isn't the expected behaviour.

Edit: I updated the issue title to be actionable.

@kipcole9
Copy link
Collaborator

kipcole9 commented Jul 1, 2024

iex calls inspect/2 on the result of any function and that's what is displays in the terminal. As of a ex_cldr 2.38.0 on April 21st, the Inspect protocol implementation for Cldr.LanguageTag.t changed to output executable code when the locale has been resolved to a known/configured locale. So while you see MyApp.Cldr.Locale.new!("en-US") in the terminal, the result is actually a struct.

I made this change to be consistent with how Inspect implementations are evolving in the Elixir code base - more towards inspect producing executable code in more situations.

Does that clear up what you're seeing?

@nietaki
Copy link
Author

nietaki commented Jul 1, 2024

It seems like an astounding anti-pattern for a number of reasons:

  • It makes it very inconvenient to use in any scripting context. What's the alternative to see the struct at a glance? Repro.Cldr.Locale.new!("en-US") |> Map.from_struct()? Knowing the stuct by heart and just fetching its fields blindly?
  • Is the intention to encourage people to evaluate code copied from logs? That can't be a good practice.
  • That breaks the encapsulation / separation of concerns / single responsibility principle - if the responsibility of Cldr.LanguageTag.t was to keep just the data, it wouldn't be able to return the function description, because it wouldn't have a reference to the backend used to create it.
  • It makes the inspect() itself not serve its purpose - I'm not inspecting the terms contents, just a random way to create it.
  • The docs for all ex_cldr versions 2.38.0 and newer make it seem like it still works normally:
Cldr.Locale.new("en-ES", TestBackend.Cldr)
{:ok, %Cldr.LanguageTag{
  backend: TestBackend.Cldr,
  canonical_locale_name: "en-ES",
  # (lines cut)
  script: :Latn,
  territory: :ES,
  transform: %{},
  language_variants: []
}}

I hoped that worst case scenario this was a ham-handed way of ex_cldr to let me know I should be using the Myapp.Cldr module instead of the Cldr module, so kept on re-checking my config. I'd check the source of Myapp.Cldr to see what's going on, but I skipped that, because it's macro-generated.

I made this change to be consistent with how Inspect implementations are evolving in the Elixir code base - more towards inspect producing executable code in more situations.

Do they really? All of the examples I can find right now have the Inspect protocol implementation return all the relevant/usable information about the underlying term:

iex(7)> ~r/(foo|bar)/u
~r/(foo|bar)/u
iex(8)> spawn fn -> 1 end
#PID<0.110.0>
iex(9)> pid(0, 110, 0)
#PID<0.110.0>
iex(10)> MapSet.new([:foo, :bar, :bar])
MapSet.new([:foo, :bar])
iex(11)> MapSet.new([:foo, :bar])
MapSet.new([:foo, :bar])
iex(12)> URI.new!("https://example.com/en/?foo=bar#header")
%URI{
  scheme: "https",
  userinfo: nil,
  host: "example.com",
  port: 443,
  path: "/en/",
  query: "foo=bar",
  fragment: "header"
}
iex(13)> Repo.get!(PersonGroup, "b9a57dde-52b9-4e71-b300-98d6433b9a79")
%Foo.Models.PersonGroup{
  __meta__: #Ecto.Schema.Metadata<:loaded, "persons_groups">,
  id: "b9a57dde-52b9-4e71-b300-98d6433b9a79",
  person_id: "85d52529-49af-4662-b432-9eec82254adf",
  person: #Ecto.Association.NotLoaded<association :person is not loaded>,
  group_id: "9092bad9-f64a-4bd7-951d-6e38fbdbcbea",
  group: #Ecto.Association.NotLoaded<association :group is not loaded>,
  inserted_at: ~U[2024-03-14 16:57:47.121775Z]
}

Yes, I don't see the whole structure of MapSet or Ecto.Association.NotLoaded, but that's because it's very difficult for me to come up with a reson why I would care about them.

What you did, however, is the equivalent of this:

iex(12)> URI.new!("https://example.com/en/?foo=bar#header")
URI.new!("https://example.com/en/?foo=bar#header")

iex(13)> Repo.get!(PersonGroup, "b9a57dde-52b9-4e71-b300-98d6433b9a79")
Repo.get!(PersonGroup, "b9a57dde-52b9-4e71-b300-98d6433b9a79")

Which in my opinion is incredibly pointless and unhelpful.

Also, please don't get me wrong, it's not like I wasn't aware of the Inspect protocol - it's just that in my decade of working with Elixir I wouldn't come up with someone using it this way. It might sound like I'm annoyed by this situation, and that's only because it's true - this "feature" cost me over an hour of my life, debugging an issue that's not there.


So the answer does clear up what I'm seeing, but makes the intentions before the decision to make it this way even more bizarre.

@nietaki nietaki changed the title Most functions return MyApp.Cldr.Locale.new!("en-US") Inspect implementation of %Cldr.LanguageTag{} is unhelpful and doesn't align with the docs Jul 1, 2024
@kipcole9
Copy link
Collaborator

kipcole9 commented Jul 3, 2024

I have posted in Elixir Forum on this topic to see what community and Elixir core team views are on this.

@pejrich
Copy link

pejrich commented Aug 7, 2024

To say you’ve been working with Elixir for a decade but have never seen this pattern is just unbelievable, it’s everywhere.

Elixir uses Inspect for Range 1..5 rather than showing the struct. DateTime, Date, Time, MapSet use Inspect.

Decimal inspects to Decimal.new("10.0") rather than the full struct.

Ecto makes heavy use of Inspect for nearly everything. Changeset, Query, Schema, NotLoaded, and more

Absinthe uses Inspect in this way too, heavily(though often with the older #.. form)

Most people don’t want their logs and iex sessions to be 95% full of 3rd party libraries struct guts that they don’t care about. I don’t care about the internals of the Ecto.Query struct most of the time. If I happen to need to see them, then I can always convert to a map. In the case of this library, I agree with @kipcole9 , most people aren’t needing to see the internals of this struct 99% of the time, and so showing it is just unwanted noise. A snippet of code that executes and generates the same struct is incredibly helpful in the event you want to copy paste some other data structure into iex that happens to contain this library's struct, it’s helpful that it just works and doesn’t throw an error.

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

3 participants