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 a few type classes to generated tuple instances #1527

Closed
wants to merge 2 commits into from

Conversation

edmundnoble
Copy link
Contributor

No description provided.

@peterneyens
Copy link
Collaborator

Should we add instances for CommutativeSemigroup, CommutativeGroup and BoundedSemilattice while we are at it?

@johnynek
Copy link
Contributor

johnynek commented Mar 6, 2017

We're going to need generators for our generators.

Are we getting the priorities correct? We want CommutativeX to be a higher priority than X.

It would be interesting if we had some better way to generalize over this.

@johnynek
Copy link
Contributor

johnynek commented Mar 9, 2017

I often wonder how long this approach is the right one and whether a shapeless based approach would be almost as fast and much cleaner to maintain.

@codecov-io
Copy link

Codecov Report

Merging #1527 into master will decrease coverage by 2.2%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1527      +/-   ##
==========================================
- Coverage   94.02%   91.82%   -2.21%     
==========================================
  Files         256      246      -10     
  Lines        4201     3754     -447     
  Branches       84      134      +50     
==========================================
- Hits         3950     3447     -503     
- Misses        251      307      +56
Impacted Files Coverage Δ
core/src/main/scala/cats/syntax/reducible.scala 40% <0%> (-60%) ⬇️
core/src/main/scala/cats/Monad.scala 50% <0%> (-46.16%) ⬇️
core/src/main/scala/cats/Apply.scala 55.55% <0%> (-33.34%) ⬇️
laws/src/main/scala/cats/laws/CategoryLaws.scala 66.66% <0%> (-33.34%) ⬇️
core/src/main/scala/cats/MonadWriter.scala 0% <0%> (-33.34%) ⬇️
core/src/main/scala/cats/data/package.scala 66.66% <0%> (-19.05%) ⬇️
core/src/main/scala/cats/instances/try.scala 78.94% <0%> (-17.42%) ⬇️
...ain/scala/cats/laws/discipline/CategoryTests.scala 83.33% <0%> (-16.67%) ⬇️
core/src/main/scala/cats/syntax/either.scala 83.92% <0%> (-15.21%) ⬇️
core/src/main/scala/cats/data/Cokleisli.scala 88.88% <0%> (-11.12%) ⬇️
... and 114 more

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 bbc97b8...7eb3e8d. Read the comment docs.

@edmundnoble
Copy link
Contributor Author

edmundnoble commented Jun 28, 2017

@johnynek What about Monoid and Semigroup for example, shouldn't they be different priorities too?

@peterneyens I added the three suggested as well.

Note this breaks bincompat, and so needs to be for the next release of cats-kernel, whenever that will be.

@edmundnoble edmundnoble changed the title Add generated CommutativeMonoid to tuple instances Add a few type classes to generated tuple instances Jun 28, 2017
@kailuowang
Copy link
Contributor

If there is concerns over breaking bin comp for kernel we can add these instances in cats.core right?

@johnynek
Copy link
Contributor

these are fine from algebird's point of view. We don't use these traits directly.

@johnynek
Copy link
Contributor

@edmundnoble yes, I think the priority issue might also be a problem for Semigroup. I think it was an oversight. We should probably have resolution tests to make sure things work as expected. Just using Tuple2 is probably enough.

@kailuowang
Copy link
Contributor

@edmundnoble I wonder if you have time to finish up this PR? Looks like we are close (1. fix the priority, and 2. add mima exception)

@kailuowang
Copy link
Contributor

Continued in #1967

@kailuowang kailuowang closed this Oct 13, 2017
@kailuowang kailuowang removed this from the 1.0.0-RC1 milestone Oct 13, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants