Open danthorpe opened 6 years ago
@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'
@attheodo - pull, & try again?
@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?
@attheodo - done, try again?
@danthorpe yeap, things look good on my end! Thanks for your effort mate. 👍
Is there any reason to still use NSLocale
over Locale
?
@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.
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'
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...
@aravasio this branch is a rewrite of Money. Looks like CustomStringConvertible
has not yet been implemented. I have the exact same problem.
@attheodo @aravasio CustomStringConvertible
has now been added, including a dedicated formatting string API.
@danthorpe I'm playing with the current build from this branch (as of 9/25), and it looks like your minorUnits are off -- if I create a Money object using Money(floatLiteral: 5000)
, accessing minorUnits
gives me a value of 5000, where the previous version would give me 500000 (2 decimal places).
.description
appears to be working here - I get "$5,000"
@mattcorey thanks for raising this - There is an issue with the scale
property of the Currency
which isn't getting set correctly for the device
currency (via Locale
), which is affecting the the minor units. This might well be a bug in Locale
vs NSLocale
- and it's why the previous versions did not fully embrace Foundation.Locale
- I will look into this later today.
Also, as a side note - .description
might not be correctly working - as possibly it is just not showing you the 2 decimal places.
@danthorpe any updates on the minorUnits
/Locale
issue?
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
@clintonmedbery try now?
@attheodo is this fixed now? Still not got to the bottom of the changes in NumberFormatter
for this.
@bartvandendriessche thanks! Fixed.
@danthorpe Nope, sorry. Was rounding up, now I get AFA15 instead of 15 when I should get 14.78. Sorry...
@danthorpe nope, still doesn't look good. Getting $499.00
instead of $4.99
.
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.
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?
@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 testcalculation1]' 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 testcalculation1]' failed (0.117 seconds).
So, what's going on here?
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.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.
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
?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:)
271
about? - no idea about this.So, what does this mean right now?
Money
and equivalent perform decimal arithmetic, using "Bankers" rounding mode using Apple's underlying Decimal
types.FloatingPoint
- adding conformance is non-trivial (not even supported in master
of Swift yet).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.@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 :)
Is this stable now? Was the issue with minorUnits
solved in the end?
@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.
Is this going to be merged?
Any update on this?
@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
@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?
@mattcorey https://bugs.swift.org/browse/SR-2980
@tplester - That ticket claims to be resolved in December of 2016 - is it still an issue?
@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)
@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:
pod repo update
or with pod install --repo-update
.Note: as of CocoaPods 1.0, pod repo update
does not happen on pod install
by default.
Can you help me on this
@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.
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?
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 🚀
Xcode 9 snuck in some quite interesting breaking changes with Swift 3.2 and numeric protocols. In particular
Numeric
,SignedNumeric
,FloatingPoint
andSignedNumber
which no longer exists properly.Additionally,
Decimal
is now a value type bridged toNSDecimalNumber
but which does not provide a full implementation ofFloatingPoint
yet, but it does still expose decimal math operations (methods are now deprecated in favour of operators). TheDecimal
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:
[x]
CurrencyProtocol
- protocol which provides currency description info[x]
MoneyProtocol
- protocol which provides math functionality[x]
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.[x]
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.[x] Autogenerated typealiases for ISO money
[x] Localised formatting of Money values
Things which are currently missing