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

CompactDecimal and ScientificDecimal #2847

Merged
merged 12 commits into from
Dec 5, 2022
Merged

Conversation

eggrobin
Copy link
Member

@eggrobin eggrobin commented Nov 29, 2022

Getting back to #1267, where we had decided to add these types.

I have a couple of questions here.

  1. Is this is the right place for these structs? Edit: no it wasn’t, CompactDecimal and ScientificDecimal #2847 (comment).
  2. ScientificDecimal is a lot like FixedDecimal; it has the nice property all of its values can be formatted (e.g., 0012.300e+003 would unambiguously be 0012,300×10⁺⁰⁰³ in fr-FR, which, while silly-looking, is well-defined).
    CompactDecimal is much messier; 1c4 cannot be formatted in en-US, and arguably neither can 1000c6 because of the way CLDR compact decimal data is structured.
    I think it nevertheless makes sense to use the source number as our input here, because it allows users to have their own rounding logic (something for which there is a use case, and which is very tricky to do with ICU4C; when I needed that, I resorted to counting digits in the formatted output!). However this is not directly usable without also providing the locale-dependent mapping from the magnitude of the source number to the power of ten used in compact notation (in en-US, something like 3↦3, 4↦3, 5↦3, 6↦6, etc.).
    I would like to confirm that are aware of that, and that we still want to use CompactDecimal as our source format.

@eggrobin eggrobin requested a review from sffc as a code owner November 29, 2022 15:35
@CLAassistant
Copy link

CLAassistant commented Nov 29, 2022

CLA assistant check
All committers have signed the CLA.

Copy link
Member

@sffc sffc left a comment

Choose a reason for hiding this comment

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

Great start!

utils/fixed_decimal/src/decimal.rs Outdated Show resolved Hide resolved
utils/fixed_decimal/src/decimal.rs Outdated Show resolved Hide resolved
utils/fixed_decimal/src/decimal.rs Outdated Show resolved Hide resolved
utils/fixed_decimal/src/decimal.rs Outdated Show resolved Hide resolved
utils/fixed_decimal/src/decimal.rs Outdated Show resolved Hide resolved
utils/fixed_decimal/src/lib.rs Outdated Show resolved Hide resolved
@eggrobin eggrobin requested a review from sffc December 1, 2022 12:42
utils/fixed_decimal/src/scientific.rs Outdated Show resolved Hide resolved
utils/fixed_decimal/src/integer.rs Outdated Show resolved Hide resolved
utils/fixed_decimal/src/compact.rs Show resolved Hide resolved
utils/fixed_decimal/src/compact.rs Outdated Show resolved Hide resolved
utils/fixed_decimal/src/scientific.rs Show resolved Hide resolved
@eggrobin eggrobin requested a review from sffc December 5, 2022 10:39
sffc
sffc previously approved these changes Dec 5, 2022
utils/fixed_decimal/src/scientific.rs Show resolved Hide resolved
@sffc
Copy link
Member

sffc commented Dec 5, 2022

Please update missing_apis.txt:

+fixed_decimal::CompactDecimal#Struct
+fixed_decimal::CompactDecimal::from_str#FnInStruct
+fixed_decimal::CompactDecimal::write_to#FnInStruct
+fixed_decimal::FixedInteger#Struct
+fixed_decimal::FixedInteger::from_str#FnInStruct
+fixed_decimal::FixedInteger::write_to#FnInStruct
+fixed_decimal::ScientificDecimal#Struct
+fixed_decimal::ScientificDecimal::from#FnInStruct
+fixed_decimal::ScientificDecimal::from_str#FnInStruct
+fixed_decimal::ScientificDecimal::write_to#FnInStruct

Make sure you merge in main first.

@eggrobin eggrobin requested a review from a team as a code owner December 5, 2022 13:05
@eggrobin eggrobin requested a review from sffc December 5, 2022 13:06
@eggrobin eggrobin merged commit 967898f into unicode-org:main Dec 5, 2022
@robertbastian robertbastian mentioned this pull request Jan 23, 2023
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.

3 participants