-
Notifications
You must be signed in to change notification settings - Fork 5
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.
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
Initial implementation #1
Conversation
a7b9b9d
to
9305ed4
Compare
9305ed4
to
2e91195
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me 👍 !
Have minor comments about naming that you can ignore if you feel strongly about it.
Sources/Currency/Money.swift
Outdated
/// | ||
/// See `CurrencyMetadata.minorUnits` and `Foundation.Decimal.RoundingMode.bankers`. | ||
/// - Note: This is usually the desired value to work with, as it is as precise as usually needed. | ||
var amount: Decimal { get } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rawAmount
is not super meaningful. What we call Amount
here is not the actual amount but the rounded value.
To be more explicit, should we have:
rawAmount
=> amount
amount
=> roundedAmount
or rounded
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually had that original naming, but towards the end I moved to the current naming - as my concern is many people would use amount
when they really want roundedAmount
. 99% of the time, people want the every day banker's rounded value.
How do you feel about:
amount
=> roundedAmount
rawAmount
=> exactAmount
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Sources/Currency/Money.swift
Outdated
/// To get around this, the provided `other` amount will be rounded to the same precision as the `minorUnits` of the Money's currency using the "banker" mode. | ||
/// | ||
/// See `Money.amount`. | ||
/// - Parameter other: The other amount to "bankers' round, then compare against. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The parameter description kind of implies that we should pass other
as the rounded amount. Where we should actually pass the actual amount
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a reasonable misunderstanding - I'll clarify this section.
No description provided.