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

Revert "Drop support for initialization with another money object." #289

Merged

Conversation

elfassy
Copy link
Contributor

@elfassy elfassy commented Apr 11, 2024

Reverts #279

Why

Removing the ability from Money.new to accept a money object is creating toil on multiple repos, including core, preventing the gem to be updated
ex: https://github.com/Shopify/shipify/pull/27262

It will also allow us to stay consistent with expectations shared with the RubyMoney gem https://github.com/RubyMoney/money/blob/main/spec/money_spec.rb#L57-L60

We still want to prevent changing the currency as mentioned in the original PR description.

What

  • allow Money.new to accept a money object (revert)
  • raise if Money.new is passed a money object with a different currency, that would cause the currency to change

@elfassy elfassy force-pushed the revert-279-csalvato/drop-support-for-initialization-with-money branch from 0fd7158 to 241365b Compare April 11, 2024 16:06
Gemfile.lock Outdated
@@ -1,7 +1,7 @@
PATH
Copy link
Contributor

Choose a reason for hiding this comment

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

Was it intentional to remove Gemfile.lock from gitignore? Historically this is something that gems avoided to prevent situations where our Gemfile.lock unnecessarily pins versions that are incompatible with the dependencies of client apps instead of letting bundler figure it out

@mkorostoff-shopify
Copy link
Contributor

Looks good to me! The one thing I'm uncertain about is how/where the version numbering should be configured. This would would go in 2.2.0 (not 2.1.0 as shown in the lockfile) but we could make a separate PR for that.

@elfassy elfassy force-pushed the revert-279-csalvato/drop-support-for-initialization-with-money branch from c671051 to 905aa7b Compare April 11, 2024 17:52
@elfassy elfassy marked this pull request as ready for review April 11, 2024 17:55
Gemfile.lock Outdated Show resolved Hide resolved
@elfassy elfassy force-pushed the revert-279-csalvato/drop-support-for-initialization-with-money branch from 905aa7b to 995d7b9 Compare April 17, 2024 13:52
@elfassy elfassy merged commit f795fd2 into main Apr 17, 2024
11 checks passed
@elfassy elfassy deleted the revert-279-csalvato/drop-support-for-initialization-with-money branch April 17, 2024 13:54
@elfassy elfassy mentioned this pull request Apr 17, 2024
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.

None yet

3 participants