-
Notifications
You must be signed in to change notification settings - Fork 388
Allow nil to be set as default currency #698
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
Conversation
80bfef3
to
99a2216
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! Can you add an entry to the CHANGELOG as well? 🙏🏻
This is blocked by RubyMoney/money#1095 |
@sirwolfgang rebase this one with |
@yukideluxe Rebased, lmk if I need to make changes to the testing stuff. Seems look right, but it's been a min |
I guess we need to release We intend to do a release by the end of THIS WEEK, and I'll make sure your change in |
@sirwolfgang I am back! I did some changes here that I am pretty sure will get this branch green so we can just merge 🙏🏻 Care to resolve the conflicts so we can try?! 😊 |
@yukideluxe Done |
@@ -35,7 +35,7 @@ def register_currency=(currency_options) | |||
end | |||
|
|||
def set_currency_column_for_default_currency! | |||
iso_code = default_currency.iso_code | |||
iso_code = default_currency && default_currency.iso_code |
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.
Since Money only supports Ruby > 3.1, it is possible to use the safe navigation operator.
This kind of issues can be spotted with RuboCop
iso_code = default_currency && default_currency.iso_code | |
iso_code = default_currency&.iso_code |
context 'without a default currency' do | ||
let(:product) { OtherProduct.new } | ||
|
||
around do |example| | ||
default_currency = Money.default_currency | ||
Money.default_currency = nil | ||
|
||
example.run | ||
|
||
Money.default_currency = default_currency | ||
end |
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.
it is better to use ensure
here in case the test does not pass
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.
Using around
like this still calls whatever is called after example.run
even when the test fails or raises exceptions, so this is fine 👍🏻
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.
Thanks, and apologies for the misleading comment.
I have a branch with improvements to the spec, I'll remove the ensure
from here
money-rails/spec/active_record/monetizable_spec.rb
Lines 783 to 790 in c7645b0
around(:each) do |example| | |
begin | |
Money.locale_backend = :currency | |
example.run | |
ensure | |
Money.locale_backend = :i18n | |
end | |
end |
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 am merging this because it has been open for a while! We can work on followup improvements – like https://github.com/RubyMoney/money-rails/pull/698/files#r2202413849 – in another PR 🙏🏻
Thanks <3
This is a part of fixing the #697