ConfDecoder: optionally decode with initial value

[kitbellew]

No description provided.

[kitbellew]

@olafurpg @poslegm @gabro PTAL

[olafurpg]

Thank you for this contribution! A few minor comments, otherwise looks good to me

[olafurpg]

Can we revert the diff in this file?

[kitbellew]

done, figured it out

[olafurpg]

Can we move this to a separate file?

[kitbellew]

done

[olafurpg]

Exact name up for debate, but can we come up with something else than apply?

Suggested change

	def apply(default: A): ConfDecoder[A]
	def decoder(default: A): ConfDecoder[A]

[kitbellew]

done

[kitbellew]

@olafurpg @poslegm i will reply to code comments later but now I'm not so sure this approach actually works.

as I've come to realize, for nested structures, the trick that scalafmt uses is by listing decoders for each of its fields explicitly. would the macro implementing the factory be able to do the same?

even if, let's say, all the field factories were somehow implicitly available, how would we invoke them, too? not an expert on macros nor reflection.

@olafurpg, you asked if i could revert the diff to one of the test suites: i couldn't make that work, it was for some reason entering an infinite loop while instantiating the case class, and why that was affected by the change in the macro is not clear to me...

so, help? any suggestions?

[kitbellew]

will submit the new patches soon, i think i figured out how it works, just need to think of tricky tests.

[kitbellew]

done, figured it out

[kitbellew]

done

[kitbellew]

done

[kitbellew]

@olafurpg @poslegm i will reply to code comments later but now I'm not so sure this approach actually works.

Well, maybe it does, see new patch.

as I've come to realize, for nested structures, the trick that scalafmt uses is by listing decoders for each of its fields explicitly. would the macro implementing the factory be able to do the same?

The macro uses Conf.getSettingOrElse and actually passes both the type and the default, so it becomes easy to require an implicit field factory and use that.

So the only thing I needed to address is parsing derived types (Option, Map, Seq), for which the default provided is the instance of that type and not the generic. For that, ConfDefault, a wrapper around a type default value, is introduced.

@olafurpg, you asked if i could revert the diff to one of the test suites: i couldn't make that work, it was for some reason entering an infinite loop while instantiating the case class, and why that was affected by the change in the macro is not clear to me...

Don't know why the infinite loop was happening, but I reversed the invocations and now it works.

[olafurpg]

Would it be possible to achieve this same functionality by introducing a macro that generates A => ConfDecoder[A] functions? 🤔 It would be nice to avoid introducing a new ConfDecoderFactory typeclass.

[olafurpg]

For future PRs, please try to avoid unnecessary diffs like here since it makes reviewing the changes harder.

[kitbellew]

