-
Notifications
You must be signed in to change notification settings - Fork 15
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
Migrate from turtles to droste #2
Conversation
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 a few, small, stylish, comments. LGTM.
src/main/scala/protobuf/schema.scala
Outdated
val printSymbols = symbols.map({ case (s, i) => s"$s = $i;" }).mkString("\n") | ||
val printAliases = aliases.map({ case (s, i) => s"$s = $i;" }).mkString("\n") | ||
|
||
s""" |
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.
Here, you could use stripMargin
?
s"""
|enum $name {
| $printOptions
| $printSymbols
| $printAliases
|}
""".stripMargin
src/main/scala/protobuf/schema.scala
Outdated
case TMessage(name, fields, reserved) => | ||
val printReserved = reserved.map(l => s"reserved " + l.mkString(", ")).mkString("\n ") | ||
def printOptions(options: List[Option]) = | ||
if (options.isEmpty) { |
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.
No need for braces in this if-then-else.
src/main/scala/protobuf/schema.scala
Outdated
.map(f => s"""${f.tpe} ${f.name} = ${f.position}${printOptions(f.options)};""") | ||
.mkString("\n ") | ||
s""" | ||
message $name { |
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.
You can use stripMargin
here, too.
src/main/scala/protobuf/schema.scala
Outdated
|
||
val printFields = | ||
fields | ||
.map(f => s"""${f.tpe} ${f.name} = ${f.position}${printOptions(f.options)};""") |
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, for this line, a single-quoted string will suffice.
proto.name, | ||
proto.types.map(toFreestyle), | ||
proto.messages.map( | ||
msg => |
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 extract this as a def toOperation
function.
docs/src/main/tut/optimizations.md
Outdated
```scala | ||
def nestedNamedTypesTrans[T](implicit T: Basis[Schema, T]): Trans[Schema, Schema, T] = Trans { | ||
case TProduct(name, fields) => | ||
TProduct[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.
Here, I would extract the function of the map
as a def
, so as to get:
def ttt(f: Field[T]) = f.copy(tpe = namedTypes(T)(f.tpe))
TProduct[T](name, fields.map(tt) )
case TProduct(name, fields) => | ||
TProduct[T]( | ||
name, | ||
fields.map { f: Field[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.
Same as the comment for the optimize.md
file.
invariants.toList.mkString("Cop[", " :: ", ":: TNil]") | ||
case TSum(name, fields) => | ||
val printFields = fields.map(f => s"case object $f extends $name").mkString("\n ") | ||
s""" |
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.
Another use for stripMargin
.
""" | ||
case TProduct(name, fields) => | ||
val printFields = fields.map(f => s"${f.name}: ${f.tpe}").mkString(", ") | ||
s""" |
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 the next three lines be the same as the following line?
"@message case class $name($printFields)"
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.
Indeed, good catch! ;)
src/main/scala/protobuf/schema.scala
Outdated
if (options.isEmpty) { | ||
"" | ||
} else { | ||
options.map(printOption).mkString(" [", ", ", "]") |
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.
In the mkString
, I had some problem reading apart each quote and comma. It may be worth using named parameters:
mkString( start = " [" , sep = ", " , end = "]" )
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.
👍
docs/src/main/tut/optimizations.md
Outdated
``` | ||
|
||
```tut | ||
ast[Mu[Schema]].transCataT(namedTypes) | ||
val optimization = Optimize.namedTypes[Fix[Schema]] |
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.
You should be able to use Mu
still, if you want.
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.
Yeah, IDK why I didn't see it in droste :) Thanks!
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.
LGTM
Use droste for recursion schemes