New approach for pretty printers #17
Conversation
| */ | ||
|
|
||
| package skeuomorph | ||
| package freestyle |
There was a problem hiding this comment.
We might want to create some tickets to rename this and also moving this project to the hk organization (under higherkindness.io group id). Makes sense?
There was a problem hiding this comment.
Yep, I'll tackle those in #18
src/main/scala/freestyle/print.scala
Outdated
| """.stripMargin | ||
| case TProduct(name, fields) => | ||
| val printFields = fields.map(f => s"${f.name}: ${f.tpe}").mkString(", ") | ||
| s"@message case class $name($printFields)" |
src/main/scala/freestyle/print.scala
Outdated
| def service[T](implicit T: Basis[FreesF, T]): Printer[Service[T]] = | ||
| ((konst("@service(") *< serializationType >* konst(") trait ")) >*< | ||
| (string >* konst("[F[_]] {") >* newLine) >*< | ||
| (mkList(operation, "\n") >* newLine >* konst("}"))).contramap(serviceTuple) |
There was a problem hiding this comment.
There was a problem hiding this comment.
Or at least, I'd suggest for extracting that out to a val
pepegar
force-pushed
the
new-approach-for-pretty-printers
branch
from
6fb6621
to
8a2d213
Compare
|
|
||
| import cats.Contravariant | ||
|
|
||
| trait Divisible[F[_]] extends Contravariant[F] { |
There was a problem hiding this comment.
Is this related to Ed Kmett's Divisible library? If it is, then a similar thing has already been added to cats, under the inconspicuous name of ContravariantMonoidal.
There was a problem hiding this comment.
Yep, it's a port of kmett's Divisible indeed. But ContravariantMonoidal is lacking divide right?
There was a problem hiding this comment.
It is not lacking it. It just so happens that, in a very curious leap of abstraction, they use Semigroupal[F].product to represent both the divide method from the Divisible type-class, and the product (the old |@|) from the Applicative class.
There was a problem hiding this comment.
For the same token, the method conquer has a very similar signature to ContravariantMonoidal.trivial. Nevertheless, even if you base the code of cats, It may be easier to keep the Divisible syntax.
There was a problem hiding this comment.
Yep, I'm keeping the syntax but dropping the typeclass in favor of ContravariantMonoidal. Thanks for pointing me to it Diego!
src/main/scala/Printer.scala
Outdated
| import cats.syntax.compose._ | ||
| import cats.instances.function._ | ||
|
|
||
| case class Printer[A](print: A => String) |
There was a problem hiding this comment.
Was there a reason for using a case class? Libraries like circe usually write the encoders or printers as traits with the abstract method:
trait Printer[A]{
def print(a: A): String
def contraMap[B](f: B => A): Printer[B]
}There are some benefits of these approach. First, you save a bit of efficiency, but turning two objects (case class and functions) into a single one. Also, in Scala 2.12, thanks to the SAM syntax, you can assign a lambda to it
val print: Printer[Person] = { x => s"${x.name}, ${x.age} years." } On another matter: should we make A contravariant, trait Printer[- A]?
There was a problem hiding this comment.
Does it make it more performant? I didn't know it but I like your idea and the fact that we can benefit from SAM, I'm changing Printer to a trait.
I don't see the benefit of explicitly making the type param contravariant, I don't want to encourage subtyping :)
| def print(a: A): String = f(a) | ||
| } | ||
|
|
||
| def konst(str: String): Printer[Unit] = |
There was a problem hiding this comment.
I think that, with SAM, this is just
def konst(str: String): Printer[Unit] = _ => strThere was a problem hiding this comment.
I can't use SAM in the library, since I crosscompile it to 2.11 too :(
src/main/scala/Printer.scala
Outdated
| def optional[A](p: Printer[A]): Printer[Option[A]] = | ||
| Printer(_.fold("")(p.print)) | ||
|
|
||
| def mkList[A](p: Printer[A], sep: String): Printer[List[A]] = |
There was a problem hiding this comment.
Following the naming of monadic parsing libraries (which is just the reverse), I would call this sepBy.
src/main/scala/Printer.scala
Outdated
| def konst(str: String): Printer[Unit] = | ||
| Printer(Function.const(str)) | ||
|
|
||
| def space = konst(" ") |
There was a problem hiding this comment.
This def can be val. Since it is a public method, I would add the type of it.
src/main/scala/Printer.scala
Outdated
|
|
||
| def space = konst(" ") | ||
|
|
||
| def newLine = konst("\n") |
|
|
||
| def newLine = konst("\n") | ||
|
|
||
| val string: Printer[String] = Printer(identity) |
There was a problem hiding this comment.
Could we add a val empty: Printer[Unit] = _ => ""?
src/main/scala/Printer.scala
Outdated
| implicit val divisiblePrinter: Decidable[Printer] = new Decidable[Printer] { | ||
| def contramap[A, B](fa: Printer[A])(f: B => A): Printer[B] = | ||
| Printer(f >>> fa.print) | ||
| def conquer[A]: Printer[A] = |
There was a problem hiding this comment.
This can refer the empty printer from the previous comment.
| Printer(_.fold("")(p.print)) | ||
|
|
||
| def mkList[A](p: Printer[A], sep: String): Printer[List[A]] = | ||
| Printer { |
There was a problem hiding this comment.
Again, by SAM, you do not need to add the Printer here. It will take the pattern-match as a literal for PartialFunction, which in Scala is a Function (with some controversy I know).
There was a problem hiding this comment.
I can't introduce SAM inside the lib because crosscompiling to 2.11...
clone of #14 . I don't really know when I closed it :)