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

Add methods to derive readers, writers and full converters for enumerations #466

Merged
merged 20 commits into from May 9, 2019

Conversation

Projects
None yet
3 participants
@jcazevedo
Copy link
Member

commented Apr 21, 2019

This PR adds methods to semi-automatically derive readers, writers and full converters for enumerations encoded as sealed families of case objects.

This is particularly helpful in a semi-automatic approach, because if we opted for an EnumCoproductHint to derive a reader/writer for a sealed trait, we still had to provide readers/writers for the subclasses, even though they were left unused.

This PR also deprecates EnumCoproductHint. Even in a fully automatic approach, I think it's preferable to rely on these new methods than deriving unnecessary readers/writers for subclasses.

jcazevedo added some commits Apr 20, 2019

Add methods to derive enumerations
Deprecate EnumCoproductHint in favor of methods in the `semiauto` package that
allow the derivation of readers, writers and full converters for enumerations
encoded as sealed traits. In a semi-automatic setting, this allows the
derivation of enumeration-based type classes for the sealed trait type without
requiring implicit type classes in scope for all the subtypes.

@jcazevedo jcazevedo requested review from ruippeixotog and leifwickland Apr 21, 2019

@ruippeixotog
Copy link
Member

left a comment

Great work, @jcazevedo! I love how we now check at compile-time whether we are trying to derive an enum reader for non-compatible sealed families.

One question: the issue you mention about using coproduct hints with semiauto derivation is not specific to enumerations, right? For other sealed families you also need to derive readers for all subclasses even though you may not them, right?

Show resolved Hide resolved ...c/src/main/scala/pureconfig/generic/EnumerationConfigReaderBuilder.scala
implicit def deriveEnumerationReaderBuilderCCons[K <: Symbol, H, T <: Coproduct](
implicit
vName: Witness.Aux[K],
hGen: LabelledGeneric.Aux[H, HNil],

This comment has been minimized.

Copy link
@ruippeixotog

ruippeixotog Apr 21, 2019

Member

Nice advantage, getting a compile-time guarantee that this is a well-formed enumeration.

Show resolved Hide resolved ...c/src/main/scala/pureconfig/generic/EnumerationConfigWriterBuilder.scala
Show resolved Hide resolved ...c/src/main/scala/pureconfig/generic/EnumerationConfigWriterBuilder.scala Outdated
final def deriveEnumerationReader[A](
implicit
readerBuilder: Lazy[EnumerationConfigReaderBuilder[A]]): ConfigReader[A] =
deriveEnumerationReader(_.toLowerCase)

This comment has been minimized.

Copy link
@ruippeixotog

ruippeixotog Apr 21, 2019

Member

Question for later: what do you think of doing a breaking change to make (PascalCase, KebabCase) the default transformation to use for coproduct hints and enumerations?

I believe that the only reason the default is .toLowerCase was because that's what spray-json-shapeless used, but it doesn't seem to be a popular one. Technically it's also ambiguous, as classes like MySQL, MySql and Mysql all map to the same config value. In product hints we are already assuming Scala code to be pascal case and Typesafe configs to be kebab case, so maybe we can make this consistent across the library?

This comment has been minimized.

Copy link
@leifwickland

leifwickland Apr 21, 2019

Collaborator

That lowercase default is an annoyance to me. I'd forgotten about it last week and spent a few minutes wondering why nothing worked as I expected.

I don't love the idea of a breaking change but moving in the direction of a more consistent API with less surprising defaults seems like a good reason for it.

This comment has been minimized.

Copy link
@leifwickland

leifwickland Apr 22, 2019

Collaborator

After thinking about it more, if we're going to make a change change to the naming conventions for enums, I think doing it when this API is introduced would be preferable to waiting until afterward and then breaking this API. My vote is that if we're going to switch, we should do in this PR.

This comment has been minimized.

Copy link
@jcazevedo

jcazevedo Apr 22, 2019

Author Member

I also prefer having pascal case to kebab case as the default transformation. I agree with @leifwickland that this a good opportunity to do it. I've done it in 2fdbf7a.

I'm not sure of how to put this in the changelog, though. Should we put it as a breaking change, or not mention it at all, since it's the behavior of a new method?

This comment has been minimized.

Copy link
@leifwickland

leifwickland Apr 22, 2019

Collaborator

I suggest that the changelog mention that the old way is deprecated and mention in the new features section that the new approach has a different naming behavior than the old.

This comment has been minimized.

Copy link
@ruippeixotog

ruippeixotog Apr 22, 2019

Member

@jcazevedo if we are to change the lowercase default it should not be only for enumeration readers, but for other coproduct hints too - otherwise we'll be encoding the same data (class names) differently in different hints, which is worse than being inconsistent between product and coproduct hints. Doing that requires "real" breaking changes, though :)

This comment has been minimized.

Copy link
@jcazevedo

jcazevedo Apr 22, 2019

Author Member

I'm OK with introducing that breaking change for the next version. I suppose we can improve the NoValidCoproductChoiceFound failure to show more information when deriving a reader with FieldCoproductHint. Showing the value found in the discriminating field together with a mention of the changed behavior should be helpful for users to migrate.

This comment has been minimized.

Copy link
@ruippeixotog

ruippeixotog Apr 24, 2019

Member

Cool! Can you do that change on this PR, to avoid having two different conventions when we merge this? :)

This comment has been minimized.

Copy link
@jcazevedo

jcazevedo Apr 25, 2019

Author Member

Sure. I'll update this tomorrow.

This comment has been minimized.

Copy link
@jcazevedo

jcazevedo May 2, 2019

Author Member

I changed the default transformation of FieldCoproductHint to PascalCase -> kebab-case in 1735d05.

I introduced a new UnexpectedValueForFieldCoproductHint failure reason, in order to improve error messages when using FieldCoproductHint. Incidentally, I moved the responsibility of raising ConfigReaderFailures when the disambiguation fails to CoproductHint. I think this also adds some extra flexibility to implementations of CoproductHint, but let me know what you think.

Sorry for the delay!

Show resolved Hide resolved ...c/src/main/scala/pureconfig/generic/EnumerationConfigReaderBuilder.scala
Show resolved Hide resolved ...c/src/main/scala/pureconfig/generic/EnumerationConfigWriterBuilder.scala Outdated
Show resolved Hide resolved ...c/src/main/scala/pureconfig/generic/EnumerationConfigReaderBuilder.scala Outdated
Show resolved Hide resolved tests/src/test/scala/pureconfig/DerivationModesSuite.scala Outdated
Show resolved Hide resolved docs/src/main/tut/docs/overriding-behavior-for-sealed-families.md Outdated
@leifwickland
Copy link
Collaborator

left a comment

I don't have a strong feeling about Rui's suggestions.

Thanks for this improvement.

case object RainyBlue extends Color
case object SunnyYellow extends Color
implicit val colorReader: ConfigReader[Color] = deriveEnumerationReader[Color](ConfigFieldMapping(PascalCase, KebabCase))

This comment has been minimized.

Copy link
@leifwickland

leifwickland Apr 21, 2019

Collaborator

That's nice. Kudos.

final def deriveEnumerationReader[A](
implicit
readerBuilder: Lazy[EnumerationConfigReaderBuilder[A]]): ConfigReader[A] =
deriveEnumerationReader(_.toLowerCase)

This comment has been minimized.

Copy link
@leifwickland

leifwickland Apr 21, 2019

Collaborator

That lowercase default is an annoyance to me. I'd forgotten about it last week and spent a few minutes wondering why nothing worked as I expected.

I don't love the idea of a breaking change but moving in the direction of a more consistent API with less surprising defaults seems like a good reason for it.

writerBuilder: Lazy[EnumerationConfigWriterBuilder[A]]): ConfigConvert[A] =
deriveEnumerationConvert(_.toLowerCase)

