Skip to content

Conversation

@smudge
Copy link
Contributor

@smudge smudge commented Jan 23, 2018

Before:

This works as expected:

Money.new(0) - Money.new(100)
=> #<Money fractional:-100 currency:USD>

But this does not:

0 - Money.new(100)
=> #<Money fractional:100 currency:USD>

A common case for the above would be when summing a list and subtracing a value from the sum. For instance, this works:

my_list = [Money.new(-10), Money.new(10)]
my_list.sum - Money.new(100)
=> #<Money fractional:-100 currency:USD>

But if you happen to end up with an empty list and forget to handle it:

my_list = []
my_list.sum - Money.new(100)
=> #<Money fractional:100 currency:USD>

Uh oh! That's a bug!

Rather than requiring the developer to know to provide a default Money.empty value to .sum (which would fix the latter issue, but not the former one), this PR addresses the underlying issue of commutativeness being assumed during coercion even on subtraction. In reality, subtraction is not commutative, even if all we care about is the zero numeric case.

After:

Once we apply the correct order on subtraction with a zero CoercedNumeric, we get a negative monetary amount:

0 - Money.new(100)
=> #<Money fractional:-100 currency:USD>
my_list = []
my_list.sum - Money.new(100)
=> #<Money fractional:-100 currency:USD>

@coveralls
Copy link

coveralls commented Jan 23, 2018

Coverage Status

Coverage decreased (-0.6%) to 99.322% when pulling c7f0d4e on smudge:fix-zero-minus-money into 0a157b6 on RubyMoney:master.

4 similar comments
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.6%) to 99.322% when pulling c7f0d4e on smudge:fix-zero-minus-money into 0a157b6 on RubyMoney:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.6%) to 99.322% when pulling c7f0d4e on smudge:fix-zero-minus-money into 0a157b6 on RubyMoney:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.6%) to 99.322% when pulling c7f0d4e on smudge:fix-zero-minus-money into 0a157b6 on RubyMoney:master.

@coveralls
Copy link

coveralls commented Jan 23, 2018

Coverage Status

Coverage decreased (-0.6%) to 99.322% when pulling c7f0d4e on smudge:fix-zero-minus-money into 0a157b6 on RubyMoney:master.

Copy link
Contributor

@antstorm antstorm left a comment

Choose a reason for hiding this comment

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

Makes perfect sense, great PR, thank you!

@antstorm antstorm merged commit 6510825 into RubyMoney:master Jan 27, 2018
@smudge smudge deleted the fix-zero-minus-money branch January 27, 2018 16:37
AlexWayfer pushed a commit to AlexWayfer/money that referenced this pull request Aug 29, 2018
* Add failing spec for 0 - Money

* Fix 0 - Money case

* One more test to cover addition case (was passing before)
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.

3 participants