Correct Min.semigroup #651
Conversation
|
|
|
Hmm, build failure seems unrelated to the PR changes. |
|
Amazing. It looks like the monoid was correct and since all the numeric types use that we were saved. |
| property("Max.semigroup returns the maximum item") { | ||
| forAll { v: NonEmptyVector[Int] => | ||
| val maxItems = v.items.map { Max(_) } | ||
| v.sorted.last == Max.semigroup[Int].combineAllOption(maxItems).get.get |
There was a problem hiding this comment.
Can you also test this for a custom type that does not have a special instance defined?
Also, scala has .max on Iterable. Can we just use that rather than sorted.last?
| forAll { v: NonEmptyVector[Int] => | ||
| val minItems = v.items.map { Min(_) } | ||
| v.sorted.head == Min.semigroup[Int].combineAllOption(minItems).get.get | ||
| } |
Codecov Report
@@ Coverage Diff @@
## develop #651 +/- ##
===========================================
+ Coverage 82.83% 82.89% +0.05%
===========================================
Files 109 109
Lines 5209 5209
Branches 316 316
===========================================
+ Hits 4315 4318 +3
+ Misses 894 891 -3
Continue to review full report at Codecov.
|
|
👍 will merge when green. |
|
Thank you for taking the time to send a PR here. |
|
This was broken in 0.13.2 by me. :( I'm sorry y'all... I just hit this in tests trying to update to the latest algebird. At least our tests caught it.... Will publish 0.13.4 presently. |
Min.semigroupwas incorrectly returning the greater element.This PR corrects the semigroup and adds tests to check the results from both
Min.semigroupandMax.semigroup(which was already correct).