Fix: add % method to BigDecimal

[ghost]

This PR adds a % method to the BigDecimal class in src/big/big_decimal.cr. Should fix #9782 .

[asterite]

Looks great! Could you add some tests?

[ghost]

Could you add some tests?

Sure! I assume I'd just add some files in the spec directory?

[ghost]

All done, should have some specs now!

[asterite]

Thanks!

A couple of things:

• We usually write some_computation.should eq(expected_value) but in the spec you wrote it's the other way around
• It would be nice to have a few more specs to make sure all of the if branches you wrote are covered

[ghost]

We usually write some_computation.should eq(expected_value) but in the spec you wrote it's the other way around

My bad, first time contributing. I'll go ahead and fix that!

[ghost]

I was looking at the BigDecimal specs though, and most of them are written similar to this:

BigDecimal.new(1).should eq(BigDecimal.new(1) / BigDecimal.new(1))

[asterite]

Yeah, I think I was absent when BigDecimal was introduced, the specs could have been written in a better way. For starters, we never have a huge single spec to test different methods. There's so much to improve in this class.

Since I don't have knowledge of this and I have very little time now, I'll leave others to review this.

Thank you again!

[asterite]

Looks good!

It might be nice to also have some tests that check that decimals are properly divided. For example checking the remainder of 5678.912 % 12.34. After all these are decimal types :-)

[bcardiff]

Thanks @sugarfi , I think that we need a couple of more specs before merging this to ensure things works as in float.

In float_spec.cr, the following specs are used to check for the result sign on negative operands, and some cases with decimal values.

  describe "modulo" do
    it "raises when mods by zero" do
      expect_raises(DivisionByZeroError) { 1.2.modulo 0.0 }
    end

    it { (13.0.modulo 4.0).should eq(1.0) }
    it { (13.0.modulo -4.0).should eq(-3.0) }
    it { (-13.0.modulo 4.0).should eq(3.0) }
    it { (-13.0.modulo -4.0).should eq(-1.0) }
    it { (11.5.modulo 4.0).should eq(3.5) }
    it { (11.5.modulo -4.0).should eq(-0.5) }
    it { (-11.5.modulo 4.0).should eq(0.5) }
    it { (-11.5.modulo -4.0).should eq(-3.5) }
  end

The same test cases in BigDecimal would be enough.

float also has #modulo (as an alias to %) and #reminder with a different treatment of the sign. Those can be handled in another PR or include it here if wanted.

[ghost]

@bcardiff ok, will update the spec. I did implement this modulo using the built-in % operator - will that cause floating point noise issues?

[ghost]

These are the specs I just added, anything I need to change before I test and commit?

it "handles modulus correctly" do
    (BigDecimal.new(13.0) % BigDecimal.new(4.0)).should eq(BigDecimal.new(1.0))
    (BigDecimal.new(13.0) % BigDecimal.new(-4.0)).should eq(BigDecimal.new(-3.0))
    (BigDecimal.new(-13.0) % BigDecimal.new(4.0)).should eq(BigDecimal.new(3.0))
    (BigDecimal.new(-13.0) % BigDecimal.new(-4.0)).should eq(BigDecimal.new(-1.0))
    (BigDecimal.new(11.5) % BigDecimal.new(4.0)).should eq(BigDecimal.new(3.5))
    (BigDecimal.new(11.5) % BigDecimal.new(-4.0)).should eq(BigDecimal.new(-0.5))
    (BigDecimal.new(-11.5) % BigDecimal.new(4.0)).should eq(BigDecimal.new(0.5))
    (BigDecimal.new(-11.5) % BigDecimal.new(-4.0)).should eq(BigDecimal.new(-3.5))
end

[bcardiff]

The DivisionByZeroError one should be added also.

I think they will all work directly thanks to the implementation, but better be safe.

I don't follow why you said: will that cause floating point noise issues?. The BigDecimal % implementation uses BigInt one. There are no floats involved I think.

[Sija]

Same overload could be added for Float perhaps?

[ghost]

Yeah, I'll add that. Thanks!

[ghost]

@bcardiff @Sija ok, I've committed both of the changes you suggested: the spec handles DivisionByZeroError, and there is an overload BigDecimal#%(other : Float).

[ghost]

Just realized that BigDecimal#**(BigDecimal) isn't a thing, so I added a fix and some quick specs for that and I'll group it in with this PR.

[bcardiff]

I think is good to go. But since you add BigDecimal#**(BigDecimal), aren't we also missing BigDecimal#**(Float)? It can be added later, just pointing out the lack of it.

Also, when I went to the integer division refactor I started leaving only the operations within the same type in the main file of the type, and moving to big/number.cr all the convenient operations to operate between different types. There is no need to change things on this, but I wanted to share the criteria.

[bcardiff]

Oh, please run the $ crystal tool format ./src/big/big_decimal.cr to make the CI happy. The formatter can be integrated with the editor of your preference.

[ghost]

Will do!

…

On Sat, Oct 3, 2020 at 8:35 PM Brian J. Cardiff ***@***.***> wrote: Oh, please run the $ crystal tool format ./src/big/big_decimal.cr to make the CI happy. The formatter can be integrated with the editor of your preference. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#9784 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AKXXAMTKYWRBESOCI2YZLRTSI67LVANCNFSM4SAJGELA> .

[asterite]

Please revert the ** changes. Please keep PR simple. I already reviewed this, now I need to review it again, it's much more work for reviewers. It's much simpler to send one PR per thing to discuss/add.

[ghost]

Oh, my bad. Will revert!

…

On Sun, Oct 4, 2020 at 6:34 AM Ary Borenszweig ***@***.***> wrote: ***@***.**** requested changes on this pull request. Please revert the ** changes. Please keep PR simple. I already reviewed this, now I need to review it again, it's much more work for reviewers. It's much simpler to send one PR per thing to discuss/add. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#9784 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AKXXAMVG4VPVUZC7XR33LRLSJBFU3ANCNFSM4SAJGELA> .

[bcardiff]

@sugarfi with the force push I think you discarded the changes requested in #9784 (comment)

[ghost]

@bcardiff sorry for the delay, was caught up with other stuff. The specs should be re-added now. Thank you for your patience!

[Sija]

This should be reopened, I guess.

[straight-shoota]

Hm, I don't think this reopening works. The original repo and branch appear to have completely vanished. And GitHub even fails to reconstruct the commit contents.
Even the commits are not accessible in this PR. But they are still available on the repo directly: https://github.com/crystal-lang/crystal/tree/ca391dd4e19f91c819aeff9b57b5efaf8c69ea8b

[Blacksmoke16]

Closing in favor of #13255.