Fix parsing foreign currency strings #1074

itingliu · 2024-12-07T01:12:51Z

Currently parsing a currency string would fail if the currency code does not match FormatStyle's locale region.

For example,

let style = Decimal.FormatStyle.Currency(code: "GBP", locale: .init(identifier: "en_US")).presentation(.isoCode)

This formats 3.14 into "GBP\u{0xa0}3.14", but parsing such string fails.

Fix this by always set the ICU formatter's currency code.

Resolves rdar://138179737

itingliu · 2024-12-07T01:13:08Z

@swift-ci please test

itingliu · 2024-12-07T02:02:09Z

@swift-ci please test

itingliu · 2024-12-07T02:02:46Z

Tests/FoundationInternationalizationTests/Formatting/NumberFormatStyleTests.swift

-
-        let frenchPrice = "57 379 €"
-        _verifyMatching(frenchPrice, formatStyle: frenchStyle, expectedUpperBound: frenchPrice.endIndex, expectedValue: 57379)
-        _verifyMatching(frenchPrice, formatStyle: floatStyle.locale(frFR), expectedUpperBound: frenchPrice.endIndex, expectedValue: 57379)


The tests incorrectly assumed that a currency style with "USD" code but "fr_FR" locale can correctly match a currency string with EUR symbol ("57 379,00 €"). This isn't correct, and is exactly what this bug is about: we only respect the locale regional currency, but ignore the currency code.

theMomax

Generally looks good! I think we just need to make sure the serialization format is backwards compatible, then this should be good to go!

Tests/FoundationInternationalizationTests/Formatting/NumberParseStrategyTests.swift

Sources/FoundationInternationalization/Formatting/Number/NumberFormatStyleConfiguration.swift

Currently parsing a currency string would fail if the currency code does not match `FormatStyle`'s locale region. For example, ```swift let style = Decimal.FormatStyle.Currency(code: "GBP", locale: .init(identifier: "en_US")).presentation(.isoCode) ``` This formats 3.14 into "GBP\u{0xa0}3.14", but parsing such string fails. Fix this by always set the ICU formatter's currency code. Resolves rdar://138179737

itingliu

I like this approach better than the previous one. We already have the currency code stored in the FormatStyle as a top-level property. There's no need to store it again in the nested configuration.

Also this should address @theMomax your concern about changing Codable representation. This should not change the serialized output anymore since we're not touching any Codable type in public types.

itingliu · 2024-12-17T01:53:00Z

@swift-ci please test

itingliu · 2024-12-17T05:24:45Z

@swift-ci please test

theMomax

This new approach is definitely better because I think people serializing FormatStyles is much more common than serializing ParseStrategys. But I think we should still make it backwards compatible in any scenario. Could we make currencyCode an optional property on CurrencyFormatStyleConfiguration.Collection? That way you could still avoid writing out a custom codable conformance.

Tests/FoundationInternationalizationTests/Formatting/NumberFormatStyleTests.swift

Sources/FoundationInternationalization/Formatting/Number/ICULegacyNumberFormatter.swift

theMomax · 2024-12-17T12:47:52Z

