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

Decouple frees config #300

Merged
merged 8 commits into from
Jun 6, 2018
Merged

Decouple frees config #300

merged 8 commits into from
Jun 6, 2018

Conversation

pepegar
Copy link
Member

@pepegar pepegar commented Jun 5, 2018

In this PR I:

  • create a bare minimum ConfigM tagless to be used wherever freestyle.tagless.config were used before
  • remove a couple of @module annotations
  • bump frees-todolist-libto v0.8.1
  • bump sbt-freestyle to 0.3.23

FIXES #294

@pepegar pepegar requested a review from juanpedromoreno June 5, 2018 17:22
@pepegar pepegar changed the title Ppg/decouple frees config Decouple frees config Jun 5, 2018
@L-Lavigne L-Lavigne self-requested a review June 5, 2018 21:45
Copy link
Contributor

@L-Lavigne L-Lavigne left a comment

Choose a reason for hiding this comment

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

Great work! Could you update the docs accordingly? The code samples are executed via tut and are currently failing the build (https://travis-ci.org/frees-io/freestyle-rpc/jobs/388386394). Thanks!

We need to make the calls to `ConfigForAddress` with an F that has
`Sync`, so we're making them with `IO` now.
def apply[F[_]](implicit S: Sync[F], C: ConfigM[F]): ChannelConfig[F] = new ChannelConfig[F] {
def sync = S
def configM = C
}
Copy link
Member

Choose a reason for hiding this comment

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

I'd suggest the following approach, more natural perhaps?

class ChannelConfig[F[_]](implicit S: Sync[F], C: ConfigM[F]) {

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

  def loadChannelAddress(hostPath: String, portPath: String): F[ChannelForAddress] =
    for {
      config <- C.load
      host <- S.pure(
        Either
          .catchOnly[Missing](config.getString(hostPath)))
      port <- S.pure(Either.catchOnly[Missing](config.getInt(portPath)))
    } yield ChannelForAddress(host.getOrElse(defaultHost), port.getOrElse(defaultPort))

  def loadChannelTarget(targetPath: String): F[ChannelForTarget] =
    for {
      config <- C.load
      target <- S.pure(Either.catchOnly[Missing](config.getString(targetPath)))
    } yield ChannelForTarget(target.getOrElse("target"))

}

object ChannelConfig {
  def apply[F[_]](implicit S: Sync[F], C: ConfigM[F]): ChannelConfig[F] = new ChannelConfig[F]

  implicit def defaultChannelConfig[F[_]](implicit S: Sync[F], C: ConfigM[F]): ChannelConfig[F] =
    apply[F]
}

trait ServerConfig {
trait ServerConfig[F[_]] {

implicit def sync: Sync[F]
Copy link
Member

Choose a reason for hiding this comment

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

Same here

Copy link
Member

@JesusMtnez JesusMtnez left a comment

Choose a reason for hiding this comment

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

LGTM

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.

Looks great, I've left an optional suggestion. Thanks


trait ConfigM[F[_]] {
def load: F[Config]
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm wondering if we could add a new method to this algebra:

def get[A](from: (Config) => A): F[Either[Missing, A]]

With the implementation:

def get[A](getValue: => A): F[Either[Missing, A]] =
  S.pure(Either.catchOnly[Missing](getValue))

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 think that, at some point in the near future, we'll reintroduce frees-config, and we could maybe add it there? this implementation is temporary. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good

@pepegar pepegar merged commit 40a338e into master Jun 6, 2018
@pepegar pepegar deleted the ppg/decouple-frees-config branch June 6, 2018 13:08
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.

6 participants