Money library change #110
Money library change #110
Conversation
agrberg
force-pushed
the
money_lib_change
branch
from
ba380e1
to
10daba0
Compare
lib/iex/resources/resource.rb
Outdated
| require 'money' | ||
|
|
||
| Money.locale_backend = :currency | ||
| Money.default_currency = 'USD' |
There was a problem hiding this comment.
This global worries me because it will impact anyone using this gem and money directly. Default these in parameters as needed, but don't change any global defaults.
There was a problem hiding this comment.
Agreed. I didn't know how big of a deal it would be as the locale_backend is set in money_helper https://github.com/artsy/money_helper/blob/main/lib/money_helper.rb#L9 but happy to make the change.
There was a problem hiding this comment.
So does this mean that we can accidentally break someone? Is this set anywhere in our tests?
I think this deserves an entry in UPGRADING.md with something like "you may need to set these in your application"?
There was a problem hiding this comment.
If they use Money in their project w/o setting locale_backend it will generate the following deprecation warning:
[DEPRECATION] You are using the default localization behaviour that will change in the next major release. Find out more - https://github.com/RubyMoney/money#deprecation
Depending where it is used, it can generate the following error:
I18n::InvalidLocale (:en is not a valid locale)
For example, I get this error when calling Money#format without the thousands_separator or decimal_mark options in the console for this project. However, I have no problem in the Rails project I'm using this gem in.
Definitely worth putting something in UPGRADING.MD.
agrberg
force-pushed
the
money_lib_change
branch
from
10daba0
to
87271a3
Compare
| @@ -19,7 +19,7 @@ Gem::Specification.new do |s| | |||
| s.add_dependency 'faraday', '>= 0.17' | |||
| s.add_dependency 'faraday_middleware' | |||
| s.add_dependency 'hashie' | |||
| s.add_dependency 'money_helper' | |||
| s.add_dependency 'money', '~> 6.0' | |||
There was a problem hiding this comment.
Do we need to lock to 6.0 major? Most likely 7 will work fine.
There was a problem hiding this comment.
We unfortunately do 馃槶 . 7 is still unreleased, introduces breaking changes, and even changes the name of the gem to money2 though the API is still on Money.
- 7 rc1 tag
- unreleased 7.0.0.rc1
- Option names changed as well in "renamed options, dropped fallbacks"
| raise ArgumentError unless options[:date].present? | ||
| raise ArgumentError unless options[:chartByDay].present? | ||
| raise ArgumentError unless options[:date] | ||
| raise ArgumentError unless options[:chartByDay] |
There was a problem hiding this comment.
Those don't support nil values, right?
There was a problem hiding this comment.
According to the current API docs chartByDay is an optional query param so we shouldn't enforce it being set here.
I can update the code and test but it feels separate from the purpose of this PR.
Drop dependency on `money_helper` Utilize `money` directly as defaults are for USD Remove #present? as `money_helper` brought in active support
agrberg
force-pushed
the
money_lib_change
branch
from
87271a3
to
46a5f7a
Compare
agrberg
force-pushed
the
money_lib_change
branch
from
46a5f7a
to
e672773
Compare
|
Merged. Maybe we should release as 1.6, feels too major for a patch version bump. |
While working with the
money_helperlibrary to enable the upgrade in #109 I realized my non-breaking change in artsy/money_helper#27 would eventually need to be a breaking change to improve the API.with_symbolis just a pass through tosymbolfor theMoney#formatoptions and that PR enables those to be passed through directly.When I looked at the
moneylibrary itself, I determinedResource.to_dollarcould instead depend on an even smaller interface as the defaults make sense for the purposes of a US market API.A minor drawback/benefit is that this effectively drops the hidden dependency on
activesupport(for built gems, it is still needed in test fordanger-toc).Feedback very welcome 馃檹