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

default_currency misconception #855

Closed
adamduffy opened this issue Mar 13, 2019 · 3 comments
Closed

default_currency misconception #855

adamduffy opened this issue Mar 13, 2019 · 3 comments

Comments

@adamduffy
Copy link

when doing multi-currency business, it is very dangerous to not know for sure and assume a "default currency". i.e. storing {amount: 1, currency: nil} and assuming that's USD. USA is not the center of the universe, and neither is any other country/currency.

ex:

Money.from_amount(1)
=> #<Money fractional:100 currency:USD>

i have a hard time thinking of a use case where not knowing and guessing is better than knowing for sure. i can think of use cases for the idea of a "preferred currency", which could be used for comparisons, but never as a fallback for unknown.

I am curious if there are use cases for the dangerous assumption, or if this is intended to be used more as a "preferred" currency.

i do see that Money.default_currency=nil is a good workaround. perhaps this should be the default instead of USD?

@antstorm
Copy link
Contributor

@adamduffy I guess the reason behind default_currency in the first place is for single-currency apps where everything is expected to be in one currency. But if anything it's just a shorthand that is potentially dangerous as you've noted.

I like the idea of switching the default to nil forcing people to pick one at their own risk. As it's a breaking change we can include it in the upcoming v7 release. Would you be interested in opening a PR?

@thealiilman
Copy link

thealiilman commented May 21, 2019

Hey @antstorm, I stumbled upon this issue recently. There doesn't seem to be any work being done on this, and I thought this is a nice issue for me to get my hands dirty with contributing to open source. 🤓

I misunderstood the exchange above between you and Adam, hence the initial PR was closed. I had thought it was to force the developer to set the default value, and not initialising a Money object with nil as its currency.

I have created a PR for this change. 👌

@antstorm
Copy link
Contributor

antstorm commented Jun 2, 2019

#868 is now merged and will get included in v7 release

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

No branches or pull requests

3 participants