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
ValueReader instances for Either[L,R] #45
Conversation
package net.ceedubs.ficus.readers | ||
import com.typesafe.config.{Config, ConfigException} | ||
|
||
/** |
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.
Can you remove these ide-generated comments? I think git blame suffices.
@joprice, @kailuowang , |
@eyalfa sorry for the delay , we had a long weekend vacation here in the U.S.A. |
@@ -45,9 +46,10 @@ object ConfigSerializer { | |||
|
|||
final case class ConfigSerializerOps[A](a: A, serializer: ConfigSerializer[A]) { | |||
def asConfigValue: String = serializer.serialize(a) | |||
def toConfigValue = ConfigFactory.parseString( s"dummy=$asConfigValue").root().get("dummy") |
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.
can we add the return type here, just so that people don't have to read through the code to tell the difference between asConfigValue
and toConfigValue
(also how about change the name to toConfig
to further differentiate the two?)
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 method returns a ConfigValue, not a Config. (ConfigValue might be a ConfigBoolean.Int.Double/String...)
so I'm not sure about renaming this method.
} | ||
|
||
object ConfigSerializerOps { | ||
object ConfigSerializerOps { |
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.
accidentally introduced space?
Looking good to me except the two minor comments above. |
Yes to both suggestion,will do once I have computer access.
…On Nov 28, 2016 5:59 PM, "Kai(luo) Wang" ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In src/test/scala/net/ceedubs/ficus/ConfigSerializer.scala
<#45 (review)>:
> }
-object ConfigSerializerOps {
+ object ConfigSerializerOps {
accidentally introduced space?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#45 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABFFOaSvoIu0itd8-aL2HB1SWIZfZXBXks5rCvpTgaJpZM4K7_9O>
.
|
2 similar comments
👍 thanks a lot! |
No description provided.