-
Notifications
You must be signed in to change notification settings - Fork 358
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
Ensure backward compatibility of Hikari Config #1842
Ensure backward compatibility of Hikari Config #1842
Conversation
Thanks @sideeffffect I'm not too sure what motivated this change given that we're breaking back compat in 1.0? |
The point is to enable backward compatible evolution of Binary compatibility with case classes is a tricky topic in Scala https://docs.scala-lang.org/overviews/core/binary-compatibility-for-library-authors.html#evolving-code-without-breaking-binary-compatibility |
If you want to keep backwards binary compatibility you'll need to use package private I think. Using private makes it unavailable at the bytecode level. |
Are you referring to the access modifier of the constructor of |
I've added a demo of making method private breaking bin compat https://github.com/jatcwang/binary-compatibility-guide
You can see that projects like cats use package private to maintain bin compat. |
Of course it breaks 😃 There's probably a misunderstanding. I know the changes that I'm proposing in this PR are breaking bin compatibility. I'm introducing them so that once we get to |
) | ||
|
||
@nowarn | ||
def fromProduct(p: Product): Config = p.productArity match { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry not sure why we need this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Scala 3 defines fromProduct
in companion object of case classes. If you add a new field to the case class, you need to add a new case to fromProduct
to ensure compatibility with old code which counts with the old number of fields.
I've improved the comment on top of the case class.
@jatcwang so what do you think? Can we merge this? |
Will need some time @sideeffffect. I ran javap on a Scala 3 compiled case class but still couldn't find |
Did you look into the companion object? |
@jatcwang do you think we could merge this PR? |
@jatcwang ping 😸 |
/cc @jatcwang