Skip to content
This repository has been archived by the owner on Jul 2, 2018. It is now read-only.

Rewrite for Xcode 9 & Swift 3.2 #84

Open
wants to merge 22 commits into
base: development
Choose a base branch
from

Conversation

danthorpe
Copy link
Owner

@danthorpe danthorpe commented Sep 20, 2017

Xcode 9 snuck in some quite interesting breaking changes with Swift 3.2 and numeric protocols. In particular Numeric, SignedNumeric, FloatingPoint and SignedNumber which no longer exists properly.

Additionally, Decimal is now a value type bridged to NSDecimalNumber but which does not provide a full implementation of FloatingPoint yet, but it does still expose decimal math operations (methods are now deprecated in favour of operators). The Decimal operators use .plain rounding mode.

So, this PR is pretty much the start of a complete re-write of Money, and what will eventually become v3. This PR is focused on getting core functionality working. To that end we have:

  • CurrencyProtocol - protocol which provides currency description info

  • MoneyProtocol - protocol which provides math functionality

  • Money which is for weakly typed currency (defaults to "device") - and provides an API entry point to eventually support dynamic money where the currency is unknown at compile time.

  • ISOMoney which is for money with a strongly typed currency, known at compile time. e.g. USD, and should have the same API as v2 types.

  • Autogenerated typealiases for ISO money

  • Localised formatting of Money values

Things which are currently missing

  • Codable support.
  • ApplePay extensions
  • Bitcoin/Crypto Currencies

@danthorpe danthorpe self-assigned this Sep 20, 2017
@danthorpe danthorpe added this to the 3.0 milestone Sep 20, 2017
@attheodo
Copy link

attheodo commented Sep 21, 2017

@danthorpe getting lots of Argument labels '(minorUnits:)' do not match any available overloads on what previously used to work fine on Money types.

Also Value of type 'Money' has no member 'amount'

@danthorpe
Copy link
Owner Author

@attheodo - pull, & try again?

@attheodo
Copy link

@danthorpe forgot to mention: you probably want to declare Money struct explicitly as public or else the compiler nags about not being to Use module 'Money' as a type.

Also, can you quickly take care of Value of type 'Money' has no member 'amount' too?

@danthorpe
Copy link
Owner Author

@attheodo - done, try again?

@attheodo
Copy link

@danthorpe yeap, things look good on my end! Thanks for your effort mate. 👍

@lmcd
Copy link

lmcd commented Sep 21, 2017

Is there any reason to still use NSLocale over Locale?

@danthorpe
Copy link
Owner Author

@lmcd no, and in fact Locale is better because properties like currencySymbol are actually defined all the way back to iOS 7, vs iOS 10 in NSLocale.

These are internal implementation details which will change here when I do the formatting stuff.

@aravasio
Copy link

aravasio commented Sep 23, 2017

Is there an expected merge date for this PR? I'm using Money and it currently breaks build. :/
Also, in case everyone else stumbles on this, too, for now you can do this and work with the content from this PR:

pod 'Money', :git => 'https://github.com/danthorpe/Money.git', :branch => 'MNY-79_updates_for_xcode_9'

@aravasio
Copy link

aravasio commented Sep 23, 2017

Also, what changed for Money? Because I used to be able to use strings as such:
myLabel.text = "( Money(floatLiteral: 10.2) )"

And the text would be converted to "$10.20" no problem. Now, however, it just prints:

po "\(Money(floatLiteral: data.price))"
"Money(decimal: 90, currency: Money.Currency(code: \"USD\", scale: 0, symbol: Optional(\"$\")))"

Am I SOOL with Swift 3.2/4 and should rollback to 3?

Edit: NVM. It seems locale is part of the stuff that Swift 4 broke...

@attheodo
Copy link

@aravasio this branch is a rewrite of Money. Looks like CustomStringConvertible has not yet been implemented. I have the exact same problem.

@danthorpe
Copy link
Owner Author

@attheodo @aravasio CustomStringConvertible has now been added, including a dedicated formatting string API.

@attheodo
Copy link

@danthorpe any updates on the minorUnits/Locale issue?


