Refactor: Move `MeasureUnit` to a Separate Crate

younies commented 1 month ago

Task

Refactor the codebase by moving the MeasureUnit functionality into a dedicated crate.

Background

The MeasureUnit module is responsible for representing single or compound CLDR units (e.g., meter, square-meter, kilometer-per-second). This module is essential for various components, including unit formatting, unit conversion, and locale-aware unit representation.

To improve modularity and reusability across the project, it is necessary to isolate MeasureUnit into its own crate. This will facilitate better code organization and allow independent development and maintenance of unit-related functionalities.

younies commented 1 month ago

@younies - The crate represents units, including combined units. It can process "square-meter" and break it down into parts. It can also get the CLDR identifier for the unit. Currently I'm calling it "measure", but I'm not sure this is a good name.
@sffc - It's similar to icu_calendar and icu_timezone and fixed_decimal. Not clear if it is a component or a utility.
@younies - It has its own data provider, the trie for parsing the unit string.
@sffc - I recall that previously we discussed having icu_units and icu_dimension. https://docs.google.com/document/d/1AftqfMIFjgQAKpIfO8pmYMZW-KIOBtYkVHZuwgYcfsw/edit
@sffc - We previously decided that the crate icu_units would contain both the units conversion and the units structs. So we should first decide whether we want to split these crates.

A few ways we could split these crates:

icu_units (core data types), icu_units_conversion (conversion)
icu_units (conversion and re-exports), icu_units_core (core data types). Could be extended to also split out icu_units_conversion
icu_foo (core data types) and icu_bar (conversion)
icu_units (re-exports), icu_units_parser (core data types), icu_units_conversion (conversion)

Discussion:

@younies - We could have icu_measure (core data types) and icu_units (conversion)... which is a little confusing
@sffc - That's basically my option 3.
@sffc - Option 2 is consitent with what we did with icu_locale.
@younies - A problem with option 2 is that most users will just want the data types. They don't need the conversion utilities.
@echeran - There are users who don't need to optimize dependencies, etc. They want a top-level crate that has everything they need.
@younies - The core data types do parsing, etc. It is its own use case.
@sffc - The main downside of option 3 is that you need to invent new namespaces.
@younies - Maybe option 4, since conversion is a narrow use case, so it should have a narrow crate.
@sffc - Option 2 could be extended to have another child crate later. I prefer starting with fewer crates when possible.
@younies - I already started the work to create the icu_measure crate. I'd like to finish doing that and fix it up to the agreement later.
@younies Does the data provider get split when we split out code from a crate into a smaller related crate?
@sffc Yes. icu_units_core vs. icu_measure is basically the same.
@younies In order to migrate icu_units_core, do I also need to graduate the conversion?
@younies - Can we name it icu_units_representation instead of icu_units_core?
@sffc - We can add that to the bikeshed ballot. I think icu_units_core is best because it is consistent with icu_locale_core which the module structure is modeled after.

Proposal:

Adopt option 2 above
OK for Younies to make icu_measure and then rename it to icu_units_bikeshed after we decide the name.
icu_units_bikeshed can be graduated independently from icu_units

LGTM: @younies @echeran @sffc

sffc commented 4 weeks ago

Based on the ballot, the winning crate name is: icu_units_core

unicode-org / icu4x

Refactor: Move `MeasureUnit` to a Separate Crate #5335

Task

Background