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.

Sign up for GitHub

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

Jump to bottom

Add minimumBy/maximumBy/Option to Foldable #3084

Merged
merged 1 commit into from
Sep 30, 2019
Merged

Add minimumBy/maximumBy/Option to Foldable #3084

merged 1 commit into from
Sep 30, 2019

Conversation

joroKr21
Copy link
Member

  • mimimumByOption / maximumByOption on Foldable
  • minumumBy / maximumBy on Reducible

Thank you for contributing to Cats!

Fixes #2385

@kailuowang
Copy link
Contributor

Let's add these to the type class. Now that we dropped 2.11 on master it's BC to add methods to trait. The syntax addition can be a PR to the Scala_2.11 branch

@joroKr21
Add syntax for minimumBy/maximumBy/Option
449d54f 
  * `mimimumByOption` / `maximumByOption` on `Foldable`
  * `minumumBy` / `maximumBy` on `Reducible`
@joroKr21
Copy link
Member Author

Looks like unrelated failure:

[info] - CommutativeGroup[BigDecimal].commutativeGroup.associative *** FAILED ***
[info]   GeneratorDrivenPropertyCheckFailedException was thrown during property evaluation.
[info]    (Discipline.scala:12)
[info]     Falsified after 39 successful property evaluations.
[info]     Location: (Discipline.scala:12)
[info]     Occurred when passed generated values (
[info]       arg0 = -4.420570652140785E+214,
[info]       arg1 = -5.873313741220067E+186,
[info]       arg2 = 5.83376395628893E+188
[info]     )
[info]     Label of failing property:
[info]       Expected: -4.420570652140784999999999942249692E+214
[info]   Received: -4.420570652140784999999999942249691E+214

@codecov-io
Copy link

Codecov Report

Merging #3084 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3084      +/-   ##
==========================================
+ Coverage   93.49%   93.49%   +<.01%     
==========================================
  Files         368      368              
  Lines        6979     6983       +4     
  Branches      184      195      +11     
==========================================
+ Hits         6525     6529       +4     
  Misses        454      454
Impacted Files Coverage Δ
core/src/main/scala/cats/Foldable.scala 100% <100%> (ø) ⬆️
core/src/main/scala/cats/Reducible.scala 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c56edd7...449d54f. Read the comment docs.

kailuowang
kailuowang previously approved these changes Sep 25, 2019
View reviewed changes
Copy link
Contributor

@kailuowang kailuowang left a comment

Choose a reason for hiding this comment

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

Lgtm thanks!

@kailuowang kailuowang changed the title Add syntax for minimumBy/maximumBy/Option Add minimumBy/maximumBy/Option to Foldable Sep 25, 2019
kailuowang
kailuowang reviewed Sep 25, 2019
View reviewed changes
tests/src/test/scala/cats/tests/FoldableSuite.scala
@@ -192,6 +192,20 @@ abstract class FoldableSuite[F[_]: Foldable](name: String)(implicit ArbFInt: Arb
}
}

test(s"Foldable[$name].maximumBy/minimumBy") {
Copy link
Contributor

Choose a reason for hiding this comment

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

sorry I should've mentioned this in the earlier comment. Can we make these laws like the other _Ref laws in Foldable Laws and Reducible laws?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm I'm not sure what you mean 🤔 couldn't find _Ref anywhere in the file.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah ok I found them. Ok I will add some laws. Should I keep the tests in the suite?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think having them in law is enough. sorry I should've been clear about where the law code is.

Copy link
Member Author

@joroKr21 joroKr21 Sep 29, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Hmm, if I want to add tests for minimum/maximum to FoldableTests that would mean adding an Order constraint on B in def foldable[A: Arbitrary, B: Arbitrary], which is not binary compatible.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, right. Then we can't add to laws. We can only use the current tests

kailuowang
kailuowang approved these changes Sep 29, 2019
View reviewed changes
Copy link
Contributor

@kailuowang kailuowang left a comment

Choose a reason for hiding this comment

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

Thanks! Lgtm

@kailuowang kailuowang merged commit 87cd258 into typelevel:master Sep 30, 2019
@joroKr21 joroKr21 deleted the min-max-by branch October 2, 2019 21:21
@travisbrown travisbrown mentioned this pull request Oct 28, 2019
Merged
kailuowang pushed a commit that referenced this pull request Oct 28, 2019
@travisbrown @kailuowang
Follow-up #3084 (#3122)
007155f 
* Remove type class constraint from the type class's methods

* Fix documentation

* Change names for consistency
@travisbrown travisbrown added this to the 2.1.0-RC1 milestone Nov 6, 2019
@travisbrown travisbrown mentioned this pull request Nov 13, 2019
Closed
22 tasks
gagandeepkalra added a commit to gagandeepkalra/cats that referenced this pull request Feb 16, 2020
@gagandeepkalra
backported typelevel#3084 Add minimumBy/maximumBy/Option to Foldable
4db14ad
travisbrown pushed a commit that referenced this pull request Feb 19, 2020
@gagandeepkalra @travisbrown
backported #3084 Add minimumBy/maximumBy/Option to Foldable
d278b04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LukaJCB LukaJCB LukaJCB approved these changes

@kailuowang kailuowang kailuowang approved these changes

Assignees
No one assigned
Labels
enhancement
Projects
None yet
Milestone
2.1.0-RC1
Development

Successfully merging this pull request may close these issues.

Add Foldable#[max|min]ByOption, Reducible#[maxBy|minBy]
5 participants
@joroKr21 @kailuowang @codecov-io @LukaJCB @travisbrown