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.

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

Fixed GetCurrency to be case-insensitive #125

Merged
merged 2 commits into from
Jul 29, 2024

Conversation

kotaroyamazaki
Copy link
Contributor

@kotaroyamazaki kotaroyamazaki commented Nov 8, 2022

NewFromFloat did not support lowercase currency codes because it used getCurrencyByCode.
As same as New(), fix to use newCurrency(code).get() instead.

Fixed GetCurrency to be case-insensitive so that it wouldn't fail at upper case.

And Add test case.

money.go Outdated
@@ -89,7 +89,7 @@ func New(amount int64, code string) *Money {
// NewFromFloat creates and returns new instance of Money from a float64.
// Always rounding trailing decimals down.
func NewFromFloat(amount float64, currency string) *Money {
currencyDecimals := math.Pow10(GetCurrency(currency).Fraction)
currencyDecimals := math.Pow10(newCurrency(currency).get().Fraction)
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think on letting GetCurrency to return currency in a case insensitive manner?

newCurrecy().get() seems a little convolute to do the same.

Copy link
Contributor Author

@kotaroyamazaki kotaroyamazaki Nov 11, 2022

Choose a reason for hiding this comment

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

Thanks, that's a good one. (in my way , GetCurrency still fails with uppercase codes.)
Fixed GetCurrency to be case-insensitive

686ba37

@kotaroyamazaki kotaroyamazaki changed the title fix NewFromFloat to use newCurrency instead of GetCurrency Fixed GetCurrency to be case-insensitive Nov 11, 2022
@Rhymond Rhymond merged commit 49b65f8 into Rhymond:master Jul 29, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants