Skip to content
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

Merged
merged 19 commits into from Jun 30, 2018

Conversation

@pepegar
Copy link
Member

commented Jun 26, 2018

This PR:

  • makes the @service macro take the serialization type as parameter (and removes it from @rpc macro)
  • makes the $methodDescriptor method on @service annotated traits take marshallers for request and response as parameters

FIXES #278

@@ -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 :+ "}"

This comment has been minimized.

Copy link
@diesalbla

diesalbla Jun 26, 2018

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.

This comment has been minimized.

Copy link
@pepegar

pepegar Jun 26, 2018

Author Member

This AvroSrcGenerator is a code generator, that generates a *.scala file from an *.av[dl|pr], it doesn't do macros.

Copy link

left a comment

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

This comment has been minimized.

Copy link
@diesalbla

diesalbla Jun 26, 2018

I think that we could just have the compression type as another annotation, e.g. @gzip, instead of an argument of @rpc.

@pepegar

This comment has been minimized.

Copy link
Member Author

commented Jun 26, 2018

Hm... I like the idea! It would also align the way @services are defined with what @tagless or @free do in Freestyle.

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 ?

@fedefernandez

This comment has been minimized.

Copy link
Collaborator

commented Jun 26, 2018

I like it, my vote is in favor of the @diesalbla suggestion

@juanpedromoreno

This comment has been minimized.

Copy link
Member

commented Jun 26, 2018

Actually, with the code generation from protocols, we cannot use the gzip annotation given it is not part either avro nor protocolbuff standards, which it would be a limitation in my opinion.

So, I'm wondering if the gzip argument should be also part of the @service annotation, together with the SerializationType.

@higherkindness higherkindness deleted a comment from delvr Jun 26, 2018
@L-Lavigne

This comment has been minimized.

Copy link

commented Jun 26, 2018

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.

@pepegar

This comment has been minimized.

Copy link
Member Author

commented Jun 26, 2018

Sounds good to me, @juanpedromoreno ! I think it makes much more sense to have it all in the @service annotation. I'll go do it tomorrow and update this PR!

Copy link
Member

left a comment

I added a minor suggestion, other than that, great job @pepegar !

Oustanding job

@@ -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")

This comment has been minimized.

Copy link
@juanpedromoreno

juanpedromoreno Jun 29, 2018

Member

I think we could remove the stream, since we are no longer supporting the @rpc annotation

object ProtoRPCService {}
object AvroRPCService {}
object AvroWithSchemaRPCService {}

This comment has been minimized.

Copy link
@juanpedromoreno
@L-Lavigne L-Lavigne self-requested a review Jun 29, 2018
Copy link

left a comment

Great stuff! 👍

Copy link
Collaborator

left a comment

Good job

@juanpedromoreno juanpedromoreno merged commit a11252e into release/0.14.0 Jun 30, 2018
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@juanpedromoreno juanpedromoreno deleted the ppg/marshallers-as-implicit-params branch Jun 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.