Closed an-tex closed 1 year ago
TBH, IMHO the previous behavior is a bug, and we haven't caught it before since there was no test which used Patchers that way. As far as our tests were concerned (in 0.7.5) using val
s in constructors were unspecified behavior. Currently, Chimeny doesn't distinguish between val
s defined as a constructor's parameters and val
s defined in class body: all of them are val
instantiated in a constructor, a stable values that are always safe to use as the source values. (And now we are testing for this behavior with our tests).
In future, we plan to implement case class merging, and rewrite Patcher
to be a specialized (obj, patch) => patch.into[A].withFallbackValue(obj).transform
internally, so I am inclining against adding more differences to the behavior between Patchers and Transformers than we already have.
i'd argue vals defined in a class body should be ignored in general as they rather contain domain specifics which should not be accounted for in transformations (that's the unintentional 0.7.5 behavior)
furthermore to me it seems there's already a difference in transformer and patcher behavior, e.g.:
https://scastie.scala-lang.org/Dbf0kkhVQPOuhBbochlsww
we have pretty much the same use case in our code base. two case classes have exactly the same constructor parameter val definitions, but both also define a class body val with the same name. in this case we can't transform between those two (as a workaround we could add .ignoreRedundantPatcherFields but then we loose that important check for the constructor parameters)
I agree, there are mismatches between Patchers and Transformers. But the only intended difference is that:
Option
fields when it comes to merging 2 Options in PatchersEverything else is an accidental consequence of the fact that the current implementations of Patchers was merely put together in a quick-and-dirty way ~3~ 5 years ago, wasn't redesigned since, and is basically scheduled for deletion. I'd also argue that Chimney in general is not intended to be used as a "way of avoiding writing runtime tests, because types" nor "we'll cover all possible use cases". If your Patchers are domain objects... well, then I think you are making assumptions that the library isn't making. I would e.g. solve your problem with:
import io.scalaland.chimney.dsl._
case class A(i: Int) {
val b: Boolean = false
}
case class B(i: Int) {
val b: Boolean = false
}
A(0).transformInto[B] // compiles
A(0).using(new { val i = 1 }).patch // <------ also compiles (*)
A(0).using(B(1)).ignoreRedundantPatcherFields.patch // compiles
// (*) I guess on Scala 3 I could get away with:
// val patch = B(1)
// new { export patch.i } or
// new { export patch.{b => _, *} } or
// new { export patch.{b as i} } + given Transformer[Boolean, Int]
// etc, depending on what I need
(see Scastie)
and write unit tests checking if the runtime value is as I would expect it to be. And if that doesn't look super useful... Well, TBH I myself see current Patcher
s as something that is hardly ever useful. Since they are flat, hardly ever you have a big gain over writing that .copy
manually, there is also no big win over Quicklens + a.modify(_.field)).setTo(patch.field.transformInto)
. The patch class would have be really big to see a noticeable difference. Until Patcher
s are rewritten to be derived as transformers (which recursively merge and transform 2 input values) they won't be able to:
I'd say that currently Patchers are designed mostly for cases when you define Patch
object only for patching purposes, and only a very few situations when you can get away with reusing something existing.
i'd argue vals defined in a class body should be ignored in general as they rather contain domain specifics which should not be accounted for in transformations (that's the unintentional 0.7.5 behavior)
That was intentional behavior that we tested for since ~I think~ 0.3 :) Ever since we rewrote the library into macros we are intentionally looking at every val
in the source value. Only def
s and inherited properties were enabled by flags. I guess people assumed that Chimney only works with case class
-> case class
(and that it only looked at constructor val
s) since it didn't boast about it (now we describe that explicitly in the documentation - section "into case class or pojo", example 3), but you could do something like:
class Foo {
val a: Int = 42
}
case class Bar(a: Int)
(new Foo).transformInto[Bar]
for quite a while. The main difference in 0.8.0 is that we relaxed requirements on target from "case class
or Java Bean (or rather POJO)" to "any non-abstract class with a public constructor". Reading from any source val
is the intentional 0.3 behavior :) Patchers didn't follow that behavior only by accident (they reused some logic from Transformers and by mistake reimplemented some other, so they ended up pretty inconsistent, at least until 0.8.0 until-it-passes-scala2+3-tests-rewrite with the intention of one-more-final-rewrite into Transformers with value merging, once they are a thing).
We find patchers still very useful (applying an Event as a patch on top of a State in EventSourcing) but I understand our use case is fairly specific. I guess we'll just add a .ignoreRedundantPatcherFields
in the mean time (with appropriate test coverage in place) and wait for the value merging approach (the suggested alternative approaches are unfortunately not feasible in our case).
Nevertheless, thanks @MateuszKubuszok for your fast and detailed reply! Really appreciate the time you take and thanks for creating and improving this great library :)
No problem, hopefully, with Scala 3 and improvements we might pull off in the future Patcher
s will be more useful and easier to apply in cases like yours.
Checklist
TransformerF
s) orunsafeOption
flagsDescribe the bug Chimney 0.7.x ignored additional case class field variables on the source when using a patcher. It only complained about additional parameters. In chimney 0.8.x this has changed.
I've checked the release notes of 0.8.x and the migration guide but I couldn't find anything describing this breaking change. is this intentional?
Reproduction 0.7.5 compiles: https://scastie.scala-lang.org/DrVVl6FWSGKhev9hlbOViA
0.8.0 does not compile: https://scastie.scala-lang.org/DQ0iG1YsRIWw2GgfLKlRKA