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 Comparison to cats package #2110

Merged
merged 2 commits into from
Dec 15, 2017
Merged

add Comparison to cats package #2110

merged 2 commits into from
Dec 15, 2017

Conversation

kailuowang
Copy link
Contributor

fixes #2109

@kailuowang kailuowang changed the title add Comparison to Cats add Comparison to Cats package Dec 15, 2017
@kailuowang kailuowang changed the title add Comparison to Cats package add Comparison to cats package Dec 15, 2017
@LukaJCB LukaJCB mentioned this pull request Dec 15, 2017
LukaJCB
LukaJCB previously approved these changes Dec 15, 2017
@codecov-io
Copy link

codecov-io commented Dec 15, 2017

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2110      +/-   ##
==========================================
+ Coverage   94.79%   94.79%   +<.01%     
==========================================
  Files         325      325              
  Lines        5494     5495       +1     
  Branches      219      218       -1     
==========================================
+ Hits         5208     5209       +1     
  Misses        286      286
Impacted Files Coverage Δ
core/src/main/scala/cats/package.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 5234312...3b21ad6. Read the comment docs.

@iravid
Copy link
Contributor

iravid commented Dec 15, 2017

Thanks @kailuowang! Should the type be exported as well?

@@ -90,6 +90,7 @@ package object cats {
val Eq = cats.kernel.Eq
val PartialOrder = cats.kernel.PartialOrder
val Order = cats.kernel.Order
val Comparison = cats.kernel.Comparison
Copy link
Contributor

Choose a reason for hiding this comment

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

do we not need the type Comparison?

@kailuowang
Copy link
Contributor Author

I don't see much of the benefit of aliasing Comparison type. Unlike existing aliases, it's not a type class. The original request in #2109 is to do pattern match on comparison without an extra import, the current change should suffice for that requirement right?

a comparison b match {
   case Comparison.GreaterThan => ...
   ...
}

Of course if we want to pass the comparison result around we would need the type, but how often is that? I was just trying to do the minimum (and minimize namespace pollution) to satisfy the original request.

@johnynek
Copy link
Contributor

I think it is inconsistent to be able to do def getEq: cats.kernel.Comparison = Comparison.Equal why does one have the kernel, but not the other?

@iravid
Copy link
Contributor

iravid commented Dec 15, 2017

For pattern matching accessing the data constructors is indeed sufficient. I don’t have a concrete usecase for passing around a comparison currently but it looks inconsistent with the rest of the kernel re-exports.

Namespace Pollution is an interesting consideration but I’m not familiar with the trade offs here. My desire is to stick with the cats, cats.data and cats.implicits imports :-)

for the sake of consistency
@kailuowang
Copy link
Contributor Author

Consistency is a good argument. I added the type.

Copy link
Contributor

@iravid iravid left a comment

Choose a reason for hiding this comment

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

Thanks again!

@LukaJCB
Copy link
Member

LukaJCB commented Dec 15, 2017

👍

@LukaJCB LukaJCB merged commit 5255077 into master Dec 15, 2017
@stew stew removed the in progress label Dec 15, 2017
@kailuowang kailuowang added this to the 1.0.0-RC2 milestone Dec 18, 2017
@kailuowang kailuowang deleted the fix-2109 branch December 19, 2017 01:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cats.kernel.Comparison not exported by the cats package object
6 participants