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

Issue 2657 - Ambiguous duration instances #2670

Merged
merged 9 commits into from
Jan 9, 2019
Merged

Issue 2657 - Ambiguous duration instances #2670

merged 9 commits into from
Jan 9, 2019

Conversation

barambani
Copy link
Contributor

This is to address #2657

@@ -9,12 +9,12 @@ package object instances {
object byte extends ByteInstances
object char extends CharInstances
object double extends DoubleInstances
object duration extends DurationInstances
object duration extends CoreDurationInstances with DurationInstances
Copy link
Contributor Author

@barambani barambani Dec 16, 2018

Choose a reason for hiding this comment

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

I'm still trying to solve an issue here as importing both cats.instances.duration._ and cats.instances.finiteDuration._ explicitly prevents for the instance to be resolved. The scenario is when there's the need for kernel instances together with core's

object test {

  import cats.instances.duration._
  import cats.instances.finiteDuration._

  implicitly[Order[Duration]]

  implicitly[ContravariantShow[FiniteDuration]]
}

Copy link
Contributor

Choose a reason for hiding this comment

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

This is why we don't have variance in type classes. I'd say in this case user have to use workarounds.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I found a way to mitigate the problem. Now the only case that would fail is the below (see these test cases as an example).

test("fails") {
  import cats.instances.duration._
  import cats.instances.finiteDuration._

  implicitly[ContravariantShow[Duration]] // fails compilation
  implicitly[ContravariantShow[FiniteDuration]] // is inferred
}

I couldn't find a way to remove also this drawback.

@codecov-io
Copy link

codecov-io commented Dec 16, 2018

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2670      +/-   ##
==========================================
+ Coverage   95.12%   95.13%   +<.01%     
==========================================
  Files         364      365       +1     
  Lines        6707     6756      +49     
  Branches      285      300      +15     
==========================================
+ Hits         6380     6427      +47     
- Misses        327      329       +2
Impacted Files Coverage Δ
testkit/src/main/scala/cats/tests/CatsSuite.scala 70% <ø> (ø) ⬆️
core/src/main/scala/cats/instances/duration.scala 100% <100%> (ø) ⬆️
...src/main/scala/cats/instances/finiteDuration.scala 100% <100%> (ø) ⬆️
core/src/main/scala/cats/syntax/parallel.scala 70.58% <0%> (-9.42%) ⬇️
...estkit/src/main/scala/cats/tests/ListWrapper.scala 100% <0%> (ø)

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 b329cf1...294b2e8. Read the comment docs.

@barambani
Copy link
Contributor Author

I don't think I can do more here without breaking the BC. It's anyway a step forward compared to the current situation. If you agree, I would consider this PR complete.

LukaJCB
LukaJCB previously approved these changes Jan 3, 2019
Copy link
Member

@LukaJCB LukaJCB left a comment

Choose a reason for hiding this comment

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

Thanks very much @barambani :)

@barambani
Copy link
Contributor Author

👍

@kailuowang kailuowang added the bug label Jan 8, 2019
@kailuowang kailuowang added this to the 1.6 milestone Jan 8, 2019
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 a lot! @barambani

@kailuowang kailuowang merged commit d88b3c4 into typelevel:master Jan 9, 2019
@barambani
Copy link
Contributor Author

👍

@barambani barambani deleted the issue-2657 branch January 9, 2019 21:07
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.

4 participants