Lifted transformers #146

krzemin · 2020-02-18T14:26:00Z

This PR is implementation of #118 approached in a bit different way to alternative #145 proposed by @ulanzetz.

Differences highlight:

unlike to TransformerF and cats.data.Validated support #145, this one keeps pure Transformer type class, along with TransformerF; the macro is generalized to be able to derive any transformer (pure or wrapped) as the derivation structure and rules seems to be the same; TransformerF and cats.data.Validated support #145 removes Transformer and uses TransformerF[Id, From, To] for pure transformers
in both PRs there is supporting type class that need to be defined any wrapper type F[_]; in this PR there is TransformerFSupport, in TransformerF and cats.data.Validated support #145 there is TransformationContext; there are minor differences in signatures (and naming), but they are mostly do the same
due to rewrite of mapping algorithm in this PR, we are able to mix pure values (withFieldConst/withFieldComputed) with wrapped ones (withFieldConstF/withFieldComputedF) in such a way that pure ones doesn't need to be boxed using TransformerFSupport.pure; as far as I understand, TransformerF and cats.data.Validated support #145 always wraps in TransformationContext.pure, even in case F = Id
TransformerF and cats.data.Validated support #145 provides enhanced support for capturing more detailed transformation errors with object path hold in a context; this PR doesn't support it, but it may be added later (I need to look closer at TransformerF and cats.data.Validated support #145 proposal)
TransformerF and cats.data.Validated support #145 provides a module for cats Validated integration, while in this PR there are instances only for Option[*] and Either[M[E], *]

After a brief look, both solutions should provide source compatibility, however in this PR several compile error messages may have changed due to refactor of mapping algorithm.

TODO list includes:

adding more tests, verifying border cases more comprehensively
completing implementation of wrapped transformers derivation for cases like options, eithers, sealed families, etc.
further refactor of transformer configuration (both type level in DSL and compile-time value level)
completing DSL operations in TransformerFDefinition and TransformerFInto

I believe we can pick best of both (this and #145) to the final release.

Any feedback is highly appreciated.

chimney/src/main/scala/io/scalaland/chimney/dsl/TransformerDefinition.scala

chimney/src/main/scala/io/scalaland/chimney/dsl/TransformerInto.scala

chimney/src/main/scala/io/scalaland/chimney/TransformerFSupport.scala

codecov · 2020-02-19T13:53:25Z

Codecov Report

Merging #146 into master will increase coverage by 4.29%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #146      +/-   ##
==========================================
+ Coverage   95.06%   99.35%   +4.29%     
==========================================
  Files          13       20       +7     
  Lines         527      623      +96     
  Branches       79       52      -27     
==========================================
+ Hits          501      619     +118     
+ Misses         26        4      -22

Impacted Files	Coverage Δ
...laland/chimney/internal/macros/PatcherMacros.scala	`96.42% <ø> (ø)`	⬆️
...o/scalaland/chimney/internal/DerivationError.scala	`100% <ø> (ø)`	⬆️
.../main/scala/io/scalaland/chimney/Transformer.scala	`100% <100%> (ø)`	⬆️
.../scalaland/chimney/internal/utils/MacroUtils.scala	`100% <100%> (ø)`	⬆️
...nd/chimney/internal/macros/TransformerMacros.scala	`99.15% <100%> (+3.68%)`	⬆️
...ala/io/scalaland/chimney/dsl/TransformerInto.scala	`100% <100%> (ø)`	⬆️
...la/io/scalaland/chimney/dsl/TransformerFInto.scala	`100% <100%> (ø)`
.../scalaland/chimney/dsl/TransformerDefinition.scala	`100% <100%> (+100%)`	⬆️
...a/io/scalaland/chimney/internal/macros/Model.scala	`100% <100%> (ø)`
...scalaland/chimney/internal/utils/EitherUtils.scala	`94.11% <100%> (+34.11%)`	⬆️
... and 16 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a8211b7...e4f1c76. Read the comment docs.

MateuszKubuszok

First round of review

chimney/src/main/scala/io/scalaland/chimney/TransformerF.scala

chimney/src/main/scala/io/scalaland/chimney/Transformer.scala

chimney/src/main/scala/io/scalaland/chimney/TransformerFSupport.scala

chimney/src/main/scala/io/scalaland/chimney/dsl/TransformerDefinition.scala

chimney/src/main/scala/io/scalaland/chimney/dsl/TransformerFDefinition.scala

chimney/src/main/scala/io/scalaland/chimney/internal/macros/DslWhiteboxMacros.scala

krzemin · 2020-03-10T00:01:49Z

Now I think almost all of the bullets from TODO list was addressed. There is comprehensive test suite which should cover all the cases in the context of lifted transformers.

I think it's a good moment to take another round of review. cc: @ulanzetz, @Phill101, @MateuszKubuszok

Next steps would be:

cats Validated module
decide what to do with enhanced error reporting
write docs

ulanzetz · 2020-03-11T12:50:16Z

chimneyCats/src/main/scala/io/scalaland/chimney/cats/package.scala

+package object cats {
+
+  implicit def TransformerFValidatedSupport[EE: Semigroup]: TransformerFSupport[Validated[EE, +*]] =
+    new TransformerFSupport[Validated[EE, +*]] {


Maybe add cats.data.Ior for semigroup A instance too?

Looks like a good idea 👍

But I'm struggling with reasonable semantics of

def product[A, B](fa: Ior[EE, A], fb: => Ior[EE, B]): Ior[EE, (A, B)]

for few cases:

when fa = Left(e) and fb = Right(b) - we cannot have Ior.Both, as we can't construct right pair ((A, B)), since we only have B; Ior.Left is the only result we can have here

when fa = Left(e1) and fb = Both(e2, b) - we can combine e1 and e2 using Semigroup instance, but also we cannot have Ior.Both as a result
It also includes symmetrical cases.

I'm not really familiar with using Ior, but I'm afraid that product can't be implemented with substantial information loss. The question is if such semantics makes sense at all?

chimney/src/main/scala/io/scalaland/chimney/TransformerFSupport.scala

chimney/src/main/scala/io/scalaland/chimney/internal/utils/EitherUtils.scala

MateuszKubuszok

I haven't found any issues with the code

chimney/src/main/scala/io/scalaland/chimney/Transformer.scala

MateuszKubuszok · 2020-03-11T18:32:55Z

chimney/src/main/scala/io/scalaland/chimney/TransformerFSupport.scala

+  *
+  * In order to create lifted transformation from `A` to `F[B]`,
+  * we need these few operations to be implemented for specific `F`
+  * wrapper type.


Link to docs showing why using Iterator is not a problem if anyone had problem with that

Isn't this explained enough in a scaladoc of traverse?

chimney/src/main/scala/io/scalaland/chimney/dsl/package.scala

initial design of lifted transformers - separate type class (LiftedTransformer) that works with F[_] - LiftedTransformerSupport type class that code is generated against which decides on composition semantics of resulting F[_] - parallel hierarchy of Lifted-{TransformerInto/TransformerDefinition} (not complete yet) - few test cases working with Option and Either (not passing yet) better error message on withFieldComputedLifted when types don't match renaming: LiftedTransformer -> TransformerF stronger type signatures for whitebox macro impl backed dsl operations code cosmetics LiftedDslSpec -> DslFSpec connect dsl with macro stuff code formatting first, always-succeeding test for TransformerF passes - added pure: A => F[A] to TransformerFSupport - use pure to wrap target transformer tree first bunch of tests on TransformerF passes code formatting simplify generated tuple extractor pattern: untyped bind is ok extract TypeOps.applyTypeArg for type argument application use kind-projector plugin generalized TransformerFEitherErrorAccumulatingSupport to support any collection of errors as long as we have implicit Factory for creating Builders code formatting renaming trying to pass recursive TransformerF test - wip reworking case class mapping resolution and recursive transformer body expansion (wip) reworking case class expansion (wip) comment next steps still wip new case class mapping algorithm works with all TransformerF tests! - there's one test case that doesn't work (IssuesSpec -> #121) fix error messages in recursive cases code formatting backport 2.13-only either combinators to EitherUtils towards 2.11/2.12 support work arounds for type inference so that DslFSpec pass on 2.11 too code formatting refactor: extract mkNewJavaBean refactor: extract mkNewJavaBean/mkNewClass to TargetConstructorMacros - extract Model trait rewrite expandDestinationTuple using new mapping routines code formatting use new mapping algorithm in tuples and java beans destinations use &&& in scala-collection-compat dependency get rid of unused implicit relax `traversable` type bound to IterableOnce bring back -Xfatal-warnings on 2.12+ generalize traverse signature and implementations to support different input/output collection types use -language:higherKinds compiler flag instead of scala.language.higherKinds imports more test cases for 3-arg wrapped transformation TransformerF support for value classes code formatting figure out transformation under Option figure out transformation under Option remove redundant case in expandOptions refactor more detailed test cases for collections add more collection test cases (array as target) refactor collection transformation use .fold on Either instead of pattern matching bugfix wrong subtype test refactor expandEithers expandEithers working with lifted transformers code formatting lifted subtype transformation eliminate expandMaps case keeping the same functionality - Maps are iterables, where inner type is tuple - we can re-use tuple expansion for expanding inner transformer - perhaps added well defined collection -> map and map -> collection conversions for free (but need to test it more extensively) - all current tests pass remove dead code add test cases: - between iterables and maps - between arrays and maps tests for wrapped maps added few TODO comments lifted transformers for expandTargetWrappedInOption - refactored to be expressed using expandOptions lifted transformers for expandSourceWrappedInOption refactor expandSealedClasses lifted transformer support for expandSealedClasses - new DSL operator: withCoproductInstanceF - changed rules of expanding sealed classes (so that instances provided by DSL has priority over automatic derivation) - need to require covariant wrapper type F (due to convenience of withCoproductInstanceF) - number of test cases for lifted transformers, verifying also support for GADTs code formatting changed order of type parameters in `intoF` - F comes first to be consistent with `transformIntoF` docs wip extract ConfigDsl to avoid code/docs duplication code formatting complete operations in both pure and lifted dsl - scaladocs - simplified implementation, removed DslWhiteboxMacros scaladocs for dsl scaladocs for TransformerFSupport refactor fresh term names not measure coverage on impossible cases materialize TransformerFSupport before macro expansion fix term name prefix ("left" -> "right") can't eliminate WrapperType refactor mkWrappedTransformerBody - added special implementations for argless and unary cases code formatting removed misleading return types from scaladocs refactor TransformerCfg type constructors rename: combine to product in TransformerFSupport cats module with TransformerFSupport instance for cats.data.Validated refactor in DslFSpec tests for cats Validated module increasing test coverage

krzemin · 2020-03-12T20:51:49Z

As this PR is already huge enough, I think that enhanced error path reporting (as in @ulanzetz's #145 proposal), although highly welcomed, might be added later as a separate PR. Doing it right is a major challenge itself and I don't intend to make review of this PR even harder for anyone.

- as the function doesn't always produce wrapped tree

ldrygala

Far from being chimney expert but it looks good to me 🚀 🎉

…onflicts (#151) * Add tests for enforcing explicit resolution of transformer implicit conflicts * hacky utils for refineTransformerDefinition that work on 2.11-2.13 * fixed test cases to compile * code formatting * keep corespondence between value-level TransformerDefinition(F) instances/overrides and its type-level configuration - so far the config was parsed in wrong way so that if someone provided withFieldXXX twice for the same field, it could end up with generating unsound code * enhance recursive wrapped transformer resolution algorithm - first look for bot pure and wrapped implicits - if one of them was found, use it - if both of them were found, report ambiguity error - if no implicit was found try deriving and instance - first try to derive pure instance - if pure instance cannot be derived, try deriving lifted instance * docs for improved lifted transformer derivation Co-authored-by: Piotr Krzemiński <pio.krzeminski@gmail.com>

MateuszKubuszok

LGTM - let's keep it open on case someone would found an issue, but IMHO it's a complete feature

Krever · 2020-03-13T21:00:46Z

I cant go through code right now but I tried to think about cases in which this feature interacts with my quite long experience with using chimney in production.
Weirdly, I never felt the need for lifted transformers or at least I can't recall such need.
Although I remember that chimney had an unsafe option to transform Option[T] => T via .get which now may stop being unsafe. I can't check right now if this was addressed somehow in the PR, so I'm just leaving the comment for your consideration.

Everything else looks good! I'm quite astonished by the amount of work put in this. Great job!

krzemin · 2020-03-13T23:04:07Z

@Krever this PR actually keeps Option[T] => T as unsafe even in the lifted context, but it provides withFieldConstF which allows you to express your domain validation rules, possibly capturing validation errors. I think that in the long term Option[T] => T can be marked at deprecated in favour of lifted transformers (which is already recommended in docs as a safer alternative).

ulanzetz · 2020-03-14T08:59:29Z

@Krever also you can define something like this

 implicit def optionUnwrapped[F[_]: ApplicativeError, A, B](implicit underlying: Transformer[A, B]): TransformerF[F, Option[A], B] =
    {
      case Some(value) => ApplicativeError[F].pure(underlying.transform(value))
      case None        => ApplicativeError[F].raiseError(...)
    }

and make your option unwrapping safe

krzemin requested a review from MateuszKubuszok February 18, 2020 14:26

krzemin linked an issue Feb 18, 2020 that may be closed by this pull request

Transformation returning Validated/Either/... #118

Closed

ulanzetz suggested changes Feb 19, 2020

View reviewed changes

Phill101 reviewed Feb 19, 2020

View reviewed changes

chimney/src/main/scala/io/scalaland/chimney/TransformerFSupport.scala Show resolved Hide resolved

Phill101 reviewed Feb 19, 2020

View reviewed changes

chimney/src/main/scala/io/scalaland/chimney/TransformerFSupport.scala Outdated Show resolved Hide resolved

MateuszKubuszok reviewed Mar 3, 2020

View reviewed changes

krzemin force-pushed the feature/lifted-transformers branch from 30b2ca5 to bc07c0d Compare March 9, 2020 17:35

krzemin requested a review from MateuszKubuszok March 9, 2020 23:52

krzemin requested a review from ldrygala March 10, 2020 13:21

ulanzetz reviewed Mar 11, 2020

View reviewed changes

ldrygala reviewed Mar 11, 2020

View reviewed changes

krzemin force-pushed the feature/lifted-transformers branch 2 times, most recently from 257f521 to 69ec21a Compare March 11, 2020 14:12

krzemin changed the title ~~[WiP] Lifted transformers (PoC)~~ Lifted transformers Mar 11, 2020

MateuszKubuszok approved these changes Mar 11, 2020

View reviewed changes

krzemin added 4 commits March 12, 2020 08:51

port behavior from #148

a050230

avoid building right collection as early as first error appeared

067e450

withFieldComputed(F) scaladocs improvement

29b10a7

krzemin force-pushed the feature/lifted-transformers branch from 365e686 to 29b10a7 Compare March 12, 2020 07:57

krzemin added 5 commits March 12, 2020 08:59

scaladocs improvements

7b504f7

user documentation for lifted transformers and cats integration module

164537f

define a trait separated from package object

0c0e9f9

a section about deriving lifted transformers

685b135

eliminate c. macro context prefix where possible

4f5223b

krzemin requested a review from ldrygala March 12, 2020 21:10

rename: mkWrappedTransformerBody -> mkTransformerBodyTree

4de7927

- as the function doesn't always produce wrapped tree

ldrygala approved these changes Mar 13, 2020

View reviewed changes

MateuszKubuszok approved these changes Mar 13, 2020

View reviewed changes

MateuszKubuszok mentioned this pull request Mar 13, 2020

Transformation returning Validated/Either/... #118

Closed

krzemin merged commit ec98b4a into master Mar 17, 2020

krzemin deleted the feature/lifted-transformers branch March 17, 2020 16:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Lifted transformers #146

Lifted transformers #146

krzemin commented Feb 18, 2020 •

edited

Loading

codecov bot commented Feb 19, 2020 •

edited

Loading

MateuszKubuszok left a comment

krzemin commented Mar 10, 2020 •

edited

Loading

ulanzetz Mar 11, 2020

krzemin Mar 11, 2020 •

edited

Loading

MateuszKubuszok left a comment

MateuszKubuszok Mar 11, 2020

krzemin Mar 12, 2020

krzemin commented Mar 12, 2020

ldrygala left a comment

MateuszKubuszok left a comment

Krever commented Mar 13, 2020

krzemin commented Mar 13, 2020

ulanzetz commented Mar 14, 2020 •

edited

Loading

Lifted transformers #146

Lifted transformers #146

Conversation

krzemin commented Feb 18, 2020 • edited Loading

codecov bot commented Feb 19, 2020 • edited Loading

Codecov Report

MateuszKubuszok left a comment

Choose a reason for hiding this comment

krzemin commented Mar 10, 2020 • edited Loading

ulanzetz Mar 11, 2020

Choose a reason for hiding this comment

krzemin Mar 11, 2020 • edited Loading

Choose a reason for hiding this comment

MateuszKubuszok left a comment

Choose a reason for hiding this comment

MateuszKubuszok Mar 11, 2020

Choose a reason for hiding this comment

krzemin Mar 12, 2020

Choose a reason for hiding this comment

krzemin commented Mar 12, 2020

ldrygala left a comment

Choose a reason for hiding this comment

MateuszKubuszok left a comment

Choose a reason for hiding this comment

Krever commented Mar 13, 2020

krzemin commented Mar 13, 2020

ulanzetz commented Mar 14, 2020 • edited Loading

krzemin commented Feb 18, 2020 •

edited

Loading

codecov bot commented Feb 19, 2020 •

edited

Loading

krzemin commented Mar 10, 2020 •

edited

Loading

krzemin Mar 11, 2020 •

edited

Loading

ulanzetz commented Mar 14, 2020 •

edited

Loading