func numberFormatter(withStyle style: NumberFormatter.Style, forLocaleId localeId: String) -> NumberFormatter {
let canonicalId = Locale.canonicalIdentifier(from: localeId)
let locale = Locale(identifier: "\(canonicalId)@currency=\(code))")

Choose a reason for hiding this comment

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

There is one closing parentheses too many in the identifier here. It's causing some funky issues like getting the JPY symbol instead of the € symbol in some places etc.

This line should be:
let locale = Locale(identifier: "\(canonicalId)@currency=\(code)")

@clintonmedbery
Copy link

I apologize if this has been addressed, but my amounts are rounding to the nearest dollar. For example 15.78 would be 16.
Thanks for all your hard work @danthorpe

@danthorpe
Copy link
Owner Author

@clintonmedbery try now?
@attheodo is this fixed now? Still not got to the bottom of the changes in NumberFormatter for this.
@bartvandendriessche thanks! Fixed.

@clintonmedbery
Copy link

@danthorpe Nope, sorry. Was rounding up, now I get AFA15 instead of 15 when I should get 14.78. Sorry...

@attheodo
Copy link

@danthorpe nope, still doesn't look good. Getting $499.00 instead of $4.99.

Re-implements `.Local` class
@danthorpe
Copy link
Owner Author

So, what's important to know when using the Money type is that it determines the currency based on the devices local currency. i.e. whatever Locale.current returns. By default the iOS simulator is based in the US, and so this is en_US which has $USD currency. I am a developer in London, so when I run the tests for the macOS platform, which typically run on my own Macs they are en_GB which uses £GBP.

If you have found that you get a currency reporting as AFA after the commit above (which I guess represents a NULL currency) - then please also print out the .currency property of your Money value.

Likewise, if you are finding that the .minorUnits of Money value don't see correct - please also print out the .currency property. The issue here is being unable to detect the currency scale, which is typically 2 for currencies like USD, EUR or GBP, but is 0 for many eastern currencies such a JPN.

For ISOCurrency like the USD, GBP types etc - these are for use when you know the currency, i.e. you want to represent US dollars no matter what the territory is.

@mattcorey
Copy link

I put together a test that shows a couple of assertions that don't quite work the way I would expect. I don't print them out here, but in each case, the .currency property of the Money object is 'USD', with a scale of 2, as I would expect:

func testMoney() {
    let balance = 5000 as Money
    let payment = 100 as Money
    let apr = 0.20
    
    let monthlyInterestRate = apr / 12.0
    let interest = balance * monthlyInterestRate
    XCTAssertEqual(8333, interest.minorUnits)
    XCTAssertEqual(83.33 as Money, interest)

    let principal = payment - interest
    XCTAssertEqual(1667, principal.minorUnits)
    XCTAssertEqual(16.67 as Money, principal)
    
    let newBalance = balance - principal
    XCTAssertEqual(498333, newBalance.minorUnits)
    XCTAssertEqual(4983.33 as Money, newBalance)
}

Am I correct in my assumptions here, and the use of the Money type?

@danthorpe
Copy link
Owner Author

@mattcorey well - I understand where you're coming from. And to aid the discussion, I've re-written your test as though it was a unit test:

extension MoneyTestCase {
    
    func test__calculation1() {
        
        let balance: Money = 5000
        let payment: Money = 100
        let apr = 0.20
        
        let monthlyInterestRate = apr / 12.0
        
        let interest = balance * monthlyInterestRate
        let principal = payment - interest
        let newBalance = balance - principal
        
        print("interest: \(interest.decimal)")
        print("principal: \(principal.decimal)")
        print("newBalance: \(newBalance.decimal)")
        
        XCTAssertEqual(8333, interest.minorUnits)
        XCTAssertEqual(83.33 as Money, interest)
        XCTAssertEqual(1667, principal.minorUnits)
        XCTAssertEqual(16.67 as Money, principal)
        XCTAssertEqual(498333, newBalance.minorUnits)
        XCTAssertEqual(4983.33 as Money, newBalance)
    }
}

And this is the output on the console (slightly modified to remove file paths)

Test Case '-[MoneyTests.MoneyTestCase test__calculation1]' started.
interest: 83.33333333333330944
principal: 16.66666666666669056
newBalance: 4983.33333333333330944
XCTAssertEqual failed: ("£83.33") is not equal to ("£83.33") -
XCTAssertEqual failed: ("1667") is not equal to ("1666") -
XCTAssertEqual failed: ("£16.67") is not equal to ("£16.67") -
XCTAssertEqual failed: ("498333") is not equal to ("271") -
XCTAssertEqual failed: ("£4,983.33") is not equal to ("£4,983.33") -
Test Case '-[MoneyTests.MoneyTestCase test__calculation1]' failed (0.117 seconds).

So, what's going on here?

  1. Money conforms to CustomStringConvertible, so when used by XCTAssert and similar, the localised representation is printed out - hence, £83.83. But this is not the true value of the underlying decimal.
  2. XCTAssertEqual is testing for exactly equal, so considering the point above, what is getting asserted is here really:
    XCTAssertEqual(83.330000000000000000, 83.33333333333330944)
    which correctly fails. So, most of these tests are false negatives.
  3. However - I totally understand what you are trying to assert, and quite frankly, I agree, that it would make sense that these test pass. So, what's going on? In Xcode 9 / Swift 3.2, Apple have added XCTAssertEqual(, accuracy:), which we could use, to ensure that results are accurate to within the currency scale. But, only if Money supported FloatingPoint - which is new in Swift 3.2, this isn't even Swift 4 yet. So, what is the deal with FloatingPoint?
  4. Money is essentially a simple type which attaches currency to decimal arithmetic, floating point math in binary is tricky, and its best left to platform vendors to implement. Apple have NSDecimalNumber and its value type equivalent in Swift, Decimal - but here's the kicker - neither conform to FloatingPoint, so can't be used with XCTAssert(_:,_:,accuracy:)
  5. What the hell is 271 about? - no idea about this.

So, what does this mean right now?

  1. Money and equivalent perform decimal arithmetic, using "Bankers" rounding mode using Apple's underlying Decimal types.
  2. They print out localised representation of the decimal according to the currency.
  3. Nothing conforms to FloatingPoint - adding conformance is non-trivial (not even supported in master of Swift yet).
  4. If you use this to perform your maths - all is accurate, so long as you keep the Money type.

What does this mean going forward?

  • Money needs to support archiving - ValueCoding for Swift 3.2, Codable for Swift 4.
  • MoneyTestCase should support a mechanism for asserting equality which tests the underlying Decimal value.
  • More arithmetic tests performing combinations of math operators.

@mattcorey
Copy link

@danthorpe Excellent description - thanks! I expected that most of the assert equals were failing because of an underlying data store, but your explanation lends some nuance to my understanding. Ideally, I think the type should provide ‘intuitive’ behaviors for things likely rounding and equality - to me, “Bankers Rounding” and equality on the localized representation seem the most intuitive, but opinions would likely vary (I’m not sold on the equality behavior myself, but it’s what comes to mind first when I consider how I think it should work)

As for the 271, I’m glad you got the same result, because I was questioning my basic understanding of math on that one :)

