Skip to content

Update unit conversion to accept unit constant denominator #6199

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

Merged
merged 4 commits into from
Mar 4, 2025

Conversation

younies
Copy link
Member

@younies younies commented Feb 26, 2025

Description

  1. Updates units conversion
    • Removes TODO comment blocking constant denominator unit tests
    • Adds a new from_integer method to IcuRatio
  2. Adds comprehensive tests for unit conversion scenarios including:
    • Conversion between units with constant denominators (e.g., liter-per-100-kilometer to mile-per-gallon)
    • Temperature conversions with offsets (celsius to fahrenheit)

This commit adds comprehensive tests for unit conversion scenarios including:
- Conversion between units with constant denominators (e.g., liter-per-100-kilometer to mile-per-gallon)
- Temperature conversions with offsets (celsius to fahrenheit)
- Adds a new `from_integer` method to `IcuRatio`
- Removes TODO comment blocking constant denominator unit tests
@younies younies requested a review from a team as a code owner February 26, 2025 15:20
@Manishearth
Copy link
Member

Changelog please (all PRs until the beta2 release should contain a changelog entry. In general it is best practice to do this anyway even after a release.)

@Manishearth Manishearth reopened this Feb 26, 2025
@Manishearth
Copy link
Member

beta2 is released, the changelog entry should be moved to the unreleased section (in #6203, approve and merge that first)

@younies younies changed the title Update unit conversion to accept constant denominator Update unit conversion to accept unit constant denominator Mar 3, 2025
@younies younies requested a review from Manishearth March 3, 2025 16:36
Copy link
Member

@Manishearth Manishearth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IcuRation should probably have Mul and Div impls on integers.

@@ -8,6 +8,10 @@
- `icu_collections`
- Remove some panics from `CodePointTrie`, which should no longer pull in panic machinery even with arithmetic panics enabled for lookup (unicode-org#6204)
- Data model and providers
- `icu_experimental`
- `dimension`
- `units`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just under units not dimension/units

@younies younies merged commit 52497b7 into unicode-org:main Mar 4, 2025
28 checks passed
@younies younies deleted the measurement-constants-conversion branch March 4, 2025 16:10
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

Successfully merging this pull request may close these issues.

2 participants