Sources/FoundationInternationalization/Formatting/Number/ICULegacyNumberFormatter.swift

    enum NumberFormatType : Hashable, Codable {
        case number(NumberFormatStyleConfiguration.Collection)
        case percent(NumberFormatStyleConfiguration.Collection)
-        case currency(CurrencyFormatStyleConfiguration.Collection)
+        case currency(CurrencyFormatStyleConfiguration.Collection, /*currency code*/ String)


This type is part of the Codable format of a number of public types conforming to ParseStrategy, e.g. IntegerParseStrategy. So while this solution means the Codable conformance for Decimal.FormatStyle.Currency is backwards compatible, your decoding test would still fail for IntegerParseStrategy.

Ah, you meant this is a var in IntegerParseStrategy and FloatingPointParseStrategy and friends ... OK we need tests for that too. Thanks

Add codable test for ParseStrategy

Another attempt: In 9a1e617 I removed the stored property numberFormatType from IntegerParseStrategy and FloatingPointParseStrategy. That means the we no longer preserve numberFormatType in serialization, and numberFormatType in previous Codable representation will be ignored from now on.

This is ok because the information is already provided by the other variable, formatStyle. In other words, numberFormatType has always been redundant. I also liked this because the previous serialization output feels like we're leaking internal information.

There is indeed a slight chance of regression -- if you manually craft an archive where numberFormatType represents different configurations from that of formatStyle, previously we'd favor what's represented by numberFormatType, but now we'd favor formatStyle. I think this risk is low enough though. Besides, I don't think we ever guarantee compatibility for manually crafted archives.

itingliu · 2024-12-17T13:33:54Z

Could we make currencyCode an optional property on CurrencyFormatStyleConfiguration.Collection? That way you could still avoid writing out a custom codable conformance.

I'm flipping back and forth. A custom Codable conformance feels brittle as well because anyone could easily change the struct without realizing the Codable implementation also needs to be updated. But on the other hand with a Codable implementation it at least makes it clear what properties are expected in the serialization

…eStrategy` and `FloatingPointParseStrategy` These properties are redundant as the information is already available through the public variable `formatStyle`.

itingliu · 2024-12-18T06:47:26Z

@swift-ci please test

theMomax

That sounds like a good solution, thank you!

itingliu · 2025-01-10T17:42:18Z

@swift-ci please test

- Remove an unused variable. - Calling `JSONDecoder`'s `func decode<T: Decodable>(_ type: T.Type, from data: Data) throws -> T` with `Decimal.FormatStyle.Currency` as the type isn't correct. But for some reason, the compiler allows it incorrectly. Fix it. - Clean up whitespaces while we're here.

itingliu requested a review from theMomax December 7, 2024 01:13

itingliu marked this pull request as draft December 7, 2024 01:22

itingliu commented Dec 7, 2024

View reviewed changes

theMomax reviewed Dec 8, 2024

View reviewed changes

Tests/FoundationInternationalizationTests/Formatting/NumberParseStrategyTests.swift Outdated Show resolved Hide resolved

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

itingliu force-pushed the parse-currency branch from 73a6149 to 10091ea Compare December 17, 2024 01:37

Update the test to throw properly instead of force unwrap

6b31a63

itingliu commented Dec 17, 2024

View reviewed changes

Remove another force try

85616ad

itingliu marked this pull request as ready for review December 17, 2024 05:25

theMomax reviewed Dec 17, 2024

View reviewed changes

itingliu added 2 commits December 18, 2024 14:29

Remove the stored numberFormatType and locale inside `IntegerPars…

9a1e617

…eStrategy` and `FloatingPointParseStrategy` These properties are redundant as the information is already available through the public variable `formatStyle`.

Address feedback

876db35

theMomax approved these changes Dec 18, 2024

View reviewed changes

Fix a typo

a245a41

itingliu merged commit e9d59b6 into swiftlang:main Jan 16, 2025
3 checks passed

itingliu deleted the parse-currency branch January 16, 2025 16:42

Fix parsing foreign currency strings #1074

Fix parsing foreign currency strings #1074

Uh oh!

Conversation

itingliu commented Dec 7, 2024

Uh oh!

itingliu commented Dec 7, 2024

Uh oh!

itingliu commented Dec 7, 2024

Uh oh!

itingliu Dec 7, 2024

Choose a reason for hiding this comment

Uh oh!

theMomax left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

itingliu left a comment

Choose a reason for hiding this comment

Uh oh!

itingliu commented Dec 17, 2024

Uh oh!

itingliu commented Dec 17, 2024

Uh oh!

theMomax left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

theMomax Dec 17, 2024

Choose a reason for hiding this comment

Uh oh!

itingliu Dec 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

itingliu Dec 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

itingliu Dec 18, 2024

Choose a reason for hiding this comment

Uh oh!

itingliu commented Dec 17, 2024

Uh oh!

itingliu commented Dec 18, 2024

Uh oh!

theMomax left a comment

Choose a reason for hiding this comment

Uh oh!

itingliu commented Jan 10, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

itingliu Dec 17, 2024 •

edited

Loading

itingliu Dec 17, 2024 •

edited

Loading