Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add BigDecimal instances for cats #1088

Merged
merged 5 commits into from
Feb 16, 2023
Merged

Add BigDecimal instances for cats #1088

merged 5 commits into from
Feb 16, 2023

Conversation

FunFunFine
Copy link

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

@codecov
Copy link

codecov bot commented May 5, 2022

Codecov Report

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

❗ 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%) ⬆️
...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%) ⬆️

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

@FunFunFine
Copy link
Author

Hi @fthomas , can you merge this MR please?

@mergify mergify bot added the cats label Sep 29, 2022
@FunFunFine
Copy link
Author

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

@fthomas
Copy link
Owner

fthomas commented Feb 13, 2023

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)?

@FunFunFine
Copy link
Author

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.

@fthomas
Copy link
Owner

fthomas commented Feb 14, 2023

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

@FunFunFine
Copy link
Author

Someone who has access to merging needs to merge it

@fthomas fthomas merged commit 667c42e into fthomas:master Feb 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants