-
-
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
Add empty to State and StateT objects #2533
Conversation
Seems like there's still a bincompat issue for 2.11 |
Ah, right. 2.11 doesn't like methods added to traits. But this should be fine with an exception, no? |
We can add new methods by creating a new trait and mixing it in, similar to what we've been doing with the syntax additions :) |
Smart. I've updated the commit accordingly. |
Codecov Report
@@ Coverage Diff @@
## master #2533 +/- ##
==========================================
+ Coverage 95.17% 95.17% +<.01%
==========================================
Files 359 359
Lines 6553 6555 +2
Branches 278 286 +8
==========================================
+ Hits 6237 6239 +2
Misses 316 316
Continue to review full report at Codecov.
|
And we're green! |
@@ -223,6 +223,11 @@ object IndexedStateT extends IndexedStateTInstances with CommonStateTConstructor | |||
IndexedStateT(_ => F.map(fsb)(s => (s, ()))) | |||
} | |||
|
|||
private[data] trait CommonStateTConstructors0 extends CommonStateTConstructors { | |||
def empty[F[_], S, A: Monoid](implicit F: Applicative[F]): IndexedStateT[F, S, S, A] = | |||
pure(Monoid[A].empty) |
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.
Is there a reason to use : Monoid
for A
but not use : Applicative
for F
? I'd be inclined to not use the syntactic sugar for either so you can avoid do something like F.pure(A.empty)
and avoid an extra Monoid.apply[A]
call.
@ceedubs Is this fine with you now? |
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.
@larsrh I think that it's probably good, but I left a couple of questions. Let me know what you think. I'm okay with approving in its current form.
@@ -223,6 +223,11 @@ object IndexedStateT extends IndexedStateTInstances with CommonStateTConstructor | |||
IndexedStateT(_ => F.map(fsb)(s => (s, ()))) | |||
} | |||
|
|||
private[data] trait CommonStateTConstructors0 extends CommonStateTConstructors { |
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.
Is there any particular reason to introduce this new trait as opposed to just putting this method directly into the object
?
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.
Binary compatibility: #2533 (comment)
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.
To be precise, the function is not only provided on the IndexedStateT
object, but also on StateT
. This is the reason why it has to go into a trait in the first place.
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.
Oh okay that makes sense. Thanks for the explanation.
Co-authored-by: Lars Hupel <[email protected]>
@ceedubs Updated accordingly. |
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 @larsrh!
Work by @sderosiaux in #2294. Rebased to current master. For some reason, the bincompat error also went away (at least locally).