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

Add compareTo overload for Order #1035

Merged
merged 4 commits into from
Oct 6, 2018

Conversation

Mishkun
Copy link
Contributor

@Mishkun Mishkun commented Oct 6, 2018

This pull-request provides compareTo operators for all Order instances as described in #729.
There are two more operators mentioned in #729:

  • Equality operator (==, !=) for Eq instances - it is impossible to implement that as extension function, because member functions always shadow extensions and equals is defined for every object in Kotlin (because of Java legacy)
  • Plus operator (+) for Semigroup instances - _already added in Semigroup + syntax #768 _

It seems that all three items of issue would resolved after this PR is merged, so it will close #729

This compareTo should just delegate to Order#compare every time. No way to ensure that using kotlin type system (there are no final members in interfaces allowed)
(or at least gives the same results)
Though there are no way to make method final in interface, we can always write a test
@nomisRev
Copy link
Member

nomisRev commented Oct 6, 2018

Should we replace compare by compareTo since they're the same function?

This would also make a nice example of the usefulness of typeclasses. Since many people are familiar with the compareTo operator magic, we can explain how you could do it his easily with typeclasses yourself.

@codecov
Copy link

codecov bot commented Oct 6, 2018

Codecov Report

Merging #1035 into master will decrease coverage by 0.02%.
The diff coverage is 0%.

Impacted file tree graph

@@             Coverage Diff             @@
##             master   #1035      +/-   ##
===========================================
- Coverage     47.72%   47.7%   -0.03%     
  Complexity      725     725              
===========================================
  Files           333     333              
  Lines          8479    8483       +4     
  Branches        771     772       +1     
===========================================
  Hits           4047    4047              
- Misses         4143    4147       +4     
  Partials        289     289
Impacted Files Coverage Δ Complexity Δ
...-test/src/main/kotlin/arrow/test/laws/OrderLaws.kt 0% <0%> (ø) 0 <0> (ø) ⬇️
...classes/src/main/kotlin/arrow/typeclasses/Order.kt 0% <0%> (ø) 0 <0> (ø) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b3a2832...87f4572. Read the comment docs.

@pakoito
Copy link
Member

pakoito commented Oct 6, 2018

For consistency with the other typeclasses, I'd keep them separated for now. We can still use it as an example, and deprecate later.

@Mishkun Thanks for the PR, well done adding the laws and docs too :)

@pakoito pakoito merged commit bc3f363 into arrow-kt:master Oct 6, 2018
@raulraja raulraja mentioned this pull request Nov 2, 2018
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.

Add operator overloads to Syntax classes
3 participants