Adds logical and fixed types to code generation #340
Conversation
Codecov Report
@@ Coverage Diff @@
## master #340 +/- ##
==========================================
+ Coverage 84.02% 85.20% +1.18%
==========================================
Files 29 29
Lines 1909 1954 +45
Branches 26 28 +2
==========================================
+ Hits 1604 1665 +61
+ Misses 305 289 -16
Continue to review full report at Codecov.
|
dzanot
changed the title
build.sbt
Outdated
| "io.circe" %% "circe-parser" % "0.13.0", | ||
| "io.circe" %% "circe-yaml" % "0.13.1", | ||
| "org.scalameta" %% "scalameta" % "4.3.22", | ||
| "com.julianpeeters" %% "avrohugger-core" % "1.0.0-RC22" % Test, |
There was a problem hiding this comment.
I removed avro-compiler in favor of avrohugger-core as the latter with remove imported structures from the resulting org.apache.avro.Protocol structure.
This caused some incompatibility issue with the scala-collection-compat library so I removed that dependency as well which didn't cause any apparent failures.
| options = Nil, | ||
| declarations = proto.types.map(toMu), | ||
| services = List( | ||
| val services = if (proto.messages.isEmpty) { |
There was a problem hiding this comment.
This is to prevent an empty service when there are no rpc methods defined in the avdl
| def checkGenerated(idlName: String) = { | ||
| val idlResourceName = s"avro/${idlName}.avdl" | ||
| val idlUri = Try(getClass.getClassLoader.getResource(idlResourceName).toURI) | ||
| .getOrElse(throw new Exception(s"No avdl found for name: ${idlName}")) |
There was a problem hiding this comment.
Just a nitpick for a few places in this file: if I don't have a specific exception to throw I like to shorten throw new Exception to sys.error. Also some of the interpolated strings here don't need curly brackets.
There was a problem hiding this comment.
Changed both of these
| case Right(protocol) => protocol | ||
| } | ||
| val avroProto = avroProtos | ||
| .find(p => p.getName == idlName) |
There was a problem hiding this comment.
Another nitpick: we can probably combine this check with the collect partial function above (changing it to collectFirst).
There was a problem hiding this comment.
good spot, changed this
| @@ -89,53 +112,41 @@ class AvroSpec extends Specification with ScalaCheck { | |||
| t"Stream[$f, $a]" | |||
| } | |||
|
|
|||
| val actual = codegen.protocol(muProto, streamCtor).right.get | |||
| val actual = codegen.protocol(muProto, streamCtor).fold(s => throw new Exception(s), identity) | |||
There was a problem hiding this comment.
Perhaps we could wrap expected in an Either Right so we don't need to extract actual before comparing them.
There was a problem hiding this comment.
@dzanot I see this wasn't applied so just curious, would there be issues with such a change? Perhaps it would hinder the custom comparison message in which case we'd probably have to keep the test as-is.
There was a problem hiding this comment.
This actually changed but the test is in the new AvroProtocolSpec.scala file here https://github.com/higherkindness/skeuomorph/pull/340/files#diff-791c60e0123c350ef43edff0c1b159887e0163be800c85b7b700494cab9edfa7R117
Not sure why github is still showing this one though
There was a problem hiding this comment.
Ah OK thanks. Looks like we're still doing right.get on actual though: https://github.com/higherkindness/skeuomorph/pull/340/files#diff-791c60e0123c350ef43edff0c1b159887e0163be800c85b7b700494cab9edfa7R69
There was a problem hiding this comment.
Ah yeah I think this is because we are calling .isEqual for comparison. I tried refactoring so both actual and expected were Either, but using the specs2 shouldEqual matcher is using different equality. I can keep poking to see if I can configure specs2 it to use the scala meta isEqual
There was a problem hiding this comment.
Ah no worries then, it would be a nice to have but not worth spending too much time on IMO.
|
@L-Lavigne I've addressed your comments, and made some changes since merging latest master. Can you take another look? |
|
Sorry @L-Lavigne ... I removed some unused imports and dismissed your approval |
dzanot
added
enhancement
This is the first step to replacing avro hugger code generation with skeuomorph codegen in
sbt-mu-srcgenoutlined in this ticket.It adds the logical types that are supported in the
1.10.0avro compiler, as well as modifying theMuF.TByteArraytype to include the info of the avrofixedtypes.