-
-
Notifications
You must be signed in to change notification settings - Fork 206
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
Syntactic sugar for ad-hoc creation and use of lenses. #208
Conversation
(draft, needs review)
I initially called |
If I understand correctly, class GenApplyLens[A] {
def apply[B](field: A => B): ApplyLens[A, B] = ???
}
object GenApplyLens {
def apply[A] = new GenApplyLens[A]
} Btw, I am not really keen on the name Could you show me in the PR where is the bit about |
I tried adding it to ApplyLens. One problem I remember was that core doesn't depend on the macros project, where I thought the macros should go. I think there was another problem I currently don't recall. |
Assignment syntax for updates with member access syntax | ||
E.g. someobj.focus.a.b = ... | ||
*/ | ||
def updateDynamic[C](field: String)(value: C): S = macro internal.FocusImpl.updateDynamicImpl[S,C] |
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 implements the assignment syntax like someobj.focus.a.b = ... (requires whitebox, i.e. no code completion in IntelliJ, but maybe nicer syntax)
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 would prefer to avoid adding any white box macros and any feature where it is difficult to track what is going on (e.g. Dynamic
). Would you consider it acceptable to add a symbolic alias for set
in ApplyX
, we might be able to use :=
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.
Just to be clear, the whitebox macro and Dynamic are both not needed for the assignment syntax. They are only needed for the .focus.a.b
field syntax (selectDynamic for building the lens, updateDynamic for assignment). If you use .focus(_.a.b)
you need neither and get code completion in IntelliJ.
I agree to be careful with these features, but since we have a syntactically more verbose, but better supported "fallback", I think it is ok in this case. It means we are providing slightly more risky, fully optional syntactic sugar, which we can deprecate if we have to. Also, having worked closely with @odersky and @xeno-by for a few years, I doubt they are going away any time soon if ever.
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.
And generally, I am not a big fan of similar looking, but differently behaving syntax like :=, if it can be avoided. But looks like this isn't even needed here.
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 definitively prefer an adhoc symbol like :=
or |->
than a dynamic dispatch as user can easily see in the code to what it refers to.
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 I didn't know this feature. I tried on a basic case class but it does not compile:
case class Foo(i: Int){
def update(newI: Int): Foo = copy(i = newI)
}
val foo = Foo(3)
foo = 5
Foo(6) = 5
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.
case class Foo(i: Int){
def update(newI: Int): Foo = copy(i = newI)
}
val foo = Foo(3)
foo() = 5
=> Foo(5)
case class Bar(i: Int){
def update(oldICheck: Int, newI: Int): Bar = if(i == oldICheck) copy(i = newI) else this
}
val bar = Bar(3)
bar(3) = 5
=> Bar(5)
bar(2) = 7
=> Bar(3)
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 is me or it is quite hidden scala feature? I haven't seen it anywhere but I might have completely missed it.
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 wouldn't say it's a hidden feature, just used with the mutable part of scala, which I suppose we don't touch often. Scala's Array is implemented with this.
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've been aware of it for a long time, but had never used it. What I found slightly surprising, but great and logical is that assignments are expressions, i.e. can return something other than Unit. Which makes them really great syntax for lens updates
How about BoundLens as a name? It indicates that the lens is bound to a particular value. The name is nice, but too long for casual syntax in my opinion. And Any details of the PR I should explain more? Any preferences among the syntactic variants? /cc @milessabin Just FYI |
I didn't get your comment regarding For the moment, this is the syntax I prefer: GenX(john)(_.address.streetNumber).set(45)
GenX(john)(_.address.streetNumber) := 45 // eventually where I think it is important that the macro generates an import monocle.std.option.some
case class Address(streetNumber: Int, country: Option[String])
(GenX(john)(_.address.country) composePrism some).set("UK") I will ask in the gitter group for people to share their opinion and also thank you @cvogt for the proposal :) |
ApplyLens is in core. Focus has methods that are macros and should be in macro, so I couldn't add those methods to ApplyLens.
wouldn't need any whitebox or Dynamic magic. I'd be happy with that if we replace GenX by something short, simple, intuitive. Such as
I'd still like |
@NightRa @julien-truffaut Let's try to come up with an action plan here to get this out :). How about merging Focus and ApplyLens and calling it "BoundLens" and moving it into the macro package? And how about supporting the following two syntaxes for creating bound lenses? Blackbox Whitebox/Dynamic All ApplyLens operations would still work on them. Additionaly .update would be added to support =. |
I think it might be good idea to have a black box macro that generates an I am off for 10 days, it will good to have other people participating but if we cannot come up with a good compromise, maybe the best will be to have Lens dsl project that will use the syntax you propose. We can then put links in the Monocle readme toward this project. |
The fact that it is implemented as a macro is of secondary importance for users I think. It is actually more of an implementation detail leaking if it is reflected in the name users call. I don't see right now how standardizing names among macros alone would provide benefits to the users. I'd be ok dropping the Whitebox macro/Dynamic. .update is a standard Scala feature and even though it wasn't know to you, I think it is the best syntax available and no magic required, except for Scala built-in magic. I would like to see .update Store for a type that wraps a value and a corresponding lens? That name doesn't give away anything about it's purpose right now to me. Maybe there is some larger picture in which it does. BoundLens on the other hand is very descriptive about what it does. |
I believe that the fact that Foucs is a macro is very relevant to the users. Macro are still tagged as experimental, implementation is likely to change drastically with Scala meta and if I am not mistaken it requires an extra dependency. For all these reasons, I don't consider macro as first class construct for Monocle but rather a secondary syntax that simplify the creation of Optics. On another side, I believe that the usefulness of a library is inversely promotional to the number of concepts it introduces e.g. scalaz stream mostly define 1 concept: Process and haskell lens go even further where all optics are simply type alias. In Monocle, I didn't find a way to do to better than one class per Optic and it creates a lot of duplication and boiler plate. That's why I am reluctant to add Focus as a new data type. Regarding update or dynamic, it is not because I freshly learnt something that I don't want to use it (it tends to be the contrary). I just don't like this sort of syntactic sugar because I find that it makes a piece of code more complicated to understand. Finally, I think that it is more important to keep existing naming convention, especially if a concept already exist in Math or in another language. For example, I prefer to have Monoid or Iso even though these names didn't talk to me in the first place. Most if not all of my comments are related to design and are completely subjective. I am not saying that I am right or wrong and I am sure I will change my opinion on some of them,thats why I would like more people to participate. Also I would like to be clear that I really support your initiative but the syntax that you wish may not belong to Monocle. Recently they were a couple of very short lens libraries (e.g. Quick lens) that tries to solve your use case. Sorry if I am not responsive in the next couple of days. |
Hi @cvogt, |
let's do that. I reopen when I am ready |
@julien-truffaut Are you going to be at the Philly Typelevel summit? Would be a good place to chat about this. We have been using a variant of this very successfully for quite some time now in the x.ai code base and it would be great to finally contribute it back. And maybe give a lightning talk about it ;) |
Hi @cvogt, unfortunately I will not be at Philly typelevel submit, it is bit far away from the uk :( Regarding your proposal, I think it would nice to package it separately from monocle-core, so either a separate module or a separate project. It would give us more room to experiment and get feedbacks, once it settle down we can decide to move it to core or somewhere else. WDYT? |
@julien-truffaut uk, too bad :). So here is the ENTIRE code we ended up with. It's tiny (but super useful to us), so I'll just post it here and we can discuss where to best put it. The only problem with integrating it back into Monocle is that the current Monocle project structure splits macros and ApplyLens into separate projects IIRC, but I need them both. It works well stand-alone though. The code is so small that I am not sure if it is worth it putting it somewhere separate if we can solve the problem, but I let you be the judge: // usage example
import ai.x.generic.BoundLens.ImplicitBoundLens
case class Outer( inner: Inner, s: String )
case class Inner( i: Int )
val o = Outer(Inner(5), "foo")
o.lens(_.s).set("bar")
.lens(_.inner.i).modify(_+1) // .lens calls can be chained nicely
// implementation
package ai.x.generic
import scala.reflect.macros.blackbox.Context
import scala.language.experimental.macros
import monocle._
import monocle.syntax.ApplyLens
object BoundLens {
def apply[S]( value: S ) = new BoundLens( ApplyLens[S, S, S, S]( value, Lens.id ) )
implicit class ImplicitBoundLens[T]( v: T ) {
def lens = BoundLens( v )
}
}
/** Syntactic sugar around an ApplyLens */
class BoundLens[S, A]( val applyLens: ApplyLens[S, S, A, A] ) extends AnyVal {
def apply[C]( field: A => C ): BoundLens[S, C] = macro BoundLensMacros.apply[S, A, C]
def set( value: A ): S = applyLens.set( value )
def modify( diff: A => A ): S = applyLens.modify( diff )
}
object BoundLensMacros {
def apply[S, A: c.WeakTypeTag, C]( c: Context )( field: c.Expr[A => C] ) = {
import c.universe._
c.Expr[BoundLens[S, C]]( q"""
new _root_.ai.x.generic.BoundLens(
${c.prefix.tree}.applyLens composeLens
_root_.monocle.macros.GenLens[${c.weakTypeOf[A]}](${field})
)
""" )
}
} |
I am not sure many people saw your post since it is in a closed PR. My personal opinion would be to release this code in a separate project and advertise it in the readme / website. I know it creates some overhead to setup such project but I think it makes sense because this syntax is specific to lenses and quite different from the syntax already present in monocle. I also like the idea of small dependencies that's why I am thinking about ways to make Please tell me if I can be of any help. |
Published it here https://github.com/xdotai/lens . It's not on maven yet, but I am happy to do so. Anyone, just open an issue if you would like me to. |
awesome, I will add a section on the website to advertise it :) |
Great :) On March 8, 2016 12:51:42 PM EST, Julien Truffaut notifications@github.com wrote:
|
This is my take on syntax I promised. I currently have 3 alternatives for creating "Focus" objects, which are my macro-enabled variant of ApplyLens. Focus(obj), focus(obj), obj.focus. I like the last best, but it requires an implicit conversion on an unconstrained type, which may not be desirable, hence the 1st and 2nd version. We may want to get rid of 1 or 2 of them.
Also I implemented support for the assignment operator (.update / .updateDynamic).
Selecting fields directly on Focus is also nice, but requires a whitebox macro, which is why there is a non-whitebox fallback, which is .apply and passing a function that does the member access.
Personally my most common use case for lenses would be
john.focus.age = 45
or if workign in IntelliJ maybejohn.focus(_.age) = 45
. This syntax would make my life much easier.WDYT?