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

Jump to bottom

Initializing a currency with Decimal.nan provides unexpected data #17

Closed
thonydam opened this issue Jan 22, 2020 · 1 comment
Closed

Initializing a currency with Decimal.nan provides unexpected data #17

thonydam opened this issue Jan 22, 2020 · 1 comment
Labels
bug Something isn't working

Comments

@thonydam
Copy link
Member

thonydam commented Jan 22, 2020 •
edited
Loading 

  func testInit_withNaN() {
    let usd = USD(.nan)
    XCTAssertEqual(usd.amount, .nan) // USD(0.09) is not equal to "NaN"
  }

Setting a breakpoint in init(_:) shows that all the Decimal operations properly respond with .nan.

The issue is NSDecimalNumber(decimal:) when returning the int64Value
@thonydam thonydam added the bug Something isn't working label Jan 22, 2020
Mordil added a commit that referenced this issue Feb 22, 2020
@Mordil
Rework AnyCurrency and CurrencyProtocol implementations
fb2c0a4 
Motivation:

While working on linux CI (#7) and due to bug #17, many holes in the current implementation were found.

These include issues with handling NaN representations, integer overflow, performance, and code reusability.

The primary issue is that the default implementations for protocol requirements
were not playing nicely with each other, especially while compiling with Swift 5.2.

Modifications:

- Change: `init(exactly)` to be `init<T: BinaryInteger)(minorUnits:)` to be more forgiving to the types, and to avoid confusion with `Numeric` initializers
- Change: `init(_:)` to be `init?(amount:)` to properly handle NaN values as well as to avoid `init(floatLiteral:)` accidental usage due to SE-0213
- Change: Code documentation to be more thorough and clear
- Change: `inverse` from a computed property to `negated()`, catching overflow
- Change: Localization from string interpolation is now explicit with `\(localize: <value>)` as the new form, to avoid clashes with the default interpolation behavior and default parameters

Result:

Implementations should be less buggy, Currency should compile in Swift 5+, with happier developers using the library.
@Mordil Mordil mentioned this issue Feb 23, 2020
Merged
Mordil added a commit that referenced this issue Feb 23, 2020
@Mordil
Rework AnyCurrency and CurrencyProtocol implementations (#25)
6b0a4d9 
Motivation:

While working on linux CI (#7) and due to bug #17, many holes in the current implementation were found.

These include issues with handling NaN representations, integer overflow, performance, and code reusability.

The primary issue is that the default implementations for protocol requirements
were not playing nicely with each other, especially while compiling with Swift 5.2.

Modifications:

- Change: `init(exactly)` to be `init<T: BinaryInteger)(minorUnits:)` to be more forgiving to the types, and to avoid confusion with `Numeric` initializers
- Change: `init(_:)` to be `init?(amount:)` to properly handle NaN values as well as to avoid `init(floatLiteral:)` accidental usage due to SE-0213
- Change: Code documentation to be more thorough and clear
- Change: `inverse` from a computed property to `negated()`, catching overflow
- Change: Localization from string interpolation is now explicit with `\(localize: <value>)` as the new form, to avoid clashes with the default interpolation behavior and default parameters

Result:

Implementations should be less buggy, Currency should compile in Swift 5+, with happier developers using the library.
@Mordil
Copy link
Member

Mordil commented Feb 23, 2020

This should be fixed in the next version. >= 0.4.0

@Mordil Mordil closed this as completed Feb 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@thonydam @Mordil