-
Notifications
You must be signed in to change notification settings - Fork 37
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
marshallers as implicit params #330
marshallers as implicit params #330
Conversation
@@ -127,7 +127,8 @@ object AvroSrcGenerator extends SrcGenerator { | |||
|
|||
val serviceLines = | |||
if (requestLines.isEmpty) Seq.empty | |||
else Seq(s"@service trait ${protocol.getName}[F[_]] {", "") ++ requestLines :+ "}" | |||
else | |||
Seq(s"@service($serializationType) trait ${protocol.getName}[F[_]] {", "") ++ requestLines :+ "}" |
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.
The F[_]
parameter here, is it generated as a fresh symbol?
Macros that use fixed symbols for variable or types may end up shadowing or colliding with any type symbol in the context in which the macro is applied. This is related to macro hygiene.
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.
This AvroSrcGenerator
is a code generator, that generates a *.scala
file from an *.av[dl|pr]
, it doesn't do macros.
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 many examples, the @rpc
annotation is applied, without arguments, to all deferred methods in the @service
class. If that is the general case, we could avoid using it, and take all deferred methods in an @service
class as RPC methods.
The compression method, which is the only argument of the @rpc
annotation, could be given as another annotation @gzip
.
@@ -37,8 +33,7 @@ sealed abstract class CompressionType extends Product with Serializable | |||
case object Identity extends CompressionType | |||
case object Gzip extends CompressionType | |||
|
|||
class rpc(val serializationType: SerializationType, val compressionType: CompressionType = Identity) | |||
extends StaticAnnotation | |||
class rpc(val compressionType: CompressionType = Identity) extends StaticAnnotation |
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 we could just have the compression type as another annotation, e.g. @gzip
, instead of an argument of @rpc
.
Hm... I like the idea! It would also align the way however, for that change I'd like some consensus (since there can be corner cases for that decision). What do you think @fedefernandez @juanpedromoreno @L-Lavigne ? |
I like it, my vote is in favor of the @diesalbla suggestion |
Actually, with the code generation from protocols, we cannot use the gzip annotation given it is not part either So, I'm wondering if the gzip argument should be also part of the |
I like this a lot, great work @pepegar . 👍 @diesalbla I like the idea of getting rid of @rpc altogether; when you say "deferred methods" I guess you mean abstract? As for gzip, I'd also be in favor of moving it up into @service, possibly as its own annotation, unless we have real-world use cases of some methods being compressed and others not. I could see it happening, but then again the user could just split that into 2 services. |
Sounds good to me, @juanpedromoreno ! I think it makes much more sense to have it all in the |
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 added a minor suggestion, other than that, great job @pepegar !
@@ -37,8 +33,6 @@ sealed abstract class CompressionType extends Product with Serializable | |||
case object Identity extends CompressionType | |||
case object Gzip extends CompressionType | |||
|
|||
class rpc(val serializationType: SerializationType, val compressionType: CompressionType = Identity) | |||
extends StaticAnnotation | |||
@deprecated("Streaming type is now determined from request and response types", "0.14.0") |
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 we could remove the stream
, since we are no longer supporting the @rpc annotation
object ProtoRPCService {} | ||
object AvroRPCService {} | ||
object AvroWithSchemaRPCService {} | ||
|
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.
👍
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.
Great stuff! 👍
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.
Good job
This PR:
@service
macro take the serialization type as parameter (and removes it from@rpc
macro)$methodDescriptor
method on@service
annotated traits take marshallers for request and response as parametersFIXES #278