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

Missing/misconfigured units after upgrade #44

Open
gf3 opened this issue May 16, 2024 · 7 comments
Open

Missing/misconfigured units after upgrade #44

gf3 opened this issue May 16, 2024 · 7 comments

Comments

@gf3
Copy link

gf3 commented May 16, 2024

Strange issue and I'm not quite sure where it belongs, but it seems that after the last update of ex_cldr and ex_cldr_units some units have gone missing or become misconfigured. I've pored through the changelogs of ex_cldr_units, ex_cldr, and the actual CLDR 45 release and I haven't found anything that sticks out to me.

It seems that units that are a division or multiplication of :meter are misbehaving (e.g. :millimeter and :kilometer). Possibly this has happened to other units as well and I simply haven't noticed yet.

Example

Before:

MyApp.Cldr.Unit.display_name(:millimeter, style: :long) #=> "millimeters"

After:

MyApp.Cldr.Unit.display_name(:millimeter, style: :long) #=> {:error, {Cldr.Unit.UnitNotTranslatableError, "The unit :millimeter is not translatable"}}

Reproduce

Below is a script to reproduce the issue described above. Things to note:

  1. :millimeter is missing from MyApp.Cldr.Unit.know_units
  2. :millimeter is present in Cldr.Unit.know_units_for_category(:length)
Mix.install([
  {:jason, "~> 1.0"},
  {:ex_cldr, "2.38.1"},
  {:ex_cldr_units, "3.17.0"}
])

defmodule MyApp.Cldr do
  use Cldr,
    locales: ["en",],
    default_locale: "en",
    providers: [Cldr.Number, Cldr.Unit, Cldr.List]
end

IO.puts("known units")
dbg(Enum.sort(MyApp.Cldr.Unit.known_units))

IO.puts("known units contains: millimeters")
dbg(Enum.find(MyApp.Cldr.Unit.known_units, fn unit -> unit == :millimeter end))

IO.puts("units for category: length")
dbg(Cldr.Unit.known_units_for_category(:length))

IO.puts("display name: millimeter")
dbg(MyApp.Cldr.Unit.display_name(:millimeter, style: :long))

Note that you can reproduce the successful output by reverting the dependencies at the beginning of the script to:

Mix.install([
  {:jason, "~> 1.0"},
  {:ex_cldr, "2.37.5"},
  {:ex_cldr_units, "3.16.5"}
])
@kipcole9
Copy link
Collaborator

Sorry for the slow reply.

While there isn't much change in CLDR 45 on the units side, there is quite a bit of change on this library because I was my implementation was making some invalid assumptions. I was correlating translatable units with known units and thats not how CLDR intends. That meant a lot of work under the covers, and all the tests from previous releases pass.

But clearly I have more work to do and I will work on this today and keep this thread updated.

@kipcole9
Copy link
Collaborator

Part of the changes in implementation do relate to prefixes (SI and others). I think it's likely that Cldr.Unit.known_units/0 will only return base units - with prefixes being valid on any unit. I'll review that today as well.

On first review it looks like the bug is in determining what is a translatable unit so hopefully not too hard to pin down.

@gf3
Copy link
Author

gf3 commented May 17, 2024

Thank you for the detailed reply, @kipcole9. Please let me know if I can assist in any way.

@maltoe
Copy link
Contributor

maltoe commented Sep 30, 2024

@kipcole9 Any updates on this one? Anything we can help you with? Also been relying on display_name to shorten + localize unit names, and keeping ex_cldr_units below 3.17 keeps us from upgrading the rest of your libraries now.

@Croge32
Copy link

Croge32 commented Oct 4, 2024

I think we're seeing the same thing, can you confirm if this is the same issue?

iex(15)> Cldr.Unit.new!(:kilogram, 1.1)
Cldr.Unit.new!(:kilogram, "1.1")
iex(16)> Cldr.Unit.new!(:kilometer, 1.1)
Cldr.Unit.new!("kilometer", "1.1")

It seems like some units will remain atoms after new!/1 is called, but some units are reset as strings.

@kipcole9
Copy link
Collaborator

kipcole9 commented Oct 5, 2024

I've not been covering myself in glory on this - and it's my favourite cldr lib too. I made some fundamental errors in understanding the spec when I implemented it, and that's come back to bite me in the last few releases. I have done a lot of work to get things back on track, but not completed.

CLDR 47 is due out in October and I've done preliminary integration and everything is looking ok. Which means I can now get back to this library and get it properlly sorted out. The main challenge now is that I'm travelling with limited internet access. Realistically it means I can't complete the work and update the lib until mid November.

All of the issues noted in this thread (excepting the Math module which needs serious work and will be after fixing this issue) related to that original design error. The API will remain the same - only the underlying unit struct will change a little bit such that all the units and unit parts will be strings. And the process of resolving translations and validating units will change quite a bit to accommodate the additional unit prefixes and the revised localisation process.

I know this issues is dragging on. I will get this sorted and on a more robust foundation for the November release.

@gf3
Copy link
Author

gf3 commented Oct 7, 2024

Thanks for the update @kipcole9. Looking forward to it—please let us know if we can help when it comes to test/release time. Enjoy your travels!

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

4 participants