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

XorTInstances + cleanup tests #1106

Merged
merged 6 commits into from
Jun 17, 2016
Merged

XorTInstances + cleanup tests #1106

merged 6 commits into from
Jun 17, 2016

Conversation

yilinwei
Copy link
Contributor

@yilinwei yilinwei commented Jun 9, 2016

Add Semigroup and Monoid for XorT.

Also, cleanup the tests so the implicit resolution is clear.

@codecov-io
Copy link

codecov-io commented Jun 9, 2016

Current coverage is 88.64%

Merging #1106 into master will increase coverage by 0.01%

  1. 2 files (not in diff) in ...main/scala/cats/free were modified. more
    • Hits +7
  2. 2 files in ...main/scala/cats/data were modified. more
    • Misses +1
    • Hits +1
  3. File ...ats/std/option.scala (not in diff) was modified. more
@@             master      #1106   diff @@
==========================================
  Files           227        227          
  Lines          2946       2959    +13   
  Methods        2890       2904    +14   
  Messages          0          0          
  Branches         53         52     -1   
==========================================
+ Hits           2611       2623    +12   
- Misses          335        336     +1   
  Partials          0          0          

Sunburst

Powered by Codecov. Last updated by 5cf9da3...41ad7af

@non
Copy link
Contributor

non commented Jun 9, 2016

Looks good to me! Thanks! 👍

@yilinwei
Copy link
Contributor Author

yilinwei commented Jun 10, 2016

As @peterneyens pointed out, the Applicative in this case isn't the same as dervied from the Monad (which one doesn't extend the other).

EDIT2:

Thought about this further and I think that it's not possible to have a Applicative for XorT because it's a monad transformer which means that it needs to be dependent on the other context to change the computation flow.

If XorT could have an Applicative it wouldn't make any sense because it would have to have the same one as F[Xor[?]] which is not what we want from any monad transformer.

Because of that, I'm going to remove the Applicative instance.

EDIT:

So I played around with this, and interestingly enough:

val f = XorT(List("error".left[Int => Int], "error".left[Int => Int]))
val a = XorT(List(42.right[String], "another error".left[Int]))
//f.ap(a) returns List(Left("error"), Left("error"), Left("error"), Left("error"))
//f.flatMap(f => a.map(f)) returns List(Left("error"), Left("error"))

Going to fix this tomorrow.

@yilinwei yilinwei changed the title XorTInstances + cleanup tests XorTInstances + cleanup tests - DO NOT MERGE YET Jun 10, 2016
@xuwei-k
Copy link
Contributor

xuwei-k commented Jun 10, 2016

Thought about this further and I think that it's not possible to have a Applicative for XorT

Yes, it's not possible. see scalaz/scalaz#255

peterneyens added a commit to peterneyens/cats that referenced this pull request Jun 10, 2016
Added `Eq`, `PartialOrder`, `Order`.
Added `SemigroupK`, `MonoidK`
Added `Foldable`, `Traverse`
Added `MonadError`, `MonadCombine` and `MonadFilter` (the last two
instances are dissabled, because they violate the same laws as the
equivalent XorT instances).

Rewrote the tests to use `ListWrapper`.
I used the ListWrapper methods added by @yilinwei in typelevel#1106.
@yilinwei yilinwei changed the title XorTInstances + cleanup tests - DO NOT MERGE YET XorTInstances + cleanup tests Jun 10, 2016
@yilinwei
Copy link
Contributor Author

I've removed the Applicative.

I think it may be good to add a helper method onto XorT to find the Nested applicative like in scalaz but that's a different issue.

peterneyens added a commit to peterneyens/cats that referenced this pull request Jun 10, 2016
Added `Eq`, `PartialOrder`, `Order`.
Added `SemigroupK`, `MonoidK`
Added `Foldable`, `Traverse`
Added `MonadError`, `MonadCombine` and `MonadFilter` (the last two
instances are dissabled, because they violate the same laws as the
equivalent XorT instances).

Rewrote the tests to use `ListWrapper`.
I used the ListWrapper methods added by @yilinwei in typelevel#1106.

def ap[AA >: A, BB >: B, C](that: AA Xor (BB => C)): AA Xor C = that.flatMap(
f => this.map(a => f(a))
)
Copy link
Contributor

Choose a reason for hiding this comment

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

minor: I think this could simply be that.flatMap(this.map)

@ceedubs
Copy link
Contributor

ceedubs commented Jun 16, 2016

👍 thanks!

@non does this still look good to you?

@non
Copy link
Contributor

non commented Jun 17, 2016

LGTM 👍

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.

6 participants