-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Adding FiniteDuration instances #2544
Adding FiniteDuration instances #2544
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2544 +/- ##
==========================================
+ Coverage 95.18% 95.19% +0.01%
==========================================
Files 359 361 +2
Lines 6558 6575 +17
Branches 285 289 +4
==========================================
+ Hits 6242 6259 +17
Misses 316 316
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we also add these to cats-core? :)
@LukaJCB Yes! I'll get those added |
I'm sorry I think worded myself incorrectly, what I meant was, that we should re-export the instances in You can look at other instances in kernel to see how it's done. |
Ah, yeah I think that’s my bad. I probably should have thought of that before duplicating. I’ll get that fixed here :) |
@LukaJCB There we go, this should look a little better. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should also be added here: https://github.com/typelevel/cats/blob/master/core/src/main/scala/cats/instances/package.scala
as well as here: https://github.com/typelevel/cats/blob/master/core/src/main/scala/cats/instances/all.scala#L49
@LukaJCB Thank you, I got those added |
Anyone more familiar with this repo than me, why is the CodeCov pipeline failing? Am I missing some tests? |
@calvinbrown085 You're not testing any of the instances for their laws. Check the |
@@ -14,6 +14,7 @@ trait AllInstances | |||
with EqInstances | |||
with EitherInstances | |||
with DurationInstances | |||
with FiniteDurationInstances |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is binary incompatible, you should create another AllInstancesBinCompat0
trait here, and then mix that into all
. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, that might be what I am missing :) Thank you
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Of course :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, thank you! :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
closes #2517