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

Add configurable support for using the shopify-money gem #213

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jeremycw
Copy link

Unfortunately shopify-money and money do not play well together. Both gems provide a Money class and an entry point into the gem at lib/money.rb. This makes it pretty difficult to include double_entry in a codebase that makes use of the shopify-money gem.

To work around this I've added a configuration option shopify_money that allows double_entry to be configured to internally use the shopify-money gem instead of the money gem. shopify-money was added as a development dependency since it's expected that people using this option will be requiring shopify-money themselves. The specs can be run against either gem by providing the MONEY_GEM env variable. For example:

DB=mysql MONEY_GEM=shopify-money bundle exec rspec
DB=mysql MONEY_GEM=money bundle exec rspec

Omitting the MONEY_GEM var will use the money gem as usual.

This was done by adding a DoubleEntry::Money class to abstract the money and shopify-money apis. When using money it actually doesn't do anything and just returns the raw Money object. When using shopify-money it wraps the api to make it match the one provided by the money gem.

Tested with shopify-money 0.14.2 & 1.0.2.pre

@ricobl
Copy link
Contributor

ricobl commented May 23, 2022

Hey @jeremycw! Thanks for your contribution.

That's really a tricky issue, those gems should ideally use different namespaces, it feels like shopify-money should use the Shopify:: namespace to make it consistent with the gem name, maybe you can consider reaching out to its maintainers.

But I understand that we can't expect that to happen, so in the meantime I'll recommend a slightly different approach.

While your suggestion seems to work, it introduces a dependency on shopify-money that we would have to maintain going forward and we don't have the throughput for that.

An alternative is introducing a configurable money adapter. It could be defined in DoubleEntry.config.money_adapter having a default of Money (or ::Money). You can then override it with something like ::ShopifyMoneyAdapter which would have an implementation compatible with the money gem but based on shopify-money, similar to the solution you provided here.

I noticed that your PR mostly changes references to Money in specs but not in the gem code. Maybe the interface is somewhat equivalent but if we proceed with this I feel like all references to Money methods should be changed to use the adapter.

Let me know what you think.

@jeremycw jeremycw force-pushed the shopify-money branch 3 times, most recently from b04a8e4 to a710673 Compare May 24, 2022 18:17
Unfortunately `shopify-money` and `money` do not play well together. Both gems provide a `Money` class and an entry point into the gem at `lib/money.rb`. This makes it pretty difficult to include double_entry in a codebase that makes use of the `shopify-money` gem.

To work around this I've added a configuration option `money_adapter` that allows double_entry to be configured to internally delegate methods to this adapter instead of using the `money` gem directly. This allows users of double_entry to provide an adapter class that allows integration with arbitrary money backends for better interoperability with their system.

This was done by adding a `DoubleEntry::Money` class that delegates its singleton methods to the adapter.
@jeremycw
Copy link
Author

@ricobl Thanks for the suggestion, that makes sense. I've taken a first pass at your suggestion. I've left DoubleEntry::Money in as a module that delegates new and any singleton methods to the configured adapter. Let me know if you would prefer me to change the actual call sites to use DoubleEntry.config.money_adapter instead.

@daande
Copy link

daande commented Jun 28, 2022

@ricobl Can we get this merged in and a new release put out?

#211

@bdewater
Copy link

bdewater commented Oct 6, 2022

This PR can be simplified by replacing Money.zero(currency) with Money.new(0, currency). It's all it does anyway and then from this gem's point of view creating new Money objects works exactly the same regardless of implementation.

@jeremycw
Copy link
Author

jeremycw commented Jan 5, 2023

@orien Could you take a look at this? It seems like the original reviewer is no longer a maintainer.

@daande daande mentioned this pull request Oct 25, 2023
@daande
Copy link

daande commented Oct 25, 2023

@orien I want to see if there is any reason why we cannot get this merged and a new 2.1 release cut?

@orien
Copy link
Member

orien commented Oct 25, 2023

@daande Are you using this branch in production already? Does it work as expected?

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.

5 participants