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

[SD-1461] fix money_methods for ruby version < 2.7 #11373

Merged
merged 2 commits into from Oct 1, 2021
Merged

Conversation

KacperMekarski
Copy link
Contributor

@KacperMekarski KacperMekarski commented Sep 30, 2021

Added photo because in Travis we use use newer ruby versions:
https://github.com/spree/spree/blob/main/cli/lib/spree_cli/templates/extension/travis.yml#L17-L19

Screenshot 2021-09-30 at 16 49 59

I considered moving send(*[method_name, **args_list].reject(&:blank?)) to amount private method & default_opts.merge(opts).merge(args_list.slice(:currency) to options private method, but decided that passing arguments further will make it look too complicated.

@viezly
Copy link

viezly bot commented Sep 30, 2021

Changes preview:

Legend:

👀 Review pull request on Viezly

@damianlegawiec
Copy link
Member

@KacperMekarski this breaks the ruby 3.0 build, also the title/description is wrong, this works on ruby 2, but only on 2.7+

@KacperMekarski KacperMekarski changed the title [SD-1461] fix money_methods for ruby version < 3.0 [SD-1461] fix money_methods for ruby version < 2.7 Sep 30, 2021
@KacperMekarski KacperMekarski marked this pull request as ready for review September 30, 2021 16:15
@KacperMekarski
Copy link
Contributor Author

KacperMekarski commented Sep 30, 2021

@damianlegawiec tried another approach.
Checked again locally with Ruby 2.6.3 and it's working fine.

Comment on lines +30 to +32
amount = args_list.blank? ? send(method_name) : send(method_name, **args_list)

Spree::Money.new(amount, default_opts.merge(opts).merge(args_list.slice(:currency)))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tried earlier:
Spree::Money.new(send(*[method_name, **args_list].reject(&:blank?)), default_opts.merge(opts).merge(args_list.slice(:currency))) but did not work for ruby 3.*

@damianlegawiec damianlegawiec merged commit 55a22f9 into main Oct 1, 2021
@delete-merged-branch delete-merged-branch bot deleted the SD-1461 branch October 1, 2021 09:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants