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

Locally defined implicit transformers are ignored #207

Closed
jonathan-ostrander opened this issue Apr 2, 2021 · 2 comments
Closed

Locally defined implicit transformers are ignored #207

jonathan-ostrander opened this issue Apr 2, 2021 · 2 comments
Labels
bug Erroneous behavior in existing features
Milestone

Comments

@jonathan-ostrander
Copy link

jonathan-ostrander commented Apr 2, 2021

Chimney version: 0.6.1 (Latest)

This issue is best demonstrated with the following example:

import io.scalaland.chimney.dsl._
import io.scalaland.chimney.Transformer

case class Foo(a: Int, b: Int)
case class Bar(c: Int = 0)

case class FooWrapper(foo: Foo)
case class BarWrapper(bar: Option[Bar])

// warning below that localT is never used
implicit val t: Transformer[FooWrapper, BarWrapper] = {
  implicit val localT: Transformer[Foo, Bar] = foo => Bar(foo.a + foo.b)
  Transformer.define[FooWrapper, BarWrapper].withFieldRenamed(_.foo, _.baz).buildTransformer
}
// warning: local val localT in value t is never used
//         implicit val localT: Transformer[Foo, Bar] = foo => Bar(foo.a + foo.b)
//                      ^
// error: No warnings can be incurred under -Xfatal-warnings.

// no warning because Bar has a default value in its default constructor
implicit val t: Transformer[FooWrapper, BarWrapper] =
  Transformer.define[FooWrapper, BarWrapper].withFieldRenamed(_.foo, _.baz).buildTransformer

FooWrapper(Foo(1, 2)).transformInto[BarWrapper]
// BarWrapper(Bar(0))

// redefine wrapper transformer with a Transformer[Foo, Bar] outside of a local scope
implicit val fooBarT: Transformer[Foo, Bar] = foo => Bar(foo.a + foo.b)
implicit val newT: Transformer[FooWrapper, BarWrapper] =
  Transformer.define[FooWrapper, BarWrapper].withFieldRenamed(_.foo, _.baz).buildTransformer

FooWrapper(Foo(1, 2)).transformInto[BarWrapper]
// BarWrapper(Bar(3))

There are obvious workarounds. disableDefaultValues can be used when building the wrapper transformer, but this is annoying when defining transformers with code generated by ScalaPB because the unknownFields argument must have a default value specified. Transformers can always be defined outside of a local scope, but sometimes a local scope is desired if multiple transformers require different transformation behavior for a given type in the same package.

@MateuszKubuszok
Copy link
Member

MateuszKubuszok commented Aug 9, 2022

This issue can be solved adding a flag to scalapb:

option (scalapb.options) = {
  preserve_unknown_fields: false // true by default in Proto 3
}

Then your protobufs won't have unknownField generated in the first place.

This will also enable you to use

option (scalapb.options) = {
  no_default_values_in_constructor: true
}

@MateuszKubuszok MateuszKubuszok added the bug Erroneous behavior in existing features label Feb 27, 2023
@MateuszKubuszok
Copy link
Member

From what I checked, this seem to be fixes on our scala-3 branch. I checked in console:

chimney-build(scala-3)> chimney/console
[info] Starting scala interpreter...
Welcome to Scala 2.13.11 (OpenJDK 64-Bit Server VM, Java 17.0.3).
Type in expressions for evaluation. Or try :help.
import io.scalaland.chimney._
import io.scalaland.chimney.dsl._

scala> case class Foo(a: Int, b: Int)
     | case class Bar(c: Int = 0)
     |
     | case class FooWrapper(foo: Foo)
     | case class BarWrapper(bar: Option[Bar])
class Foo
class Bar
class FooWrapper
class BarWrapper

scala> locally {
     |   implicit val localT: Transformer[Foo, Bar] = foo => Bar(foo.a + foo.b)
     |   implicit val t = Transformer.define[FooWrapper, BarWrapper].withFieldRenamed(_.foo, _.bar).buildTransformer
     |   FooWrapper(Foo(1, 2)).transformInto[BarWrapper]
     | }
val res3: BarWrapper = BarWrapper(Some(Bar(3)))

and I didn't feel the need to add a test case for it as there is already one. Once #325 is published if you still find that there is a bug, please reopen.

@MateuszKubuszok MateuszKubuszok added this to the 0.8.0 milestone Jul 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Erroneous behavior in existing features
Projects
None yet
Development

No branches or pull requests

2 participants