refined icon indicating copy to clipboard operation
refined copied to clipboard

Add BigDecimal instances for cats

Open FunFunFine opened this issue 2 years ago • 3 comments

This MR add missing Semigroup and Monoid instances for Pos/NegBigDecimal.

FunFunFine avatar May 04 '22 16:05 FunFunFine

Codecov Report

Merging #1088 (0fb7166) into master (636e505) will decrease coverage by 0.04%. The diff coverage is 100.00%.

:exclamation: Current head 0fb7166 differs from pull request most recent head b0eac29. Consider uploading reports for the commit b0eac29 to get more accurate results

@@            Coverage Diff             @@
##           master    #1088      +/-   ##
==========================================
- Coverage   92.64%   92.61%   -0.04%     
==========================================
  Files          63       63              
  Lines         829      839      +10     
  Branches       26        8      -18     
==========================================
+ Hits          768      777       +9     
- Misses         61       62       +1     
Impacted Files Coverage Δ
...c/main/scala/eu/timepit/refined/cats/package.scala 94.11% <100.00%> (+0.27%) :arrow_up:
...rc/main/scala/eu/timepit/refined/util/string.scala 0.00% <0.00%> (ø)
...main/scala/eu/timepit/refined/scodec/package.scala 100.00% <0.00%> (ø)
...n/scala/eu/timepit/refined/predicates/string.scala 100.00% <0.00%> (ø)
...n/scala/eu/timepit/refined/scalaz/derivation.scala 100.00% <0.00%> (ø)
...u/timepit/refined/shapeless/typeable/package.scala 100.00% <0.00%> (ø)
...rc/main/scala-3.0-/eu/timepit/refined/string.scala 98.79% <0.00%> (+0.02%) :arrow_up:

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

codecov[bot] avatar May 05 '22 10:05 codecov[bot]

Hi @fthomas , can you merge this MR please?

FunFunFine avatar Jun 05 '22 08:06 FunFunFine

@FunFunFine Could you add tests for these instances to https://github.com/fthomas/refined/blob/master/modules/cats/shared/src/test/scala/eu/timepit/refined/cats/CommutativeSemigroupAndMonoidLawTests.scala ?

fthomas avatar Jun 15 '22 13:06 fthomas

@fthomas it's been a while, but here it goes.

FunFunFine avatar Feb 13 '23 13:02 FunFunFine

Thanks @FunFunFine!

Can we also add commutativeSemigroup / commutativeMonoid tests like this:

checkAll("CommutativeSemigroup[PosBigDecimal]", CommutativeSemigroupTests[PosBigDecimal].commutativeSemigroup)

Or do they also fail because those instances are only approximately associative, too (like PosDouble for example)?

fthomas avatar Feb 13 '23 14:02 fthomas

The compilation fails with the following error when I try to add this test:

diverging implicit expansion for type org.scalacheck.Arbitrary[(scala.math.BigDecimal, eu.timepit.refined.numeric.Greater[shapeless._0])]
starting with method arbTuple2 in trait ArbitraryArities
  checkAll("CommutativeSemigroup[PosBigDecimal]", CommutativeSemigroupTests[PosBigDecimal].commutativeSemigroup)

I don't really understand what the problem is because it works for other types, but not for BigDecimal apparently.

FunFunFine avatar Feb 14 '23 09:02 FunFunFine

Oh, that fails because we lack Arbitrary instances for refined BigDecimal types. I'm fine with merging this without these tests then.

fthomas avatar Feb 14 '23 19:02 fthomas

Someone who has access to merging needs to merge it

FunFunFine avatar Feb 16 '23 10:02 FunFunFine