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.

Sign up for GitHub

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

Fix parsing foreign currency strings #1074

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Fix parsing foreign currency strings #1074

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
10091ea
Fix parsing foreign currency strings
itingliu Dec 17, 2024
6b31a63
Update the test to throw properly instead of force unwrap
itingliu Dec 17, 2024
85616ad
Remove another force try
itingliu Dec 17, 2024
9a1e617
Remove the stored `numberFormatType` and `locale` inside `IntegerPars…
itingliu Dec 18, 2024
876db35
Address feedback
itingliu Dec 18, 2024
a245a41
Fix a typo
itingliu Dec 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Sources/FoundationInternationalization/Formatting/Number/Decimal+ParseStrategy.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ extension Decimal.ParseStrategy {
numberFormatType = .percent(format.collection)
locale = format.locale
} else if let format = formatStyle as? Decimal.FormatStyle.Currency {
numberFormatType = .currency(format.collection)
numberFormatType = .currency(format.collection, format.currencyCode)
locale = format.locale
} else {
// For some reason we've managed to accept a format style of a type that we don't own, which shouldn't happen. Fallback to the default decimal style and try anyways.
Expand Down
2 changes: 1 addition & 1 deletion Sources/FoundationInternationalization/Formatting/Number/FloatingPointParseStrategy.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ public extension FloatingPointParseStrategy {
self.formatStyle = format
self.lenient = lenient
self.locale = format.locale
self.numberFormatType = .currency(format.collection)
self.numberFormatType = .currency(format.collection, format.currencyCode)
}
}

Expand Down
7 changes: 4 additions & 3 deletions Sources/FoundationInternationalization/Formatting/Number/ICULegacyNumberFormatter.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@ internal final class ICULegacyNumberFormatter : @unchecked Sendable {
enum NumberFormatType : Hashable, Codable {
case number(NumberFormatStyleConfiguration.Collection)
case percent(NumberFormatStyleConfiguration.Collection)
case currency(CurrencyFormatStyleConfiguration.Collection)
case currency(CurrencyFormatStyleConfiguration.Collection, /*currency code*/ String)
itingliu marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@itingliu itingliu Dec 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@itingliu itingliu Dec 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

  • Add codable test for ParseStrategy

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

case descriptive(DescriptiveNumberFormatConfiguration.Collection)
}

Expand All @@ -143,7 +143,7 @@ internal final class ICULegacyNumberFormatter : @unchecked Sendable {
}
case .percent(_):
icuType = .percent
case .currency(let config):
case .currency(let config, let _):
icuType = config.icuNumberFormatStyle
case .descriptive(let config):
icuType = config.icuNumberFormatStyle
Expand Down Expand Up @@ -178,12 +178,13 @@ internal final class ICULegacyNumberFormatter : @unchecked Sendable {
}
}

case .currency(let config):
case .currency(let config, let currencyCode):
setMultiplier(config.scale, formatter: formatter)
setPrecision(config.precision, formatter: formatter)
setGrouping(config.group, formatter: formatter)
setDecimalSeparator(config.decimalSeparatorStrategy, formatter: formatter)
setRoundingIncrement(config.roundingIncrement, formatter: formatter)
try setTextAttribute(.currencyCode, formatter: formatter, value: currencyCode)

// Currency specific attributes
if let sign = config.signDisplayStrategy {
Expand Down
2 changes: 1 addition & 1 deletion Sources/FoundationInternationalization/Formatting/Number/IntegerParseStrategy.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,6 @@ public extension IntegerParseStrategy {
self.formatStyle = format
self.lenient = lenient
self.locale = format.locale
self.numberFormatType = .currency(format.collection)
self.numberFormatType = .currency(format.collection, format.currencyCode)
}
}
1 change: 1 addition & 0 deletions Sources/FoundationInternationalization/ICU/ICU+Enums.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,7 @@ extension UNumberFormatAttribute {

extension UNumberFormatTextAttribute {
static let defaultRuleSet = UNUM_DEFAULT_RULESET
static let currencyCode = UNUM_CURRENCY_CODE
}

extension UDateRelativeDateTimeFormatterStyle {
Expand Down
69 changes: 56 additions & 13 deletions Tests/FoundationInternationalizationTests/Formatting/NumberFormatStyleTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -595,6 +595,39 @@ final class NumberFormatStyleTests: XCTestCase {
XCTAssertEqual((-922337203685477 as Decimal).formatted(baseStyle.rounded(rule: .awayFromZero, increment: 100)), "-$1000T")
}

func testCurrency_Codable() throws {
let gbpInUS = Decimal.FormatStyle.Currency(code: "GBP", locale: enUSLocale)
let encoded = try JSONEncoder().encode(gbpInUS)
let json_gbp = String(data: encoded, encoding: String.Encoding.utf8)!
print(json_gbp)
itingliu marked this conversation as resolved.
Show resolved Hide resolved

let previouslyEncoded = """
{
"collection":
{
"presentation":
{
"option": 1
}
},
"currencyCode": "GBP",
"locale":
{
"current": 0,
"identifier": "en_US"
}
}
""".data(using: .utf8)
itingliu marked this conversation as resolved.
Show resolved Hide resolved

guard let previouslyEncoded else {
XCTFail()
return
}

let decoded = try JSONDecoder().decode(Decimal.FormatStyle.Currency, from: previouslyEncoded)
XCTAssertEqual(decoded, gbpInUS)
}

func testCurrency_scientific() throws {
let baseStyle = Decimal.FormatStyle.Currency(code: "USD", locale: Locale(identifier: "en_US")).notation(.scientific)

Expand Down Expand Up @@ -1905,23 +1938,33 @@ final class FormatStylePatternMatchingTests : XCTestCase {
_verifyMatching("€52,249", formatStyle: floatStyle, expectedUpperBound: nil, expectedValue: nil)
_verifyMatching("€52,249", formatStyle: decimalStyle, expectedUpperBound: nil, expectedValue: nil)

// Different locale
let frenchStyle: IntegerFormatStyle<Int>.Currency = .init(code: "EUR", locale: frFR)

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

Choose a reason for hiding this comment

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

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.

_verifyMatching(frenchPrice, formatStyle: decimalStyle.locale(frFR), expectedUpperBound: frenchPrice.endIndex, expectedValue: 57379)

_verifyMatching("\(frenchPrice) semble beaucoup", formatStyle: frenchStyle, expectedUpperBound: frenchPrice.endIndex, expectedValue: 57379)
_verifyMatching("\(frenchPrice) semble beaucoup", formatStyle: floatStyle.locale(frFR), expectedUpperBound: frenchPrice.endIndex, expectedValue: 57379)
_verifyMatching("\(frenchPrice) semble beaucoup", formatStyle: decimalStyle.locale(frFR), expectedUpperBound: frenchPrice.endIndex, expectedValue: 57379)

let frenchPrice = frenchStyle.format(57379)
XCTAssertEqual(frenchPrice, "57 379,00 €")
_verifyMatching("57 379,00 €", formatStyle: frenchStyle, expectedUpperBound: "57 379,00 €".endIndex, expectedValue: 57379)
_verifyMatching("57 379 €", formatStyle: frenchStyle, expectedUpperBound: "57 379 €".endIndex, expectedValue: 57379)
_verifyMatching("57 379,00 € semble beaucoup", formatStyle: frenchStyle, expectedUpperBound: "57 379,00 €".endIndex, expectedValue: 57379)

// Does not match when matching with USD style
_verifyMatching("57 379,00 €", formatStyle: floatStyle.locale(frFR), expectedUpperBound: nil, expectedValue: nil)
_verifyMatching("57 379,00 €", formatStyle: decimalStyle.locale(frFR), expectedUpperBound: nil, expectedValue: nil)

// Mix currency and locale
_verifyMatching("57 379,00 $US", formatStyle: floatStyle.locale(frFR), expectedUpperBound: "57 379,00 $US".endIndex, expectedValue: 57379)
_verifyMatching("57 379,00 $US", formatStyle: decimalStyle.locale(frFR), expectedUpperBound: "57 379,00 $US".endIndex, expectedValue: 57379)
_verifyMatching("57 379,00 $US semble beaucoup", formatStyle: floatStyle.locale(frFR), expectedUpperBound: "57 379,00 $US".endIndex, expectedValue: 57379)
_verifyMatching("57 379,00 $US semble beaucoup", formatStyle: decimalStyle.locale(frFR), expectedUpperBound: "57 379,00 $US".endIndex, expectedValue: 57379)

// Range tests
let newFrenchStr = "<remplir le blanc> coûte \(frenchPrice)"
let frenchMatchRange = newFrenchStr.firstIndex(of: "5")! ..< newFrenchStr.endIndex
_verifyMatching(newFrenchStr, formatStyle: frenchStyle, range: frenchMatchRange, expectedUpperBound: newFrenchStr.endIndex, expectedValue: 57379)
_verifyMatching(newFrenchStr, formatStyle: floatStyle.locale(frFR), range: frenchMatchRange, expectedUpperBound: newFrenchStr.endIndex, expectedValue: 57379)
_verifyMatching(newFrenchStr, formatStyle: decimalStyle.locale(frFR), range: frenchMatchRange, expectedUpperBound: newFrenchStr.endIndex, expectedValue: 57379)

// Mix currency and locale range tests
let newFrenchUSDStr = "<remplir le blanc> coûte 57 379,00 $US"
let usdPriceRange = newFrenchUSDStr.firstIndex(of: "5")! ..< newFrenchUSDStr.endIndex
_verifyMatching(newFrenchUSDStr, formatStyle: floatStyle.locale(frFR), range: usdPriceRange, expectedUpperBound: newFrenchUSDStr.endIndex, expectedValue: 57379)
_verifyMatching(newFrenchUSDStr, formatStyle: decimalStyle.locale(frFR), range: usdPriceRange, expectedUpperBound: newFrenchUSDStr.endIndex, expectedValue: 57379)

// Sign tests
let signTests = [
Expand Down
63 changes: 62 additions & 1 deletion Tests/FoundationInternationalizationTests/Formatting/NumberParseStrategyTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,67 @@ final class NumberParseStrategyTests : XCTestCase {
_verifyRoundtripCurrency(negativeData, currencyStyle.decimalSeparator(strategy: .always), "currency style, decimal display: always")
}

func testParseCurrencyWithDifferentCodes() throws {
let enUS = Locale(identifier: "en_US")
// Decimal
let style = Decimal.FormatStyle.Currency(code: "GBP", locale: enUS).presentation(.isoCode)
XCTAssertEqual(style.format(3.14), "GBP 3.14")

let parsed = try style.parseStrategy.parse("GBP 3.14")
XCTAssertEqual(parsed, 3.14)

// Floating point
let floatingPointStyle: FloatingPointFormatStyle<Double>.Currency = .init(code: "GBP", locale: enUS).presentation(.isoCode)
XCTAssertEqual(floatingPointStyle.format(3.14), "GBP 3.14")

let parsedFloatingPoint = try floatingPointStyle.parseStrategy.parse("GBP 3.14")
XCTAssertEqual(parsedFloatingPoint, 3.14)

// Integer
let integerStyle: IntegerFormatStyle<Int32>.Currency = .init(code: "GBP", locale: enUS).presentation(.isoCode)
XCTAssertEqual(integerStyle.format(32), "GBP 32.00")

let parsedInt = try integerStyle.parseStrategy.parse("GBP 32.00")
XCTAssertEqual(parsedInt, 32)
}

func test_roundtripForeignCurrency() throws {
let testData: [Int] = [
87650000, 8765000, 876500, 87650, 8765, 876, 87, 8, 0
]
let negativeData: [Int] = [
-87650000, -8765000, -876500, -87650, -8765, -876, -87, -8
]

func _verifyRoundtripCurrency(_ testData: [Int], _ style: IntegerFormatStyle<Int>.Currency, _ testName: String = "", file: StaticString = #filePath, line: UInt = #line) throws {
for value in testData {
let str = style.format(value)
let parsed = try Int(str, strategy: style.parseStrategy)
XCTAssertEqual(value, parsed, "\(testName): formatted string: \(str) parsed: \(parsed)", file: file, line: line)

let nonLenientParsed = try! Int(str, format: style, lenient: false)
XCTAssertEqual(value, nonLenientParsed, file: file, line: line)
}
}

let currencyStyle: IntegerFormatStyle<Int>.Currency = .init(code: "EUR", locale: Locale(identifier: "en_US"))
try _verifyRoundtripCurrency(testData, currencyStyle, "currency style")
try _verifyRoundtripCurrency(testData, currencyStyle.sign(strategy: .always()), "currency style, sign: always")
try _verifyRoundtripCurrency(testData, currencyStyle.grouping(.never), "currency style, grouping: never")
try _verifyRoundtripCurrency(testData, currencyStyle.presentation(.isoCode), "currency style, presentation: iso code")
try _verifyRoundtripCurrency(testData, currencyStyle.presentation(.fullName), "currency style, presentation: iso code")
try _verifyRoundtripCurrency(testData, currencyStyle.presentation(.narrow), "currency style, presentation: iso code")
try _verifyRoundtripCurrency(testData, currencyStyle.decimalSeparator(strategy: .always), "currency style, decimal display: always")

try _verifyRoundtripCurrency(negativeData, currencyStyle, "currency style")
try _verifyRoundtripCurrency(negativeData, currencyStyle.sign(strategy: .accountingAlways()), "currency style, sign: always")
try _verifyRoundtripCurrency(negativeData, currencyStyle.grouping(.never), "currency style, grouping: never")
try _verifyRoundtripCurrency(negativeData, currencyStyle.presentation(.isoCode), "currency style, presentation: iso code")
try _verifyRoundtripCurrency(negativeData, currencyStyle.presentation(.fullName), "currency style, presentation: iso code")
try _verifyRoundtripCurrency(negativeData, currencyStyle.presentation(.narrow), "currency style, presentation: iso code")
try _verifyRoundtripCurrency(negativeData, currencyStyle.decimalSeparator(strategy: .always), "currency style, decimal display: always")
}

let testNegativePositiveDecimalData: [Decimal] = [ Decimal(string:"87650")!, Decimal(string:"8765")!,
Decimal(string:"876.5")!, Decimal(string:"87.65")!, Decimal(string:"8.765")!, Decimal(string:"0.8765")!, Decimal(string:"0.08765")!, Decimal(string:"0.008765")!, Decimal(string:"0")!, Decimal(string:"-0.008765")!, Decimal(string:"-876.5")!, Decimal(string:"-87650")! ]

Expand Down Expand Up @@ -188,7 +249,7 @@ final class NumberParseStrategyTests : XCTestCase {
XCTAssertEqual(try! strategy.parse("-1,234.56 US dollars"), Decimal(string: "-1234.56")!)
XCTAssertEqual(try! strategy.parse("-USD\u{00A0}1,234.56"), Decimal(string: "-1234.56")!)
}

func testNumericBoundsParsing() throws {
let locale = Locale(identifier: "en_US")
do {
Expand Down