Skip to content

Keep a rational allocation amount exact - #1231

Merged
yukideluxe merged 3 commits into
RubyMoney:mainfrom
SashaMIT:codered-rational-allocate
Sep 29, 2026
Merged

yukideluxe merged 3 commits into
RubyMoney:mainfrom
SashaMIT:codered-rational-allocate

Conversation

@SashaMIT

Copy link
Copy Markdown
Contributor

Summary

  • Money::Allocation.generate(Rational(1, 3), 1, false) returned 0.3333333333333333. The rational was converted through a float, so one part was not the original amount.
  • A rational is now converted with to_d. One part of 1/3 matches Rational(1, 3).to_d.

Test plan

  • Red: the allocation spec expected the to_d value and got the float approximation
  • Green: rspec spec/money/allocation_spec.rb (23 examples, 0 failures)

Made with Cursor

A rational was converted through a float, so one part of 1/3 was not 1/3.

@sunny sunny left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thank you. Can you add an entry to the CHANGELOG as well please?

@yukideluxe yukideluxe left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for fixing this! Just one comment (apart from adding the CHANGELOG line that @sunny requested) so we can wrap this up nicely 👍🏻

Comment thread lib/money/money/allocation.rb Outdated
number
elsif number.is_a? Rational
BigDecimal(number.to_f.to_s)
number.to_d

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Money has this customizable option

money/lib/money/money.rb

Lines 139 to 142 in 7348778

# @!attribute [rw] conversion_precision
# Used to specify precision for converting Rational to BigDecimal
#
# @return [Integer]
that is set to 16 by default but it's not used in this conversion 🙏🏻 Could we do number.to_d((Money.conversion_precision). According to Claude there are more places that it might not be respected but I need to look into this a bit more and it's totally unrelated to this PR anyways 😊

The tests will have to be tweaked too 🙏🏻

Allocation of 1/3 used to_d with no precision. It now uses Money.conversion_precision.
@SashaMIT

Copy link
Copy Markdown
Contributor Author

Thanks, both notes are in. The changelog line is under Unreleased, and a Rational now goes through to_d(Money.conversion_precision). The spec expects that same precision. Rational(1, 3) at the default 16 is 0.3333333333333333.

@yukideluxe yukideluxe left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

just a tweak in the spec!

Comment thread spec/money/allocation_spec.rb Outdated

it "keeps a rational amount instead of a float approximation" do
amount = Rational(1, 3)
expect(described_class.generate(amount, 1, false)).to eq([amount.to_d(Money.conversion_precision)])

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this test is running the code it tests and it currently passes in main so it's not actually testing the changes. Claudito suggests the following assertions and those look good to me!

it "converts a rational with Money.conversion_precision instead of through a float" do
  expect(described_class.generate(Rational(2, 3), 1, false)).to eq([BigDecimal("0.6666666666666667")])
  expect(described_class.generate(Rational(10**400, 1), 1, false)).to eq([BigDecimal("1e400")])
  expect(described_class.generate(Rational(1, 10**400), 1, false)).to eq([BigDecimal("1e-400")])
end

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Claudito 😁

The previous example compared the result to the same to_d call, so it passed on main. 2/3, 1e400, and 1e-400 are now fixed expected values.
@SashaMIT

Copy link
Copy Markdown
Contributor Author

Updated the spec to those three values. Rational(2, 3) is 0.6666666666666667, 10400 stays 1e400, and 1/10400 stays 1e-400. The conversion still goes through Money.conversion_precision.

@yukideluxe
yukideluxe merged commit 1d60efe into RubyMoney:main Sep 29, 2026
9 checks passed
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