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

Add Defer instances for Eq, Hash, Order, Show, and variants #4414

Merged
merged 10 commits into from
Jul 10, 2023
Merged

Add Defer instances for Eq, Hash, Order, Show, and variants #4414

merged 10 commits into from
Jul 10, 2023

Conversation

morgen-peschke
Copy link
Contributor

Implements #4413

@armanbilge armanbilge mentioned this pull request Apr 1, 2023
Closed
@morgen-peschke morgen-peschke mentioned this pull request Apr 12, 2023
Merged
johnynek
johnynek previously approved these changes Jul 6, 2023
View reviewed changes
@morgen-peschke
Copy link
Contributor Author

Note to self: local reproduction of the mima issue

sbt 'project rootJVM' '++ 2.12.18!' mimaReportBinaryIssues

@johnynek
Copy link
Contributor

johnynek commented Jul 7, 2023

I really like the style this enables!

johnynek
johnynek previously approved these changes Jul 7, 2023
View reviewed changes
Copy link
Contributor

@johnynek johnynek left a comment

Choose a reason for hiding this comment

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

@armanbilge are we ready to merge? Can you take a second if so?

johnynek
johnynek previously approved these changes Jul 7, 2023
View reviewed changes
armanbilge
armanbilge reviewed Jul 8, 2023
View reviewed changes
Copy link
Member

@armanbilge armanbilge 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, just a couple typos!

tests/shared/src/test/scala/cats/tests/HashSuite.scala Outdated Show resolved Hide resolved
tests/shared/src/test/scala/cats/tests/OrderingSuite.scala Outdated Show resolved Hide resolved
danicheg
danicheg reviewed Jul 9, 2023
View reviewed changes
Copy link
Member

@danicheg danicheg left a comment

Choose a reason for hiding this comment

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

The entire job is just great! A few minor nitpicks from me

core/src/main/scala/cats/instances/eq.scala Show resolved Hide resolved
core/src/main/scala/cats/instances/eq.scala
object EqInstances {
private val catsDeferForEqCache: Defer[Eq] =
new Defer[Eq] {
case class Deferred[A](fa: () => Eq[A]) extends Eq[A] {
Copy link
Member

Choose a reason for hiding this comment

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

Just a stylish nitpick that perhaps doesn't affect anything. I'm not confident about putting underlying classes to the value instantiation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't usually use this style, and went with it to remain consistent with the existing instances (like cats.instances.FunctionInstancesBinCompat0#catsSddDeferForFunction0).

As I usually avoid it for stylistic reasons, I'm not familiar with the practical concerns around doing it this way, so I'd be interested in hearing why you're worried about it.

@morgen-peschke @armanbilge
Update tests/shared/src/test/scala/cats/tests/HashSuite.scala
ed24c98 
Co-authored-by: Arman Bilge <armanbilge@gmail.com>
@morgen-peschke @armanbilge
Update tests/shared/src/test/scala/cats/tests/OrderingSuite.scala
71b6252 
Co-authored-by: Arman Bilge <armanbilge@gmail.com>
@armanbilge armanbilge changed the title Add Defer instances for Eq, Hash, Order, Show, and variants Add Defer instances for Eq, Hash, Order, Show, and variants Jul 10, 2023
armanbilge
armanbilge approved these changes Jul 10, 2023
View reviewed changes
Copy link
Member

@armanbilge armanbilge left a comment

Choose a reason for hiding this comment

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

Thanks!

@johnynek johnynek merged commit 65a3c5b into typelevel:main Jul 10, 2023
@morgen-peschke morgen-peschke deleted the additional-defer-instances branch July 10, 2023 20:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@danicheg danicheg danicheg left review comments

@armanbilge armanbilge armanbilge approved these changes

@johnynek johnynek johnynek left review comments

Assignees
No one assigned
Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@morgen-peschke @johnynek @armanbilge @danicheg