Skip to content

Order.fromComparable Method Signature Change to Allow java.sql.Date #4734

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

Merged
merged 4 commits into from
Apr 10, 2025

Conversation

ikeyan
Copy link
Contributor

@ikeyan ikeyan commented Mar 29, 2025

The Order.fromComparable method signature in Order.scala has been modified to allow it to accept java.sql.Date.
This change enables the method to work with types such as java.sql.Date and other subtypes of java.util.Date which implements Comparable<java.util.Date>.

@satorg
Copy link
Contributor

satorg commented Apr 2, 2025

Hi @ikeyan , thank you for the PR, it can be helpful indeed!
I think it would be also nice if we could have a test that shows how the change works.

@ikeyan
Copy link
Contributor Author

ikeyan commented Apr 2, 2025

@satorg I added a test in OrderSuite.scala

Copy link
Contributor

@satorg satorg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, thanks!

def fromComparable[A <: Comparable[A]]: Order[A] = _ compareTo _
private type ContravariantComparable[A] = Comparable[? >: A]

def fromComparable[A <: ContravariantComparable[A]]: Order[A] = _ compareTo _
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems to me like this should be implicit potentially, perhaps at a lower priority.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@johnynek if an implicit parameter is added, would that break binary compatibility?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't mean an implicit parameter, I mean implicit def from comparable...

This won't break JVM binary compatibility. But maybe we need to lower the priority of this implicit by putting it in a trait and extending here.

@johnynek johnynek merged commit 324d528 into typelevel:main Apr 10, 2025
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants