-
Notifications
You must be signed in to change notification settings - Fork 174
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
Split ConfigConvert
into readers and writers
#179
Split ConfigConvert
into readers and writers
#179
Conversation
This PR is a WIP as it is still missing tests and documentation. I'd like to get some feedback from you before starting writing documentation. I'll be writing tests meanwhile. |
1 similar comment
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'm not sure why this is still WIP, I've read the PR and I think it's nice. I have no comments.
This PR is a WIP as it is still missing tests and documentation. However, as the tests for |
@ruippeixotog I'm concerned about possible conflicts with #178 , do you think we should wait for that to be merged before we change the documentation? |
Good question... I guess it's better if this is merged without the documentation then and I'll do another PR later, after #178. That way no one will have much trouble dealing with merge conflicts. |
ConfigConvert
into readers and writersConfigConvert
into readers and writers
Agreed, I'll wait for another approve and then merge |
@@ -94,36 +102,68 @@ trait ConvertHelpers { | |||
override def to(t: T): ConfigValue = ConfigValueFactory.fromAnyRef(t) | |||
} | |||
|
|||
def fromStringReader[T](fromF: String => Option[ConfigValueLocation] => Either[ConfigReaderFailure, T]): ConfigConvert[T] = new ConfigConvert[T] { | |||
def fromStringReader[T](fromF: String => Option[ConfigValueLocation] => Either[ConfigReaderFailure, T]): ConfigReader[T] = new ConfigReader[T] { |
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.
Do you think we should move reader/writer helpers in the Reader
/Writer
companion object? I think it could be nice to have ConfigReader.fromString
instead of ConverterHelper.fromStringReader
. You don't need the Reader
at the end of fromStringReader
if the object in which the method is defined is a Reader
(I hope I explained myself). What do you think?
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.
Yes, that's a good idea! I'll move all the methods, but I'll leave forwarder deprecated methods in ConfigConvert
in order not to break compatibility here, ok?
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.
Yes
@melrief besides the renaming you suggested, I ended up doing a greater number of changes in order to keep the code clean and with consistent names in 3a9823b.
I tried to match the factory method name to the type of entity to build. I left a large number of deprecated constructors in Let me know what you think of it now! |
@ruippeixotog thanks for the changes, the new API is much nicer in my opinion! I don't have time to do the review but tomorrow I'll have a look in deep. |
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.
Nice, I like the new API for ConfigReader
and ConfigWriter
even more. I'm waiting for another review.
} | ||
trait ConfigConvert[T] extends ConfigReader[T] with ConfigWriter[T] | ||
|
||
object ConfigConvert extends ConvertHelpers { |
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.
Suggestion: Add a class-level comment like:
/**
* This object provides methods to create [ConfigConvert] instances.
*/
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 in 5845cac.
def from(config: ConfigValue): Either[ConfigReaderFailures, T] | ||
} | ||
|
||
object ConfigReader extends BasicReaders with DerivedReaders { |
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.
Suggestion: Add a class-level comment like:
/**
* This object provides methods to create [ConfigReader] instances.
*/
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 in 5845cac.
def to(t: T): ConfigValue | ||
} | ||
|
||
object ConfigWriter extends BasicWriters with DerivedWriters { |
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.
Suggestion: Add a class-level comment like:
/**
* This object provides methods to create [ConfigWriter] instances.
*/
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 in 5845cac.
override def to(t: FieldType[K, V] :: T): ConfigValue = { | ||
val keyStr = hint.configKey(key.value.toString().tail) | ||
val rem = tConfigWriter.value.to(t.tail) | ||
// TODO check that all keys are unique |
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 immortal TODO
lives on.
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 think we should address old TODOs in another PR because this one is dealing with enough stuff :-)
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 agree with @leifwickland. The comment is still relevant and unrelated to this PR.
|
||
override def to(t: FieldType[Name, V] :+: T): ConfigValue = t match { | ||
case Inl(l) => | ||
// Writing a coproduct to a config can fail. Is it worth it to make `to` return 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.
Is this the next great zombie comment?
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 believe the comment is still relevant - it mentions a question for which there is still no clear answer. Though it may not be the best place to make TODOs and discussion topics visible, I don't mind having comments like this around for as long as it takes.
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.
Kudos, @ruippeixotog! This was a huge change that seems to be have executed extremely well.
I offered a couple suggestions, but this LGTM. 👍
I have just fixed the merge conflicts. Unless you want to wait for @jcazevedo to review this, this is ready to merge, @melrief. @leifwickland I believe I replied to all your comments. Please let me know if you see anything else :) |
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 looks good to merge as is! Sorry for not reviewing it earlier.
In this PR
ConfigConvert
is redefined as an union of two new traits:ConfigReader
for reading objects from configs andConfigWriter
to write objects to configs.ConfigConvert
continues to exist and I think it still plays an important role in pureconfig. Most of the times users will be able to define both operations for a type, and for organization purposes it's good to define them together. However, with PR users will be able to define only one of the operations and still use operations such asloadConfig
andtoConfig
, which is particularly useful for some particular models that only have a one-way conversion.I opted to split the
BasicConverters
andDerivedConverters
in readers and writers in order to mix into the companion objects ofConfigReader
andConfigWriter
only the implicits that mattered to each one. However, it definitely doesn't have to be the case and I think we should leave theConfigConvert
definitions in pureconfig modules as they are. AConfigConvert
can be seen both as aConfigReader as a
ConfigWriter`, and in the modules we don't have any advantage in splitting them.This change, when seen together with #171, is source-compatible with the latest version 0.6.0. If we do a release without this PR it may be possible for users to start using
BasicConverters
andDerivedConverters
directly and this PR causes them to disappear.Closes #92.