Improved big-number formatting #346

oscbyspro · 2023-12-11T13:38:55Z

Howdy! I've made some changes to improve your big-number formatting.

Changes

Removed trapping conversion to Int in BinaryInteger/formatted().
- IntegerFormatStyle can format big integers since (#262).
Removed rounding conversion to Double in BinaryFloatingPoint/formatted()
- This method now does whatever FloatingPointFormatStyle/format(_:) does.
Reenabled "Decimal Tests" in NumberFormatStyleTests.swift.
- The normal styles worked as-is, but the attributed one needed some love.
Added a "numeric string representation" case to ICUNumberFormatterBase.Value.
- IntegerFormatStyle.Attributed now uses this value rather than Int64(clamping:).
Removed conversions to Decimal in each integer format style (#186).
- IntegerFormatStyle, [...].Attributed, [...].Currency, [...].Percent
Removed isZero and doubleValue from ICUNumberFormatterBase.Value.
- This makes it easier to accommodate non-numeric payloads, e.g. String.
- Both were used in ByteCountFormatStyle, which now queries FormatInput.
Added internal _format(_:doubleValue:) method to ByteCountFormatStyle.
- Parameterized function body for use with values convertible to Double.
- Prior method was removed because it relied on [...].Value/doubleValue.

Comments

[RESOLVED] I was not sure what to do with ICUNumberFormatterBase.Value's isZero and doubleValue – please share your thoughts. I solved it ™️ by force unwrapping Double.init(_:). The force unwrap should never fail, assuming the numeric string representation is well-formed, but large values may result in ± Double.infinity. It is not a problem for isZero, but I do not know the semantics of doubleValue.

Alternatives

[OUTDATED] You can add some thing like fallbackInteger(any BinaryInteger) instead of numericStringRepresentation(String). Perhaps it makes sense if you don't want to check isZero through Double.

- [IntegerFormatStyle] I removed the trapping conversion to `Int`. IntegerFormatStyle can format big integers since (swiftlang#262). - [FloatingPointFormatStyle] I removed the rounding conversion to `Double`. `formatted()` now does whatever `format(_:)` does. - [Decimal.FormatStyle] N/A (there were no conversions here).

- IntegerFormatStyle big integer tests succeed. - IntegerFormatStyle.Attributed big integer tests fail (output is clamped to `Int64`).

- Added a numeric string representation case to ICUNumberFormatter.Value. - IntegerFormatStyle.Attributed now uses the above instead of `Int64(clamping:)`.

…g#186). BinaryInteger's numeric string representation supersedes Decimal in the following cases: 1. IntegerFormatStyle. 2. integerFormatStyle.Attributed. 3. IntegerFormatStyle.Currency. 4. IntegerFormatStyle.Percent.

The numeric string format permits redundant zeros (like `+00.00`).

oscbyspro · 2023-12-12T06:44:53Z

Just to be clear: this pull request is a smörgåsbord. I wanted to fix the trapping conversion (and I suggest you keep that part). But, after that, I reenabled some of your tests and looked for ways to fix IntegerFormatStyle.Attributed's clamping behavior. Adding a new case to ICUNumberFormatterBase.Value is one way, but I don't know if that's what you want. The numeric string representation is perhaps not something you want to query. But, since I have no way of knowing your plans for IntegerFormatStyle.Attributed, I thought I would leave it up for review.

oscbyspro · 2023-12-12T09:17:28Z

I don't want to bombard this thread. But after looking more closely at it, I see that isZero and doubleValue are only used in ByteCountFormatStyle. So you could perhaps remove isZero and doubleValue from ICUNumberFormatterBase.Value and get those from FormatInput instead. In that case, [...].Value could contain anything convertible to ICUNumberFormatterBase.FormatResult. It's one option that circumvents the current awkwardness of using a String payload in [...].Value.

Edit: here's a separate commit with this approach (+5, -37).

Both `isZero` and `doubleValue` were used in `ByteCountFormatStyle`. These values are now taken from `FormatInput` (`Int64`) instead. Removing them from `ICUNumberFormatter.Value` makes it easier to accommodate non-numeric payloads such as strings, which can be used to format arbitrary precision numbers.

itingliu · 2023-12-13T01:32:35Z

I don't want to bombard this thread. But after looking more closely at it, I see that isZero and doubleValue are only used in ByteCountFormatStyle. So you could perhaps remove isZero and doubleValue from ICUNumberFormatterBase.Value and get those from FormatInput instead. In that case, [...].Value could contain anything convertible to ICUNumberFormatterBase.FormatResult. It's one option that circumvents the current awkwardness of using a String payload in [...].Value.

Edit: here's a separate commit with this approach (+5, -37).

Thank you! I think this is indeed the most reasonable approach. I'd be ok either if you include that as part of this change or a separate PR. No bombarding at all either

Sources/FoundationInternationalization/Formatting/Number/ICUNumberFormatter.swift

oscbyspro · 2023-12-13T09:34:00Z

Thank you! I think this is indeed the most reasonable approach. I'd be ok either if you include that as part of this change or a separate PR. No bombarding at all either

Cool. I've added f065a2d to this pull request.

itingliu

@swift-ci please test

itingliu

LGTM! Thanks!

itingliu · 2023-12-15T21:40:01Z

@swift-ci please test

Sources/FoundationInternationalization/Formatting/ByteCountFormatStyle.swift

realized there's one pending point. Dismissing the review so we don't miss it

Here's an internal method simliar to the `_format(_:)` method removed earlier because  wants its method back! I removed the first method to accommodate `ICUNumberFormatter.Value` cases that cannot implement `doubleValue`. The new method parameterizes the conversion instead, so you can call it whenever conversions to `Double` are possible.

oscbyspro · 2023-12-16T12:11:01Z

Here's a solution that lets you use Double in ByteCountFormatStyle:

_format(.integer(int64), doubleValue: Double(int64))
_format(.floatingPoint(double), doubleValue: double)

If you think it's a reasonable approach, I'll add it to this pull request.

oscbyspro · 2023-12-17T06:09:56Z

Hm. I suppose I'll add the proposed solution and amend it if needed. I think that might be easier.

parkera

I'll let @itingliu have the final say here but this looks good to me.

itingliu

LGTM!

itingliu · 2023-12-19T02:04:02Z

@swift-ci please test

itingliu · 2023-12-21T01:23:04Z

CI is down due to a regression in recent toolchain. That should be fixed in swiftlang/swift#70559. I'll rerun the test after CI updates to a newer toolchain containing the fix

itingliu · 2023-12-21T23:26:28Z

@swift-ci please test

oscbyspro added 5 commits December 11, 2023 09:49

Reenabled "Decimal Tests" in NumberFormatStyleTests.swift.

4e21bcf

- IntegerFormatStyle big integer tests succeed. - IntegerFormatStyle.Attributed big integer tests fail (output is clamped to `Int64`).

Fixes IntegerFormatStyle.Attributed.

2d39560

- Added a numeric string representation case to ICUNumberFormatter.Value. - IntegerFormatStyle.Attributed now uses the above instead of `Int64(clamping:)`.

Check whether numeric string is zero using Double.

b48417e

The numeric string format permits redundant zeros (like `+00.00`).

itingliu self-requested a review December 11, 2023 22:06

itingliu reviewed Dec 13, 2023

View reviewed changes

Sources/FoundationInternationalization/Formatting/Number/ICUNumberFormatter.swift Outdated Show resolved Hide resolved

itingliu reviewed Dec 14, 2023

View reviewed changes

itingliu previously approved these changes Dec 14, 2023

View reviewed changes

itingliu reviewed Dec 15, 2023

View reviewed changes

Sources/FoundationInternationalization/Formatting/ByteCountFormatStyle.swift Show resolved Hide resolved

oscbyspro force-pushed the pull-request-001 branch from ba8bf28 to f065a2d Compare December 16, 2023 11:43

parkera approved these changes Dec 18, 2023

View reviewed changes

itingliu approved these changes Dec 19, 2023

View reviewed changes

itingliu merged commit c8b6d20 into swiftlang:main Dec 21, 2023
1 of 2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improved big-number formatting #346

Improved big-number formatting #346

oscbyspro commented Dec 11, 2023 •

edited

Loading

oscbyspro commented Dec 12, 2023 •

edited

Loading

oscbyspro commented Dec 12, 2023 •

edited

Loading

itingliu commented Dec 13, 2023 •

edited

Loading

oscbyspro commented Dec 13, 2023

itingliu left a comment

itingliu left a comment

itingliu commented Dec 15, 2023

oscbyspro commented Dec 16, 2023 •

edited

Loading

oscbyspro commented Dec 17, 2023

parkera left a comment

itingliu left a comment

itingliu commented Dec 19, 2023

itingliu commented Dec 21, 2023

itingliu commented Dec 21, 2023

Improved big-number formatting #346

Improved big-number formatting #346

Conversation

oscbyspro commented Dec 11, 2023 • edited Loading

Changes

Comments

Alternatives

oscbyspro commented Dec 12, 2023 • edited Loading

oscbyspro commented Dec 12, 2023 • edited Loading

itingliu commented Dec 13, 2023 • edited Loading

oscbyspro commented Dec 13, 2023

itingliu left a comment

Choose a reason for hiding this comment

itingliu left a comment

Choose a reason for hiding this comment

itingliu commented Dec 15, 2023

oscbyspro commented Dec 16, 2023 • edited Loading

oscbyspro commented Dec 17, 2023

parkera left a comment

Choose a reason for hiding this comment

itingliu left a comment

Choose a reason for hiding this comment

itingliu commented Dec 19, 2023

itingliu commented Dec 21, 2023

itingliu commented Dec 21, 2023

oscbyspro commented Dec 11, 2023 •

edited

Loading

oscbyspro commented Dec 12, 2023 •

edited

Loading

oscbyspro commented Dec 12, 2023 •

edited

Loading

itingliu commented Dec 13, 2023 •

edited

Loading

oscbyspro commented Dec 16, 2023 •

edited

Loading