final def deriveEnumerationReader[A](transformName: String => String)(

This comment has been minimized.

Copy link
@leifwickland

leifwickland Apr 21, 2019

Collaborator

It's be a cheat but we could make the to lower the default argument to this version and get rid of the no arg version.

jcazevedo added some commits Apr 22, 2019

@jcazevedo

This comment has been minimized.

Copy link
Member Author

commented Apr 22, 2019

One question: the issue you mention about using coproduct hints with semiauto derivation is not specific to enumerations, right? For other sealed families you also need to derive readers for all subclasses even though you may not them, right?

Yes. It is just more obvious that you don't need them with EnumCoproductHint.

jcazevedo added some commits May 1, 2019

*/
final case class UnexpectedValueForFieldCoproductHint(value: ConfigValue) extends FailureReason {
def description =
s"Unexpected value ${value.render(ConfigRenderOptions.concise())} found. You might have a misconfigured " +

This comment has been minimized.

Copy link
@leifwickland

leifwickland May 2, 2019

Collaborator

I love this error message. Great job.

Do you think it'd be worth adding the type of the ADT/sealed family?

This comment has been minimized.

Copy link
@jcazevedo

jcazevedo May 6, 2019

Author Member

Sorry! I missed this message.

I think we can add the type of the sealed family here. We need to be careful, though, because this error will be scoped under the type field (or whatever the key of FieldCoproductHint is), so we need to be explicit that we're not trying to read that type from that field.

If we opt to add the type of the sealed family to this FailureReason, I think we should also do it for NoValidCoproductChoiceFound. I think doing so will complicate a bit the current implementation for deriving enumerations because we currently don't know the original type when we're deriving an EnumerationConfigReaderBuilder for the generic representation. Taking that into account, I'd rather leave that for another PR to avoid this getting too big.

This comment has been minimized.

Copy link
@leifwickland

leifwickland May 6, 2019

Collaborator

No worries. Feel free to decide it's not worth the bother to add ultimately.

* be changed by overriding the method `fieldValue` of this class.
*/
class FieldCoproductHint[T](key: String) extends CoproductHint[T] {

private val fieldTransformation: String => String = ConfigFieldMapping(PascalCase, KebabCase)

This comment has been minimized.

Copy link
@ruippeixotog

ruippeixotog May 3, 2019

Member

Can we move this to the companion object of FieldCoproductHint, maybe as defaultMapping for consistency with the value's class name? FieldCoproductHint instances are created on demand for every sealed trait involved in auto-derivation, there is no need to make the class heavier here.

This comment has been minimized.

Copy link
@jcazevedo

jcazevedo May 3, 2019

Author Member

Makes sense. I moved the default transformation to the companion object of FieldCoproductHint in 94d2401.

* @param cur a `ConfigCursor` at the sealed family option
* @return a failed `ConfigReader` result scoped into the context of a `ConfigCursor`.
*/
def failToDisambiguate(cur: ConfigCursor): ConfigReader.Result[CNil] =

This comment has been minimized.

Copy link
@ruippeixotog

ruippeixotog May 3, 2019

Member

I like the idea of allowing users to customize this :) Two remarks here:

  • I'd prefer if we didn't expose CNil here, as we've been keeping hints shapeless-free and this result type seems unnecessarily complex. Since we always require a failed result, why don't we simply ask for a FailureReason or a ConfigReaderFailure?
  • The name failToDisambiguate is misleading here (and "disambiguate" is probably overused in the Scaladoc of this trait). It implies that we somewhat identify when a config value is ambiguous (i.e. when we're not sure whether it refers to class X or Y), while that isn't the case (e.g. if some hint implementation always returns Right(Some(x)) every value is ambiguous but we won't ever call this method). This method is only triggered when we find no suitable option; what do you think of naming it noOptionFound instead?

This comment has been minimized.

Copy link
@jcazevedo

jcazevedo May 3, 2019

Author Member

I agree with both points. I prefer asking for ConfigReaderFailures because it allows implementations to return more than one failure. I did those changes in 8da8f67.

final case class UnexpectedValueForFieldCoproductHint(value: ConfigValue) extends FailureReason {
def description =
s"Unexpected value ${value.render(ConfigRenderOptions.concise())} found. You might have a misconfigured " +
"FieldCoproductHint. Note that the default transformation of FieldCoproductHint changed from converting to " +

This comment has been minimized.

Copy link
@ruippeixotog

ruippeixotog May 3, 2019

Member

"You might have a misconfigured FieldCoproductHint" may be a bit confusing for people that never used coproduct hints before. Do you think we could make the message more beginner-friendly by avoiding mentioning FieldCoproductHint, simply explaining the naming convention that changed and pointing out to https://pureconfig.github.io/docs/overriding-behavior-for-sealed-families.html or to the CHANGELOG for more information?

This comment has been minimized.

Copy link
@jcazevedo

jcazevedo May 3, 2019

Author Member

I suppose mentioning FieldCoproductHints makes the message a bit confusing, yes. I've changed it in 2bdaad1.

putting in scope an `implicit def coproductHint[T] = new FieldCoproductHint[T]("type") { override def
fieldValue(name: String): String = name.toLowerCase }`;

- New features

This comment has been minimized.

Copy link
@ruippeixotog

ruippeixotog May 3, 2019

Member

We should probably add here the ability to configure the error message in coproduct hints, even though it's probably only important for us and for the few people that needed to implement their own hints.

This comment has been minimized.

Copy link
@jcazevedo

jcazevedo May 3, 2019

Author Member

I think we should add it, yes. Done in be94389.

jcazevedo added some commits May 3, 2019

@ruippeixotog
Copy link
Member

left a comment

LGTM, thanks for addressing all the comments!

@ruippeixotog ruippeixotog merged commit 4abff9c into master May 9, 2019

3 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.4%) to 94.461%
Details

@ruippeixotog ruippeixotog deleted the semiautomatic-enumerations branch May 9, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.