Stop setting methodNameStyle = Capitalize when useIdiomaticEndpoints is true
#342
Conversation
This PR addresses [sbt-mu-srcgen #93](higherkindness/sbt-mu-srcgen#93) along with an upcoming code-change PR for `sbt-mu-srcgen` and a documentation update PR for `mu-scala`. It does so by splitting the `useIdiomaticEndpoints` source-generation flag, created in [mu-scala #599](higherkindness/mu-scala#599) to control service method capitalization and package prefixes, into 2 distinct flags with different defaults: 1) `useIdiomaticGrpc` which controls the namespace prefix and defaults to `true`; 2) `useIdiomaticScala` which controls the endpoint capitalization and defaults to `false`. More formally, if `useIdiomaticScala` is true and if _all_ the RPC call definitions in an input IDL are capitalized, the Scala methods in the services generated from this IDL will be _de_-capitalized, while the gRPC endpoints derived from those will be re-capitalized to match the original IDL. For example, a Protobuf IDL `rpc GetBook` definition becomes a Scala `def getBook` method inside a `methodNameStyle = Capitalized` service, which then generates a `GetBook` gRPC endpoint. A `getBook` IDL definition (for example from Avro) would remain unchanged (`methodNameStyle = Unchanged`, `def getBook`, and `getBook` gRPC endpoint). The reason for the limitation that all RPC calls must be capitalized for the option to apply, is that we do not annotate individual methods, only the top-level `@service` annotation. Thus if we were to de-capitalize a mixed-capitalization group of calls and apply `methodNameStyle = Capitalized` to the service, some of the originally-lowercased methods would be incorrectly re-capitalized which is part of the issue we're addressing here (for example, `getBook` and `GetBook` would both become `def getBook` Scala and `GetBook` endpoints). Note that the PR also moves some test resources into the `resource` root, and normalizes them a bit between the Avro and Proto cases. I'm happy to provide some more background on the "idiomatic" flag's history as I understand it, in case it helps in reviewing this change. Feel free to ask about this or anything else unclear.
L-Lavigne
added
enhancement
Codecov Report
@@ Coverage Diff @@
## master #342 +/- ##
==========================================
+ Coverage 83.90% 84.02% +0.11%
==========================================
Files 29 29
Lines 1908 1909 +1
Branches 27 26 -1
==========================================
+ Hits 1601 1604 +3
+ Misses 307 305 -2
Continue to review full report at Codecov.
|
| package higherkindness.skeuomorph | ||
|
|
||
| object StringUtils { | ||
| def decapitalize(s: String): String = if (s.isEmpty) s else s"${s.head.toLower}${s.tail}" |
There was a problem hiding this comment.
Could we use s.capitalize?
| @@ -29,7 +29,7 @@ import org.apache.avro.Schema | |||
| import org.apache.avro.Schema.Type | |||
| import higherkindness.droste.{Algebra, Coalgebra} | |||
|
|
|||
| import scala.collection.JavaConverters._ | |||
| import scala.jdk.CollectionConverters._ | |||
There was a problem hiding this comment.
I'm invoking @dmarticus here, as he has been experiencing some issues lately. I'm not sure if this causing binary issues in the sbt plugin side.
There was a problem hiding this comment.
What up! I've been invoked!
Anyway, JP is right in that making this change will cause binary compat issues on the sbt plugin (specifically the sbt-mu-srcgen plugin) side, which is super obnoxious but unfortunately is something we'll have to live with until Davis finishes the work migrating away from avrohugger. Chris sums it the reasons why we have bincompat issues excellently in this comment, but a quick TL;DR is that avrohugger defines its own scala.jdk.CollectionConverters object, which means the object with the same name provided by scala-collection-compat is invisible.
In the short term, this means that we can't get rid of these 2.13 warnings until we migrate off avrohugger, but since both Lawrence and I have tried in different PRs to skeuomorph, I'm gonna write a quick issue explaining this problem and then tie that issue to the completion of the avrohugger migration task so we can make sure that we address this obnoxious warning as soon as we can.
There was a problem hiding this comment.
Thanks @dmarticus for the explanation. Not sure why AvroHugger decided to clash that namespace, but in any case I've reverted the Converters change for now.
There was a problem hiding this comment.
This might still be blocked after my task is done. I'm still planning on using avrohugger to generate the org.apache.avro.Protocol objects while then piping the resulting Protocols into skeuomorph. The reason for this is because avrohugger does stuff to strip out imported protocols from the final Protocol, something the native org.apach.avro.idl.Idl parser does not do which results in a giant protocol containing all imported definitions.
There was a problem hiding this comment.
I've found that adding avrohugger as a test dependency to skeuomorph is actually causing bin incompat issues in the tests... so back to the drawing board 😩
|
Maybe a silly question, but do we really need these flags? I don't see any reason to ever disable
This seems like an awful lot of work just to avoid a capital letter at the start of a method name. Do we have any evidence that our users actually care about this? Maybe we can reduce complexity by being a bit more opinionated and less configurable? |
@cb372 After some reflection I agree about both flags. For As for
Thoughts? |
|
So to summarise,
Sounds good to me, assuming my understanding is correct. |
|
@cb372 I removed |
L-Lavigne
changed the title
methodNameStyle = Capitalize when useIdiomaticEndpoints is true
Depends on higherkindness/skeuomorph#342. Also cleans up the Model by removing unused classes, and updates the tests and documentation.
This PR addresses sbt-mu-srcgen #93 along with an upcoming code-change PR for
sbt-mu-srcgenand a documentation update PR formu-scala.The main functional change is that we no longer set
methodNameStyle = CapitalizewhenuseIdiomaticEndpointsis true. The flag gets the default value oftruefor clarity.It does so by splitting theuseIdiomaticEndpointssource-generation flag (created in mu-scala #599 to control service method capitalization and package prefixes), into 2 distinct flags with different defaults:1)useIdiomaticGrpcwhich controls the namespace prefix and defaults totrue;2)useIdiomaticScalawhich controls the endpoint capitalization and defaults tofalse.More formally, ifuseIdiomaticScalais true and if all the RPC call definitions in an input IDL are capitalized, the Scala methods in the services generated from this IDL will be de-capitalized, while the gRPC endpoints derived from those will be re-capitalized to match the original IDL. For example, a Protobuf IDLrpc GetBookdefinition becomes a Scaladef getBookmethod inside amethodNameStyle = Capitalizedservice, which then generates aGetBookgRPC endpoint. AgetBookIDL definition (for example from Avro) would remain unchanged (methodNameStyle = Unchanged,def getBook, andgetBookgRPC endpoint).The reason for the limitation that all RPC calls must be capitalized for the option to apply, is that we do not annotate individual methods, only the top-level@serviceannotation. Thus if we were to de-capitalize a mixed-capitalization group of calls and applymethodNameStyle = Capitalizedto the service, some of the originally-lowercased methods would be incorrectly re-capitalized which is part of the issue we're addressing here (for example,getBookandGetBookwould both becomedef getBookScala andGetBookendpoints).Note that the PR also updates the documentation, moves some test resources into the
resourceroot, splits and renames tests between Schema, Protocol, and Cats laws classes, and normalizes the tests a bit between the Avro and Proto cases.I'm happy to provide some more background on the "idiomatic" flag's history as I understand it, in case it helps in reviewing this change. Feel free to ask about this or anything else unclear.