The argument methodNameStyle of service macro should default to "Unchanged" #93
Comments
|
Hi @lilac, thanks for filing this issue! This is an interesting request, I was under the impression that idiomatic gRPC services being capitalized was the expected behavior for those services; in fact there was an issue filed about a year ago about this very topic, and based on that discussion the assumption was that capitalizing the method names was how we should go about generating them. That said, after digging around on the internet about it seems like your request is perfectly reasonable; we shouldn't necessarily mandate that all idiomatic endpoints have capitalized method names, and I think providing the option to make them unchanged while still providing the namespace and all that good stuff is something that mu could reasonably support. I don't know if I'd be comfortable changing the default behavior (since changing defaults is a recipe for breaking things), but I'm definitely open to the idea of allowing mu users to specify both idiomatic endpoints and unchanged method names. Does that sound amenable to you? |
|
@dmarticus Thanks for the reply. Having read through the issue you mentioned, I did not get a hint of why method names of an idiomatic endpoint should be capitalized. In addition, I think most grpc code generators and tools make the method names unchanged, just like you type in proto files. That's why I think it should be the default behavior of mu-srcgen. In addition, since this plugin has not reached v1.0, this surprising behavior should be rectified ASAP to avoid more technical debt. However, if a break change is not expected at this stage, adding another option is acceptable to me. |
|
@lilac My pleasure! And yeah, I should've clarified that the discussion didn't explain why the previous development decided to capitalize it, just that they did. I'm more than open to seeing if they'd be willing to change the default since we're not at v1.0.0, but either way I definitely agree that this option should exist. I'll ask a few of the original engineers about this, too. @pepegar @fedefernandez @cb372 what do you think about this proposed change? |
|
I'm fine with that breaking change. We'd want to add the possibility to customize that option. |
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.
By default, when the "muSrcGenIdiomaticEndpoints" setting is true, the generated service has "methodNameStyle = Capitalize", this is a surprise to most grpc developers.
In addition, if muSrcGenIdiomaticEndpoints = false, the generated service does not have namespace set. Thus, there is not option available that allows us to have namespace and unchanged method names.
When we use protobuf as a rpc protocol, we often use some protobuf tools as well, like grpcui, or BloomRPC to test grpc services. However, due to the fact that grpc services generated by mu-srcgen is not compatible to its proto file, those tools do not work anymore.
In conclusion, we expect the generated grpc service to be like the following, to comply with conventions of grpc ecosystem.
The text was updated successfully, but these errors were encountered: