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

frees-rpc Tagless-final Migration - Release 0.8.0 #117

Merged
merged 9 commits into from
Jan 11, 2018

Conversation

juanpedromoreno
Copy link
Member

This PR migrates frees-rpc entirely to tagless-final using Freestyle @tagless.

Considering all these changes as API breaking, it also releases a new minor version: 0.8.0.

@codecov-io
Copy link

codecov-io commented Jan 11, 2018

Codecov Report

Merging #117 into master will decrease coverage by 4.42%.
The diff coverage is 66.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #117      +/-   ##
==========================================
- Coverage      82%   77.57%   -4.43%     
==========================================
  Files          12       12              
  Lines         100      107       +7     
==========================================
+ Hits           82       83       +1     
- Misses         18       24       +6
Impacted Files Coverage Δ
modules/config/src/main/scala/client/config.scala 100% <ø> (ø) ⬆️
modules/internal/src/main/scala/client/calls.scala 100% <ø> (ø) ⬆️
modules/config/src/main/scala/server/config.scala 100% <ø> (ø) ⬆️
modules/server/src/main/scala/implicits.scala 0% <0%> (ø) ⬆️
...er/src/main/scala/handlers/GrpcServerHandler.scala 87.5% <100%> (+0.83%) ⬆️

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 b63c44a...f1945fa. Read the comment docs.

@juanpedromoreno juanpedromoreno requested a review from a team January 11, 2018 16:52
@free
trait GrpcServer {
@tagless
trait GrpcServer[F[_]] {
Copy link
Contributor

@diesalbla diesalbla Jan 11, 2018

Choose a reason for hiding this comment

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

You can still use the FS, and avoid adding the F[_].

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I know. IntelliJ shows less red code in this way.

Copy link
Contributor

@fedefernandez fedefernandez left a comment

Choose a reason for hiding this comment

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

Left minor comments, amazing job

@@ -19,7 +19,7 @@ package internal
package client

import freestyle.async.AsyncContext
import freestyle.free._
import freestyle.free.FSHandler
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use ~> instead?


@module
trait ChannelConfig {

val configM: ConfigM
implicit val functor: Functor
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor comment, what about putting this at the beginning?

implicit val functor: Functor

val configM: ConfigM
val defaultHost: String = "localhost"
val defaultPort: Int    = freestyle.rpc.server.defaultPort

trait Helpers {

def server[M[_]](implicit S: GrpcServer[M]): FreeS[M, Unit] = {
def server[F[_]: Monad](implicit S: GrpcServer[F]): F[Unit] = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can Monad be replaced by Functor?

Copy link
Contributor

@peterneyens peterneyens Jan 11, 2018

Choose a reason for hiding this comment

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

For the moment we need Monad for the for comprehension below.

We could require Apply and write S.start() *> S.getPort.void I think.
We could write something similar in the other places where Fede left this comment.

Copy link
Member Author

Choose a reason for hiding this comment

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

You also need flatMap

} yield ()
}

def serverStop[M[_]](implicit S: GrpcServer[M]): FreeS[M, Unit] = {
def serverStop[F[_]: Monad](implicit S: GrpcServer[F]): F[Unit] = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above

@@ -52,17 +47,17 @@ trait CommonUtils {

def createServerConf(grpcConfigs: List[GrpcConfig]): ServerW = ServerW(SC.port, grpcConfigs)

def serverStart[M[_]](implicit S: GrpcServer[M]): FreeS[M, Unit] = {
def serverStart[F[_]: Monad](implicit S: GrpcServer[F]): F[Unit] = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above

@@ -75,29 +95,30 @@ object Utils extends CommonUtils {
import service._
import freestyle.rpc.protocol._

class ServerRPCService[F[_]](implicit C: Capture[F], T2F: Task ~> F) extends RPCService[F] {
class ServerRPCService[F[_]](implicit F: Applicative[F], T2F: Task ~> F)
Copy link
Contributor

Choose a reason for hiding this comment

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

F[_]: Applicative ?

@@ -214,8 +215,10 @@ object TaglessUtils extends CommonUtils {
// Server Runtime Configuration //
//////////////////////////////////

implicit val taglessRPCHandler: TaglessRPCService.Handler[ConcurrentMonad] =
new TaglessRPCServiceServerHandler[ConcurrentMonad]
implicit def taglessRPCHandler[F[_]](
Copy link
Contributor

Choose a reason for hiding this comment

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

F[_]: Applicative ?

@@ -47,7 +46,7 @@ class GrpcServerTests extends RpcServerTestSuite {

"behaves as expected" in {

def program[F[_]](implicit APP: GrpcServer[F]): FreeS[F, Result] = {
def program[F[_]: Monad](APP: GrpcServer[F]): F[Result] = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above

case AwaitTerminationOp() => serverMock.awaitTermination()
}
}
def grpcServerHandlerTests[F[_]](implicit F: Applicative[F]): GrpcServer[F] = {
Copy link
Contributor

Choose a reason for hiding this comment

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

F[_]: Applicative

Copy link
Member Author

Choose a reason for hiding this comment

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

I preferred this way in order to use F as F.pure, rather than Applicative[F].pure.

@fedefernandez fedefernandez merged commit 372ba3a into master Jan 11, 2018
@fedefernandez fedefernandez deleted the jp-migrates-free-algebras-to-tagless branch January 11, 2018 17:29
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.

5 participants