Add a combine method to ConfigErrors #173
Conversation
|
Looks like MiMa is complaining about bincompat. What's the correct way to deal with this? |
|
Thanks for this! Very neat idea. Definitely worth adding something to the usage guide about this, but I can do that later unless you want to. Binary compatibility fails because adding a function with default implementation to a trait in Scala 2.11 breaks compatibility. We can keep compatibility by moving the new instance to a |
Codecov Report
@@ Coverage Diff @@
## master #173 +/- ##
==========================================
+ Coverage 99.76% 99.77% +<.01%
==========================================
Files 46 47 +1
Lines 868 870 +2
Branches 9 10 +1
==========================================
+ Hits 866 868 +2
Misses 2 2
Continue to review full report at Codecov.
|
|
Thanks, moving it to a separate trait did the trick. I'll work on adding something to the user guide. |
|
I've added an example to the docs. I had to enable I'll investigate later, but I'm pretty confused by this. |
There was a problem hiding this comment.
Left a few comments, but already looks pretty great! 👍
The kittens compile error message isn't very helpful. If we tweak the code slightly, we can get a more helpful error message.
diverging implicit expansion for type cats.derived.MkShow[Config]
starting with method refTypeViaContravariant in package cats
semi.show[Config]
^
Compilation Failed
It looks like derived Show instances for refined types are provided both via an explicit Show derivation and via Contravariant derivation. I'll try to create to create a smaller example and open an issue against refined.
In the meantime, changing this import:
import eu.timepit.refined.cats._
to:
import eu.timepit.refined.cats.refTypeShow
should make things compile.
Edit: refined issue is here: fthomas/refined#553
| different types of configuration. | ||
|
|
||
| ```tut:silent | ||
| case class DbConfig(user: String, password: String) |
There was a problem hiding this comment.
The password and apiKey should probably be Secret[String].
| We load each of the configurations using `loadConfig`: | ||
|
|
||
| ```tut:invisible | ||
| sys.props.put("db.user", "myapp") |
There was a problem hiding this comment.
Can we do a sys.props.remove on these at the very end? Or they'll stick around for the duration of the running JVM. (Ideally we want a try-finally thing for these, but AFAIK nothing like that exists in tut.)
| * res0: ConfigErrors = ConfigErrors(ConfigError(error1), ConfigError(error2)) | ||
| * }}} | ||
| */ | ||
| def combine(first: ConfigErrors, second: ConfigErrors): ConfigErrors = |
There was a problem hiding this comment.
Should we move this to ConfigErrors itself? Or have both? Might be nice to be able to do first combine second.
There was a problem hiding this comment.
I'm always in two minds about this. I guess moving it to the ConfigErrors would be consistent with ++ and friends in the standard collections lib.
Having both seems a bit redundant, so I'll move it into the class.
| We can then compose the results into an `Either[ConfigErrors, AppConfig]`: | ||
|
|
||
| ```tut:book | ||
| import _root_.cats.instances.parallel._ |
There was a problem hiding this comment.
Maybe we should use a :reset modifier somewhere (top of the section?) to avoid having to use _root_ here?
There was a problem hiding this comment.
Good idea, forgot you could do that.
| import ciris._ | ||
|
|
||
| trait CirisInstancesForCatsBinCompat { | ||
| implicit val semigroupConfigErrors: Semigroup[ConfigErrors] = new Semigroup[ConfigErrors] { |
There was a problem hiding this comment.
With ConfigErrors#combine, we could write this as:
implicit val semigroupConfigErrors: Semigroup[ConfigErrors] =
Semigroup.instance(_ combine _)
|
This looks great! Thanks again! Merging. 🎉 |
Having a cats Semigroup is quite handy, as it means you can do a bunch of
loadConfigs for small parts of your configuration and thenparMapNthem together like this.