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

Bikeshed the linear converter name #4576

Closed
younies opened this issue Feb 2, 2024 · 0 comments · Fixed by #4605
Closed

Bikeshed the linear converter name #4576

younies opened this issue Feb 2, 2024 · 0 comments · Fixed by #4605
Labels
C-numbers Component: Numbers, units, currencies discuss Discuss at a future ICU4X-SC meeting

Comments

@younies
Copy link
Member

younies commented Feb 2, 2024

Currently, we are using the LinearConverter to convert non-offset units.

However, @robertbastian has expressed opposition to this approach in his comment: https://github.com/unicode-org/icu4x/pull/4404/files#r1474932838.

by saying:
I believe "linear" is not the correct term. Celsius and Fahrenheit are linearly related, but explicitly not supported here, and linear is also not used for what you call "reciprocal" relationships. I think "proportional" is a more precise term, as that is by definition offset-free, and "inverse proportional" is a correct term for the reciprocal relationships.

Therefore, we need to discuss the name of this class.

@sffc sffc added discuss Discuss at a future ICU4X-SC meeting C-numbers Component: Numbers, units, currencies labels Feb 2, 2024
@robertbastian robertbastian changed the title BikeShade the linear converter name Bikeshed the linear converter name Feb 5, 2024
robertbastian pushed a commit that referenced this issue Feb 28, 2024
This PR introduces the `UnitsConverter`, a `struct` that wraps three
distinct types of unit converters:

1. **ProportionalConverter**: Handles conversions between units where
the relationship is `unit1 = CR * unit2`.
2. **ReciprocalConverter**: Handles conversions between units where the
relationship is `unit1 = 1 / (CR * unit2)`.
3. **OffsetConverter**: Handles conversions with an offset, following
the formula `unit1 = CR * unit2 + Offset`.

_Note: CR refers to the Conversion Rate._

Additionally, this PR includes an organizational change:
- The `ConverterFactory` has been relocated to a separate folder,
enhancing the overall clarity and structure of the implementation.


Fixes: #4576
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-numbers Component: Numbers, units, currencies discuss Discuss at a future ICU4X-SC meeting
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants