-
Notifications
You must be signed in to change notification settings - Fork 21
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
Add helper methods to parse config directly into an F[_] #29
Add helper methods to parse config directly into an F[_] #29
Conversation
This uses ApplicativeError[F[_], Throwable], which is available on many cats types such as IO, Either[Throwable, ?], etc.
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.
Thanks a lot. This looks awesome. I have a few suggestions on how to improve the integration.
ConfigFactory.load().as[C].leftWiden[Throwable].raiseOrPure[F] | ||
|
||
def loadConfigF[F[_], C : Decoder](path : String)(implicit ev : ApplicativeError[F, Throwable]) : F[C] = | ||
ConfigFactory.load().as[C](path).leftWiden[Throwable].raiseOrPure[F] |
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 way ConfigFactory.load()
is used here means that load error won't be caught. I suggest moving these methods to the syntax
module either named asF
or loadF
. That way they can piggy-bag on the existing config parsers and simply focus on conversions for ApplicativeError
.
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.
To clarify, my suggest is to add the following code to io.circe.config.syntax.CirceConfigOps
:
import cats.ApplicativeError
import cats.implicits._
/**
* Read config settings into the specified container type.
*/
def asF[F[_], C: Decoder](implicit ev : ApplicativeError[F, Throwable]) : F[C] =
as[C].leftWiden[Throwable].raiseOrPure[F]
/**
* Read config settings at given path into the specified container type.
*/
def asF[F[_], C: Decoder](path : String)(implicit ev : ApplicativeError[F, Throwable]) : F[C] =
as[C](path).leftWiden[Throwable].raiseOrPure[F]
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.
Ah you're totally right, I get it now. So if there's a syntax error in application.conf
or something, there will be a parse exception that won't get caught by .as
.
Ok, I'll think about it a bit and add your changes. Thanks!
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.
Added this in 6d76b92
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.
So I took one more stab at single function usage in 8dd4315. I do like the idea of the user not having to import ConfigFactory
. Let me know what you think. I can wrap parseResources
too if that helps, or i can just remove loadConfigF.
@@ -70,6 +72,18 @@ class CirceConfigSpec extends FlatSpec with Matchers { | |||
assert(AppConfig.as[Nested]("e") == Right(Nested(true))) | |||
} | |||
|
|||
type ErrorHandler[A] = Either[Throwable, A] // Typically this would be IO or something similar |
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 suggest to bring in cats-effect
as a test dependency so we can use it in the tests below.
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.
Added cats effect and more tests in 6d76b92
ce59907
to
5ab1f57
Compare
Revise loadConfigF test to use IO
5ab1f57
to
8dd4315
Compare
let me know if i can do anything to help. I can rebase this into one commit so we can have a fresh look on it, or expand documentation and testing? Let me know your feeling. |
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.
A few comments.
import org.scalatest.{ FlatSpec, Matchers } | ||
import cats.effect.IO | ||
import cats.implicits._ | ||
import com.typesafe.config.ConfigMemorySize |
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 import
is not needed due to the wildcard import below.
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 see, fixed in a8ad180
@@ -54,6 +58,11 @@ import scala.collection.JavaConverters._ | |||
* }}} | |||
*/ | |||
package object config { | |||
def loadConfigF[F[_], C : Decoder](implicit ev : ApplicativeError[F, Throwable]) : F[C] = | |||
Either.fromTry(Try(ConfigFactory.load())).flatMap(_.as[C]).leftWiden[Throwable].raiseOrPure[F] |
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.
Either.catchNonFatal
looks more optimal here since it doesn't have to create a Try
.
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.
As a second note, I understand the point of wanting to load the default configuration to avoid importing com.typesafe.config.ConfigFactory
. However, I'm not sure whether this method belongs in the parser
module, which already calls out to ConfigFactory.parse*
. I might move the loading + leftWiden
there so this call becomes parser.load().asF[C]
.
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.
Thanks for your comment. I was able to move the load logic into parser. And now that I'm more familiar with parser, I see what you're doing in there, and I was able to do the load
operations more in correspondence with the other methods. Hopefully that makes sense.
5a65aeb
to
4a51ef4
Compare
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.
...
Update README example
95cde00
to
a8ad180
Compare
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.
Note package.scala is left unchanged by this PR
5c8ea02
to
cab6c14
Compare
* Remove pureconfig in favor of circe-config This duplicates some work we expect to see integrated in circe/circe-config#29, and we'll have to revisit this if that gets released.
Thanks a lot. I'll play a bit with it and release a new version over the weekend! |
Very cool. thanks for your review! |
This uses
ApplicativeError[F[_], Throwable]
, which is available on many cats types such asIO
,Either[Throwable, ?]
, etc.So now you can just do:
Inspired from pureconfig with cats-effect
Obviously this is just for the case of loading from
reference.conf
orapplication.conf
as done in typesafe config.