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.

Sign up for GitHub

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

Jump to bottom

Deprecate universal Equiv in Scala 2.13 #7358

Closed
wants to merge 1 commit into from
Closed

Deprecate universal Equiv in Scala 2.13 #7358

wants to merge 1 commit into from

Conversation

ceedubs
Copy link
Contributor

@ceedubs ceedubs commented Oct 22, 2018

This is a replacement for #7187 (see discussion there).

This does the following things:

  1. Deprecate universal Equiv instance.
  2. Create an Ordering -> PartialOrdering conversion.
  3. Create a PartialOrdering -> Equiv conversion at a higher priority
    than the universal instance.
  4. Add some unit tests that check that implicit resolution is working as
    expected.

@NthPortal suggested the conversions as a simple way to have Equiv
instances that could be resolved without duplicating a lot of the work
that has been done for Order instances for basic types.

@ceedubs ceedubs mentioned this pull request Oct 22, 2018
Closed
@scala-jenkins scala-jenkins added this to the 2.13.0-RC1 milestone Oct 22, 2018
NthPortal
NthPortal reviewed Oct 23, 2018
View reviewed changes
src/library/scala/math/PartialOrdering.scala Outdated
object PartialOrdering {
implicit def partialOrderingFromOrdering[T](implicit ev: Ordering[T]): PartialOrdering[T] = ev

@inline def apply[T](implicit ev: PartialOrdering[T]): Equiv[T] = ev
Copy link
Contributor

Choose a reason for hiding this comment

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

I would think this should return PartialOrdering[T]

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
@inline def apply[T](implicit ev: PartialOrdering[T]): Equiv[T] = ev
@inline def apply[T](implicit ev: PartialOrdering[T]): PartialOrdering[T] = ev

Copy link
Contributor Author

@ceedubs ceedubs Oct 23, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Ugh sloppy copy and paste error. And the universal instance masked the error. I'll fix this and improve the unit tests to make sure that they are resolving the expected instance.

Thank you; good catch.

NthPortal
NthPortal reviewed Oct 23, 2018
View reviewed changes
src/library/scala/math/Equiv.scala
@@ -58,4 +69,6 @@ object Equiv extends LowPriorityEquiv {
((x, y) => implicitly[Equiv[S]].equiv(f(x), f(y)))

@inline def apply[T: Equiv]: Equiv[T] = implicitly[Equiv[T]]

implicit def equivFromPartialOrdering[T](implicit ev: PartialOrdering[T]): Equiv[T] = ev
Copy link
Contributor

Choose a reason for hiding this comment

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

this is probably going to produce very confusing deprecation messages for Equiv[Float] and Equiv[Double], but don't worry about that. I'll improve it in a follow-up PR. that's not your problem to have to deal with

NthPortal
NthPortal approved these changes Oct 23, 2018
View reviewed changes
Copy link
Contributor

@NthPortal NthPortal left a comment

Choose a reason for hiding this comment

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

Looks pretty good to me! Just the one concern about the return type of Equiv.apply

@ceedubs
Deprecate universal Equiv in Scala 2.13
fe245fb 
This is a replacement for scala#7187 (see discussion there).

This does the following things:

1. Deprecate universal Equiv instance.
2. Create an Ordering -> PartialOrdering conversion.
3. Create a PartialOrdering -> Equiv conversion at a higher priority
than the universal instance.
4. Add some unit tests that check that implicit resolution is working as
expected.

@NthPortal suggested the conversions as a simple way to have `Equiv`
instances that could be resolved without duplicating a lot of the work
that has been done for `Order` instances for basic types.
@ceedubs ceedubs force-pushed the deprecate-universal-equiv-2.13 branch from f528d2e to fe245fb Compare October 23, 2018 14:46
lrytz
lrytz approved these changes Oct 26, 2018
View reviewed changes
Copy link
Member

@lrytz lrytz left a comment

Choose a reason for hiding this comment

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

LGTM too!

@SethTisue
Copy link
Member

I’d like to run this through the community build before we merge.

@ceedubs
Copy link
Contributor Author

ceedubs commented Oct 27, 2018

It occurs to me that there should probably be instances for Equiv[Option[A]] given Equiv[A], and similarly for other common data types like collections, Either, etc. Whether or not that should happen within this PR I don't know.

@NthPortal
Copy link
Contributor

@ceedubs I've given up on the idea of a good solution here, and have copied the code from Ordering to Equiv (and edited the names and methods to make sense). I based it on current 2.13.x. You can find it at NthPortal@7e7db7b. Feel free to cherry-pick it and incorporate it into your branch however you like.

@NthPortal NthPortal self-assigned this Oct 28, 2018
@SethTisue SethTisue self-assigned this Nov 6, 2018
@ceedubs
Copy link
Contributor Author

ceedubs commented Nov 6, 2018

Thanks @NthPortal.

For the concrete types like Int, etc, is there motivation for copying instances in the Equiv object as opposed to passing through to the Order instance?

Also I apologize in advance, because the next couple of weeks are really busy for me, so I doubt that I'll have time to revisit this PR during that time.

@NthPortal
Copy link
Contributor

@ceedubs for the ones that aren't Float and Double, not really. I don't feel like it makes a big difference either way (I can assure you it was the tiniest fraction of the copying work). I would be inclined to give them their own instances, to perhaps make it slightly less confusing to newcomers looking at the code? But I am open to being convinced that it would be better to use the Ordering instances.

Float and Double need their own logic, for reasons (the trail starts at #6410; I'm sorry it's not all in one place). They could potentially still use the Ordering instances, but that would end up with mixing objects and vals, which I feel would be confusing.

@lrytz
Copy link
Member

lrytz commented Nov 7, 2018

@NthPortal given that @ceedubs says he doesn't have time in the next couple weeks, would you like to continue and submit a fresh PR combining this PR and your branch?

@NthPortal
Copy link
Contributor

NthPortal commented Nov 7, 2018 via email

@ceedubs
Copy link
Contributor Author

ceedubs commented Nov 9, 2018

Thanks so much for all of the help on this, @NthPortal! Sorry that I haven't been more on top of this.

@NthPortal
Copy link
Contributor

@ceedubs no worries

@NthPortal NthPortal mentioned this pull request Nov 11, 2018
Merged
@SethTisue
Copy link
Member

superseded by #7414

@SethTisue SethTisue closed this Nov 27, 2018
@SethTisue SethTisue removed this from the 2.13.0-RC1 milestone Nov 27, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lrytz lrytz lrytz approved these changes

@NthPortal NthPortal NthPortal approved these changes

Assignees

@SethTisue SethTisue

@NthPortal NthPortal

Labels
None yet
Projects
None yet
Milestone
No milestone
5 participants
@ceedubs @SethTisue @NthPortal @lrytz @scala-jenkins