@lmcd
Copy link

lmcd commented Oct 4, 2017

Is this stable now? Was the issue with minorUnits solved in the end?

@danthorpe
Copy link
Owner Author

@lmcd the issue with the minorUnits has been fixed - but waiting a bit longer for feedback.

Will also address the issues that @mattcorey has helped raise in the tests before this is merged.

@clintonmedbery
Copy link

Is this going to be merged?

@danthorpe danthorpe mentioned this pull request Nov 7, 2017
@Coledunsby
Copy link

Any update on this?

@tplester
Copy link

tplester commented Feb 6, 2018

@mattcorey @danthorpe The 271 is from an issue with NSDecimalNumber breaking on values with a large amount of decimal places when you try to get the intValue. The proper solution is to get doubleValue and cast to Int

@mattcorey
Copy link

@tplester Interesting - is this issue documented anywhere?

@danthorpe I’m assuming this would need to be handled internally - are you still active with the library?

@tplester
Copy link

tplester commented Feb 6, 2018

@mattcorey
Copy link

@tplester - That ticket claims to be resolved in December of 2016 - is it still an issue?

@tplester
Copy link

tplester commented Feb 6, 2018

@mattcorey The ticket says its resolved but the bug is still there in Xcode 9.
You can test the following code in the Playground to see the issue:

var num = NSDecimalNumber(value: 100)
num = num.dividing(by: NSDecimalNumber(value: 3))

num.int32Value
num.intValue
num.int64Value
Int(num.doubleValue)

@SubbaNelakudhiti
Copy link

@danthorpe @tplester From yesterday itself, I'm trying to update or install Money framework into my project. I'm using "Pod install Money" command from terminal, then it's showing as [!] Unknown command: Money(I declared as "Pod 'Money'" in pod file). Previously I declared as "pod 'Money', '~> 4.0.0'". If I'm using same thing now I'm getting

[!] CocoaPods could not find compatible versions for pod "Money":
In Podfile:
Money (~> 4.0.0)

None of your spec sources contain a spec satisfying the dependency: Money (~> 4.0.0).

You have either:

  • out-of-date source repos which you can update with pod repo update or with pod install --repo-update.
  • mistyped the name or version.
  • not added the source repo that hosts the Podspec to your Podfile.

Note: as of CocoaPods 1.0, pod repo update does not happen on pod install by default.

Can you help me on this

@danthorpe
Copy link
Owner Author

@SubbaNelakudhiti Please stick to your other open issue.

The message says

out-of-date source repos which you can update with pod repo update or with pod install --repo-update.

Did you try this? Please answer in #91.

@manan19
Copy link

manan19 commented Jun 23, 2018

Hi @danthorpe! I'm using this feature branch for one of my projects and would love to get it merged in. How can I help?

@danthorpe
Copy link
Owner Author

Hey all - I would suggest migrating to https://github.com/Flight-School/Money - looks like a carbon copy API wise - and it's new so written for Swift 4 🚀

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.