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

Replace @free with @tagless, and drop the requirement of an annotation #92

Merged

Conversation

tbrown1979
Copy link

@tbrown1979 tbrown1979 commented Nov 28, 2017

This PR drops the @free annotation and replaces it with @tagless. This PR also removes the requirement that a @service be annotated with @tagless. You are free to define it with your own parameterized type if you want.

This resulted in some changes to the generated code because we're no longer converting from Op ~> M anymore.

@tbrown1979 tbrown1979 changed the title Remove free requirement Remove @free requirement for a @service Nov 28, 2017
@tbrown1979 tbrown1979 changed the title Remove @free requirement for a @service [WIP] Remove @free requirement for a @service Nov 28, 2017
case cls: Trait =>
defaultServiceExtras(cls.name, cls)
case _ =>
abort(s"$invalid. $abstractOnly")
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe a better error here since @free isn't required anymore


val (algName, template) = defn match {
lazy val (algName, template) = defn match {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think these all need to be lazy.

val call: Term.Tuple = streamingType match {
case Some(RequestStreaming) =>
q"""
def call(callType: Option[String]): Term.Tuple = {
Copy link
Author

@tbrown1979 tbrown1979 Nov 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Passing an Option[String] for the callType is kind of a weird way to determine which call to use - unaryMethod/unaryMethodFree.


case class ServiceAlg(defn: Defn) extends RPCService {
val typeParam: Type.Param = defn match {
case c: Class => c.tparams.head
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.head

@codecov-io
Copy link

codecov-io commented Nov 30, 2017

Codecov Report

Merging #92 into master will decrease coverage by 0.11%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #92      +/-   ##
=========================================
- Coverage   85.71%   85.6%   -0.12%     
=========================================
  Files          12      12              
  Lines         126     125       -1     
=========================================
- Hits          108     107       -1     
  Misses         18      18
Impacted Files Coverage Δ
rpc/src/main/scala/internal/service/calls.scala 81.48% <100%> (-0.67%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bd93550...5eb74a2. Read the comment docs.

@tbrown1979 tbrown1979 changed the title [WIP] Remove @free requirement for a @service Replace @free with @tagless, and drop the requirement of an annotation Nov 30, 2017
@juanpedromoreno
Copy link
Member

This is great. I'll look at it tomorrow, thanks @tbrown1979!

Copy link
Contributor

@raulraja raulraja left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent work @tbrown1979. This is a great step forward. RPC services didn't need to be encapsulated in @free because the Handlers they provide can already be implemented in terms of Free programs if the target M[_] is a FreeS[Op, ?] so the extra level of indirection was not buying us anything in the protocol algebra.

Copy link
Member

@juanpedromoreno juanpedromoreno left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great to me, outstanding job! Thanks @tbrown1979

@juanpedromoreno juanpedromoreno merged commit f4fa7cf into higherkindness:master Dec 1, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants