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
Adds coalgebroid-based constructors for Lens and Traversal #502
Conversation
@@ -27,6 +27,9 @@ class LensSpec extends MonocleSuite { | |||
val s = Lens[Example, String](_.s)(s => ex => ex.copy(s = s)) | |||
val p = Lens[Example, Point](_.p)(p => ex => ex.copy(p = p)) | |||
|
|||
implicit def ev = IndexedStoreT.storeTComonadStore[Id.Id, String] |
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.
what other instance of ComandStore
exists we could for lenses?
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 seems that scalaz contains only two instances of ComonadStore
: StoreT
and TracedT
. Honestly, I've never dealt with the second one, but it doesn't appear to be very practical for the case of Lenses (for instance, having copoint
using Monoid.zero
to feed the inner function seems odd). Given this situation, do you think it would be preferable to work directly with Store
?
@@ -251,6 +251,10 @@ object Lens { | |||
def codiagonal[S]: Lens[S \/ S, S] = | |||
PLens.codiagonal | |||
|
|||
/** creates a [[Lens]] from a Store comonad coalgebra */ | |||
def storeFCoalg[F[_], S, A](f: S => F[S])(implicit CS: ComonadStore[F, A]): Lens[S, A] = |
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.
does f
need to satisfy any law to create a lawful Lens
?
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.
As long as f
is a comonad coalgebra, classical lens laws should be satisfied (PutGet, GetPut and PutPut).
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.
what does it mean that f: S => F[S]
is a comonad coalgebra
?
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's a coalgebra that commutes with the comonad operations. The idea can be found here and especially here (The Algebraic Picture section). It seems that the comonad coalgebra laws have a direct correspondence with the lens ones.
As I said in the description, I've been toying around with the counterpart constructor for Traversals, but still don't know if traversal laws hold so nicely in that context (comonad coalgebras where FreeAp[Store[A, ?], ?]
is the functor).
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.
sorry for the delay, I was on holiday last week. I will try to go through the links you posted as fast as possible!
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.
what do you think of replacing ComonadStore by IndexedStore? It would make the relation with PTraversal.applyN more obvious.
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.
I like ComonadStore
, since it decouples particular representations, but given the lack of instances for it, I find your suggestion more than reasonable. Thereby, I've replaced this constructor by an IndexedStore
version and moved it to PLens
.
Thanks very much for links @Jeslg, in particular I wasn't aware of the relation between You PR looks good but I wonder if it is something we will ever want to use. I know that trying to figure out usage is real guess at best but do you see a use case where you would prefer to use |
Hi @julien-truffaut! I find that relation extremely beautiful 😄 In my case, I was working with several Store-coalgebra optics when I missed such a constructor. Then I saw However, I've been taking these ideas to Monocle's def applyN[S, T, A, B](f: S => FreeAp[IndexedStore[A, B, ?], T]) =
new PTraversal[S, T, A, B] {
def modifyF[F[_]: Applicative](g: A => F[B])(s: S): F[T] =
f(s).foldMap(λ[IndexedStore[A, B, ?] ~> F](is => g(is.pos).map(is.set)))
} If you think that this method does make sense, keeping the one for Lenses is reasonable, just to keep optic construction uniform. Thanks for your comments! |
One thing that annoys with
|
Hi there, I've been struggling with the compiler, because inference for Regarding mixing optional and required fields, I'm afraid this won't help. |
@julien-truffaut, here it is the aforementioned syntax. Now, it's possible to define a val ptraversalN: Traversal[ManyPropObject, Int] = PTraversal { obj =>
(obj.p1 ~ obj.p2 ~ obj.p4 ~ obj.p5 ~ obj.p6 ~ obj.p7 ~ obj.p8) {
ManyPropObject(_, _, obj.p3, _, _, _, _, _)
}
} Besides, it's also possible to define a one-focus val ptraversalN: Traversal[ManyPropObject, Int] = PTraversal { obj =>
obj.p1 { p1 => obj.copy(p1 = p1) }
} And what's more interesting, this works for def tuplePTraversal[A, B, C]: PTraversal[(A, A, C), (B, B, C), A, B] = PTraversal {
case (a1, a2, c) => (a1 ~ a2) { (_, _, c) }
} To do so, I've followed the scalaz pattern for |
it seems that kind-projector syntax is not supported in 2.10. If there is no easy fix, I would not worry too much as I think we should stop supporting 2.10. I really like that it generalises to |
I'm very glad you like it! 😄 The fix is trivial, but there seem to be other issues with traversal syntax in 2.10. Particularly, compiler is unable to infer val ptraversalN: Traversal[ManyPropObject, Int] = PTraversal { obj =>
(toTraversalBuilder(obj.p1) ~ obj.p2 ~ obj.p4 ~ obj.p5 ~ obj.p6 ~ obj.p7 ~ obj.p8) {
ManyPropObject(_, _, obj.p3, _, _, _, _, _)
}
} Any idea or workaround to improve type inference in this context? |
@Jeslg no sorry I don't know how to help type inference. I am fine if the test use manual invocation as we will need to add some doc on the website which use 2.12. |
Ok. Then, here it is! |
@@ -224,6 +224,12 @@ object PLens extends LensInstances { | |||
)(t => _.bimap(_ => t, _ => t)) | |||
|
|||
/** | |||
* create a [[PLens]] from a context (indexed store) coalgebra. | |||
*/ | |||
def contextCoalg[S, T, A, B](f: S => IndexedStore[A, B, T]): PLens[S, T, A, B] = |
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.
would storeCoalgebra
clearer than contextCoalg
?
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.
The thing is that after introducing IndexedStore
, f
is no longer a coalgebra (S => F[S]
). It becomes something different (S => F[T]
). There aren't many references about this thing, but at least in the lens context, it seems that the right name for f
should be coalgebroid. On the other hand, I know that IndexedStore
is also known as "context", but I don't know to what extent. Honestly, I don't like contextCoalg
much, so please, feel free to rename it to whatever you like.
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.
maybe something like fromStore
?
@@ -225,6 +226,13 @@ object PTraversal extends TraversalInstances { | |||
Applicative[F].apply6(f(get1(s)), f(get2(s)), f(get3(s)), f(get4(s)), f(get5(s)), f(get6(s)))(_set(_, _, _, _, _, _, s)) | |||
} | |||
|
|||
def apply[S, T, A, B](f: S => FreeAp[IndexedStore[A, B, ?], T]) = |
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.
shall we give it the same name that the Lens
constructor?
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.
I think that using the same name for both would be ok. Eventually, if users adopted Traversal
's constructor as default, it would be nice to provide an apply
alias for it.
👍 Thank you very much @Jeslg I just put a few comments regarding naming. You don't need to update the PR, I can rename it later if we agree on a new names. |
This includes an alternative constructor for lenses based on
Store
coalgebras. In fact, this constructor is even more generic, since it uses theStoreComonad
typeclass, wherepos
andpeek
can be found.(*) Similar constructors could be included for
PLens
(S => IndexedStore[A, B, T]
),Traversal
(S => FreeAp[Store[A, ?], S]
) andPTraversal
(S => FreeAp[IndexedStore[A, B, T]]
).