Repository navigation
FallibleTransformer derivation for coproducts - #69
Conversation
|
This looks fantastic. scalafix seems to be failing, could you please run |
There was a problem hiding this comment.
Just those two nitpicks, otherwise it looks gorgeous, thank you for spending your time on this 😄
| } | ||
| } | ||
|
|
||
| private def ifStatement(using Quotes)(branches: List[IfBranch]): quotes.reflect.Term = { |
There was a problem hiding this comment.
So we use duplicates of this, IfBranch and IsInstanceOf inside CoproductTransformations as well, would you mind extracting these into a separate and reusable object? eg. IfStatement that'd house IfBranch and other things related to this inside
There was a problem hiding this comment.
I've extracted the IfBranch logic into its own reusable object ExhaustiveCoproductMatching. Maybe I've went a bit to far there. What do you say?
| (source.tpe -> dest.tpe) match { | ||
| case '[src] -> '[dest] => | ||
| val value = | ||
| source.fallibleTransformerTo[F](dest).map { |
There was a problem hiding this comment.
there's an optimization we can do here where we match on the summoned transformer and if it is a FallibleTransformer.fallibleFromTotal we can strip apart the total transformer with LiftTransformation.liftTransformation which will make it so that the body of the transformer is inlined instead of creating a transformer instance at runtime. Example of how to do it is here:
There was a problem hiding this comment.
I think I've achieved what you had suggested. I had problems getting a test case to match FallibleTransformer.fallibleFromTotal. But I've added an optimization to use total transformers, if available. The cost for that optimization is a second implicit search. I'm not sure if that's worth it.
* Extract duplicated If-Branches construct into its own object * Optimize transformation logic if an total transformer is available
There was a problem hiding this comment.
Just this one thing to delete and I think we're golden 😸
| '{ $F.pure[Dest]($singleton.asInstanceOf[Dest]) } | ||
| } | ||
|
|
||
| tryTotalTransformation |
There was a problem hiding this comment.
Hmm, I think we don't need this as this is supposed to be covered in 'FallibleTransformer.fallibleFromTotal` as for why you couldn't get it to actually work... Maybe I just screwed up instance prioritization for these, I faintly remember having some issues when priotitizing 'fallibleFromTotal' over proper fallible transformers.
Let's get rid of the tryTotalTransformation case and only leave out tryFallibleTransformation and I'll try to take a look at the prioritization issues afterwards.
There was a problem hiding this comment.
Great work, thank you VERY much for your contribution.
This PR adds derivation rules for fallible transforms of coproduct.
Closes #67