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

cats.Order: Support for pattern matching on comparison results #1101

Closed
benhutchison opened this issue Jun 9, 2016 · 5 comments
Closed

cats.Order: Support for pattern matching on comparison results #1101

benhutchison opened this issue Jun 9, 2016 · 5 comments
Assignees

Comments

@benhutchison
Copy link
Member

When doing comparisons with cats.Order, would be desirable to pattern match on the cases, like eg:

a comparison b match {
  case GreaterThan => ???
  case Equivalent => ???
  case LessThan => ???
}

Order.compare returns an Int for performance and Java 1.7 compat reasons, which is fair enough, when you need that. But that requires an if (c <0) ... else if (c == 0) .. else ... construct to handle a comparison; less readable than the above alternative IMO.

Proposal is a function, implemented on top of compare: Int, perhaps named comparison: Comparison that returned a case object (GreaterThan etc) for pleasant pattern matching.

Gitter discussion: https://gitter.im/typelevel/cats?at=5758beb8064b9e7266f0c086

@julienrf
Copy link
Contributor

julienrf commented Jun 9, 2016

Note that you can still get quite the same syntax without changing Order:

object GreaterThan {
  def unapply(i: Int): Boolean = i > 0
}
(a comparison b) match {
  case GreaterThan() => ???
}

adelbertc added a commit to adelbertc/cats that referenced this issue Jun 9, 2016
adelbertc added a commit to adelbertc/cats that referenced this issue Jun 9, 2016
@ceedubs
Copy link
Contributor

ceedubs commented Jun 9, 2016

@julienrf I thought the same thing at first. The problem is that while you do get the syntax, you don't get the exhaustive matching. So if you match on the GT and LT case but forget the EQ case, your code will just blow up at runtime.

@julienrf
Copy link
Contributor

julienrf commented Jun 9, 2016

Fair point :)

@non non closed this as completed in b8bf39d Jun 9, 2016
non added a commit that referenced this issue Jun 9, 2016
Add {C,c}omparison to Order, fixes #1101
@non non removed the in progress label Jun 9, 2016
@benhutchison
Copy link
Member Author

IMO this isnt quite done, since there's no syntax to enable a comparison b.

I think (?) Ive seen some syntax in the cats project autogenerated, maybe from @typeclass; why doesnt Order use that?

@adelbertc
Copy link
Contributor

@benhutchison Ah true, oops forgot about that.

I believe kernel doesn't use simulacrum to minimize dependencies

benhutchison added a commit to benhutchison/cats that referenced this issue Jun 30, 2016
ceedubs added a commit that referenced this issue Jul 1, 2016
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

No branches or pull requests

6 participants