of course. i usually split such changes into a separate commit (in the same PR), so if you review commit by commit, it shouldn't clutter (we use gerrit, with its single-commit review model, so splitting, amending and rebasing is typical). i could also submit a separate PR and wait for it to get merged, if that's preferable (i don't think github allows splitting a sequence of dependent commits into two dependent PRs).

[olafurpg]

Breaking changes in the public Metaconfig API like in this diff here require a bump in the Scalafix major version. Custom Scalafix rules use this API to read configuration. For better or worse, I would like to avoid the situation where custom Scalafix rules that were compiled against Scalafix v0.9.14 stop working with Scalafix v0.9.15.

Is there a way to introduce ConfDecoderFactory without making it a superclass of ConfDecoder?

[olafurpg]

I am BTW open to merge breaking changes in the metaconfig.internal package to unblock improvements in Scalafmt.

[kitbellew]

@olafurpg would you be open to binary-breaking changes which (apparently) apply only to scala 2.11 and earlier?

the reason i ask is, the simplest change is adding another method (read(conf: Conf, state: A)) to ConfDecoder rather than creating a new type like @poslegm proposed (and I am coming around to) or adding a pair of new types (ConfDecoderFactory and DefaultFactory like I proposed here). then this method by default can be implemented to ignore the second parameter and call the regular read(conf: Conf).

I am not an expect on this, this is what I read:

• adding methods to traits with a default implementation is only a problem with scala 2.11: https://github.com/jatcwang/binary-compatibility-guide#dont-adding-methods-with-default-implementation-to-traits-211-or-before
• when major is 0 (both metaconfig and scalafix), binary-breaking changes can be added to minor version, but not to patch (so 0.10.1 and not 0.9.15): https://docs.scala-lang.org/overviews/core/binary-compatibility-for-library-authors.html#recommended-versioning-scheme

[olafurpg]

I'm OK with binary breaking changes in only 2.11 since Scalafix rules compile only against 2.12.

Adding a new abstract method to ConfDecoder[A] is binary breaking for clients that do new ConfDecoder[A] { ... }.

It's possible to enable MiMa to verify automatically what changes are binary compatible. So far this project hasn't had large API changes so I haven't worried about it

[olafurpg]

Commenting the review suggestion inline to elaborate on what I mean. Would it be possible to do something like this instead? The other decoder/encoder macros would then be implemented on top of this macro expansion. It would fix #58 if we make the default argument a lambda parameter since then it's always a trivial expression (Ident)

Suggested change

	def deriveCodecFactory[T]: ConfCodecFactory[T] =
	def deriveCodecFactory[T]: T => ConfCodec[T] =

[kitbellew]

this, or something very similar to this, is what didn't work in my first patch, leading to a circular dependency in one of the files which i worked around (without really understanding why it was failing) by moving some code into the companion object (and you requested to move back).

your suggestion will make one such usage simpler (but perhaps it could be emulated with an implicit conversion), but it would make chaining harder. what i am trying to allow, for instance, is the scalafmt case of presets (and also, if possible, avoiding creating an instance of a decoder for each instance of the class).

this is what scalafmt does today (all demonstrative, not exact):

class ScalafmtConfig(...) {
  implicit val decoderField1 = decoderForField1(field1)
  ...
  implicit val decoderFieldN = decoderForFieldN(fieldN)
  implicit val decoderThis = generic.deriveDecoder(this)
}

and then for presets I needed to modify it like this:

class ScalafmtConfig(...) {
  implicit val decoderField1 = decoderForField1(field1) // hopefully we don't forget presets here
  ...
  implicit val decoderFieldN = decoderForFieldN(fieldN)
  private val baseDecoderThis = generic.deriveDecoder(this)
  implicit val decoderThis = decoderForPresets(baseDecoderThis)
}

and this is what I would like to accomplish:

class Align(...)
object Align {
  implicit val decoder = ... // includes presets and possibly tokens.add handling
}
...
class ScalafmtConfig(...) {
 ...
}
object ScalafmtConfig {
  private val baseDecoder = generic.deriveDecoder(ScalafmtConfig.default)
  implicit val decoder = decoderForPresets(baseDecoderThis)
}

meaning, the decoder for a type will be defined with the type and every class using it will get it automatically. (and one instance, too.)

for that, special-case handling will need to be implemented as chaining of methods (or factories). presets handling looks like decoding read(conf: Conf): Configured[(A, Conf)] and then using that to apply regular generic decoder read(conf: Conf, A: preset). and that needs to happen for each field, too, so must be inside Conf.getSettingOrElse.

hopefully, this makes some sense for you to comment further.

[kitbellew]

i have another version that i can submit here, or as a separate PR, which essentially merges @poslegm's idea with this one and adds one more method. see question about binary compatibility for 2.11 below.

[kitbellew]

@olafurpg would you be open to binary-breaking changes which (apparently) apply only to scala 2.11 and earlier?

the reason i ask is, the simplest change is adding another method (read(conf: Conf, state: A)) to ConfDecoder rather than creating a new type like @poslegm proposed (and I am coming around to) or adding a pair of new types (ConfDecoderFactory and DefaultFactory like I proposed here). then this method by default can be implemented to ignore the second parameter and call the regular read(conf: Conf).

I am not an expect on this, this is what I read:

• adding methods to traits with a default implementation is only a problem with scala 2.11: https://github.com/jatcwang/binary-compatibility-guide#dont-adding-methods-with-default-implementation-to-traits-211-or-before
• when major is 0 (both metaconfig and scalafix), binary-breaking changes can be added to minor version, but not to patch (so 0.10.1 and not 0.9.15): https://docs.scala-lang.org/overviews/core/binary-compatibility-for-library-authors.html#recommended-versioning-scheme

[kitbellew]

of course. i usually split such changes into a separate commit (in the same PR), so if you review commit by commit, it shouldn't clutter (we use gerrit, with its single-commit review model, so splitting, amending and rebasing is typical). i could also submit a separate PR and wait for it to get merged, if that's preferable (i don't think github allows splitting a sequence of dependent commits into two dependent PRs).

[kitbellew]

this, or something very similar to this, is what didn't work in my first patch, leading to a circular dependency in one of the files which i worked around (without really understanding why it was failing) by moving some code into the companion object (and you requested to move back).

your suggestion will make one such usage simpler (but perhaps it could be emulated with an implicit conversion), but it would make chaining harder. what i am trying to allow, for instance, is the scalafmt case of presets (and also, if possible, avoiding creating an instance of a decoder for each instance of the class).

this is what scalafmt does today (all demonstrative, not exact):

class ScalafmtConfig(...) {
  implicit val decoderField1 = decoderForField1(field1)
  ...
  implicit val decoderFieldN = decoderForFieldN(fieldN)
  implicit val decoderThis = generic.deriveDecoder(this)
}

and then for presets I needed to modify it like this:

class ScalafmtConfig(...) {
  implicit val decoderField1 = decoderForField1(field1) // hopefully we don't forget presets here
  ...
  implicit val decoderFieldN = decoderForFieldN(fieldN)
  private val baseDecoderThis = generic.deriveDecoder(this)
  implicit val decoderThis = decoderForPresets(baseDecoderThis)
}

and this is what I would like to accomplish:

class Align(...)
object Align {
  implicit val decoder = ... // includes presets and possibly tokens.add handling
}
...
class ScalafmtConfig(...) {
 ...
}
object ScalafmtConfig {
  private val baseDecoder = generic.deriveDecoder(ScalafmtConfig.default)
  implicit val decoder = decoderForPresets(baseDecoderThis)
}

meaning, the decoder for a type will be defined with the type and every class using it will get it automatically. (and one instance, too.)

for that, special-case handling will need to be implemented as chaining of methods (or factories). presets handling looks like decoding read(conf: Conf): Configured[(A, Conf)] and then using that to apply regular generic decoder read(conf: Conf, A: preset). and that needs to happen for each field, too, so must be inside Conf.getSettingOrElse.

hopefully, this makes some sense for you to comment further.

[kitbellew]

@olafurpg @poslegm please take a look at the current patchset, it should be fully binary compatible. i also tested with scalafmt and its presets logic, seems to work.

[olafurpg]

Can you elaborate on the exact motivation for this PR? Is the goal to avoid the pattern where we repeat inner implicit val fieldN = ... inside ScalafmtConfig? Or is the goal to enable some functionality that was not possible before?

In Scalafix we already achive factory-like functionality by declaring a method like this

def factory(default: ScalafixConfig): ConfDecoder[ScalafixConfig] =
  metaconfig.generic.deriveDecoder(default)

While this approach requires cumbersome boilerplate, it works OK. If the motivation of this PR is only to avoid boilerplate then I'm inclined to not merge it as I would like to avoid breaking changes in the API.

A bit of background: I'm not familiar with any Scala library that provides automatic decoders based on a default value. This was the primary motivation for creating Metaconfig in the first place (to support style = .. in Scalafmt). The current implementation may be a bit clunky, but it at least works.

[olafurpg]

I am not keen on adding a new read() method to the ConfDecoder[T] typeclass since it complicates the meaning of other operations like map(). A typeclass should only have one method.

[kitbellew]

@olafurpg thank you, i didn't know about type classes, they resemble template specializations in c++ which are unavailable in Java generics.

as to motivation, yes, it reduces a little bit of boilerplate but not much as the field types are not repeated (it might violate the DRY principle you occasionally refer to in your code comments).

primarily, it seems that i inadvertently derailed @poslegm #88 where he was adding useful functionality of merging values from conf with some input state (default or coming from earlier parsing of the same conf, like with presets).

in general, decoded scalafmt configuration is not fully defined from the initial value and simple generic parsing of conf, but a multistage process where output of each stage is a partial state and partial conf to be processed by the next stage.

for instance, first stage is applying presets and removing matched keys from conf. second is extracting incremental updates (.add and possibly .remove), then generic parsing and merging with prior steps. without moving the state through these decoders, hard to achieve cleanly.

as to binary compatibility. you're right, somehow everyone talks about adding methods with concrete implementation to a trait as breaking 2.11 but no one ever mentioned adding a method without an implementation (which is probably obvious, in hindsight).

[poslegm]

I still want to achieve default values support via .add = in metaconfig. My first try on it with prioritized typeclasses has a smell, and I think that ideas about ConfDecoderFactory and ConfDecoder.WithState are very useful. I will dig into it when have time.

But as I understand this implementation still has the problem of past attempts: if user call .map on stateful decoder he got ConfDecoder.WithoutState. In ideal world all combinators should save previous data.

[poslegm]

I think in the direction of such API: what if user will call map/flatMap and other combinators on ConfDecoderFactory which will propagate state over transformations chain? And at the it he calls .decoder to generate final ConfDecoder with all context.

[kitbellew]

the problem was that state: => B cannot be passed to underlying ConfDecoder[A], and not to break compatibility, I added mapExt with covariance and in/out (maybe an implicit CanBuildFrom would also work).

read about type classes, very interesting (however, they don't require a single method and even allow inheritance):

• https://scalac.io/typeclasses-in-scala/ (also https://github.com/mpilquist/simulacrum)
• https://typelevel.org/cats/typeclasses.html
• https://alvinalexander.com/scala/fp-book/type-classes-101-introduction/

[olafurpg]

My primary motivation to keep a single abstract method is that it makes it easier to reason about the meaning of methods like map(). It's the same reason I want to avoid multi-layer typeclasses because implicit resolution gets hairy when you have inheritance and you want to influence the priority of how typeclass instances get selected.

[olafurpg]

I am wondering if we can provide an out of the box solution for both the preset pattern + .add and extend the deriveDecoder macro to implement support for this without introducing changes to the public API 🤔

field types are not repeated (it might violate the DRY principle you occasionally refer to in your code comments).

When creating Metaconfig I was less concerned about repeated types more concerned about repeating field names in strings. A bit of boilerplate is OK IMO as long as you get compile-errors when you make a mistake. I still agree it would be nice if we can avoid the boilerplate of repeating inner fields to support presets.

[olafurpg]

To elaborate, we could provide a class like this in Metaconfig

case class Presets[T](values: Map[String, T])

That users can use like this

case class ScalafmtConfig(
  preset: Presets = Presets(Map("default" -> ScalafmtConfig.default, "align" -> ScalafmtConfig.align))
  align: Boolean = false)

object ScalafmtConfig {
  val default = ScalafmtConfig()
  val align = default.copy(align = true)
  val codec = metaconfig.generic.deriveDecoder(default)
}

The macro derived decoder would automatically replace the default value if the user defines a preset. I estimate this would require a fairly small change in the macro implementation around here

metaconfig/metaconfig-core/shared/src/main/scala/metaconfig/internal/Macros.scala

Line 118 in aaab8fe

val tmp = $default

[olafurpg]

I am open to do something similar to solve the .add/.remove pattern, but I don't have a concrete proposal at the top of my head

[olafurpg]

BTW, I am open to introduce breaking changes. I am planning to do a Scalafix v1.0 release at some point in which I think it's OK to upgrade to a new stable version of Metaconfig.

[kitbellew]

BTW, I am open to introduce breaking changes. I am planning to do a Scalafix v1.0 release at some point in which I think it's OK to upgrade to a new stable version of Metaconfig.

@olafurpg that makes it easier. i will, however, yield to @poslegm and his #92 as it was his idea to start with. i got carried away :)

[kitbellew]

@olafurpg here's a version which doesn't break binary compatibility and has only one method in the interface (aka, type class). ptal.

[olafurpg]

LGTM! Thank you for preserving binary compatibility 🙏

it’s not ideal to have duplicated the ConfDecoder class but it’s the lesser evil IMO. If you prefer, you can put ConfDecoderEx class in an internal package so you can break the api in the future.

[kitbellew]

LGTM! Thank you for preserving binary compatibility 🙏

it’s not ideal to have duplicated the ConfDecoder class but it’s the lesser evil IMO. If you prefer, you can put ConfDecoderEx class in an internal package so you can break the api in the future.

will do, thanks