Shopify / Shopify/money

Summing `Money`s

Open
#282 2 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
Ruby
Stars
361
Forks
50
PR merge metrics
No merged PRs in 30d

Description

We ran into an interesting bug today. We had code that looked like this:

foos.sum(&:amount) == expected_total

where foos is a collection that has an amount property that returns a Money, and expected_total is also a Money.

This works fine, except when foos is empty. In that case, the default value 0 (an Integer) is used, and as it turns out:

0 == Money.new(0, "USD") # => false

The fix is easy enough, just provide an explicit initial value:

foos.sum(Money.new(0, "USD"), &:amount) == expected_total

but this still seems like a very easy way for developers to shoot themselves in the foot when dealing with Money. I was wondering if Shopify (or others) have dealt with this issue before and have any recommendations for avoiding this issue.

One thought I had was a Rubocop rule that requires an explicit initial value for all #sum calls, but that seems overkill and annoying since it works fine when dealing with non-Money numerics.

Contributor guide

No contributing guide indexed for this repository

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with the reported Ruby Enumerable#sum behavior for an empty collection whose values are Money, using the examples in this issue. No repository file or test is named; first locate the relevant sum integration and existing Money tests, then define a regression test and confirm the project agrees on the desired behavior.

Written by the indexing model from the issue text.

Assessment

Tech stack
ruby
Domain
fintech-quant
Issue type
Bug
Difficulty
5/5
Estimated time
Over a week
Activity status
Stale
Clarity
Needs clarification
Newbie friendliness
30/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.