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

Added an ApplicativeError instance for Kleisli and a MonadError[Option, Unit] to std.option #1029

Merged
merged 3 commits into from
May 13, 2016

Conversation

kailuowang
Copy link
Contributor

No description provided.

@@ -15,8 +15,25 @@ class KleisliTests extends CatsSuite {
implicit def kleisliEq[F[_], A, B](implicit A: Arbitrary[A], FB: Eq[F[B]]): Eq[Kleisli[F, A, B]] =
Eq.by[Kleisli[F, A, B], A => F[B]](_.run)

implicit val xorT = XorT.xorTEq[Kleisli[Option, Int, ?], Unit, Int]

implicit val optionApplicativeError : ApplicativeError[Option, Unit] = new ApplicativeError[Option, Unit] {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to just move this into the main source?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, I don't really know the use case for it. But one could be like here, in a kleisli, anything else?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also curious how @ceedubs thinks about this, he suggested it "weird" in gitter right?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it could give users an option (hah) to plug in Option if working with a MonadError API (I'm guessing Option can implement MonadError, not just ApplicativeError) when they don't care about the errors

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, it's a little weird, but I think I'd approve of it living in the main source. I also want it for #689 :)

@codecov-io
Copy link

codecov-io commented May 13, 2016

Current coverage is 88.24%

Merging #1029 into master will decrease coverage by -0.52%

  1. 6 files (not in diff) in ...in/scala/cats/kernel were modified. more
    • Misses +11
    • Hits -11
  2. 1 files (not in diff) in kernel were modified. more
    • Misses +3
    • Hits -3
  3. 2 files (not in diff) in ...n/scala/cats/functor were modified. more
  4. 2 files (not in diff) in ...main/scala/cats/data were modified. more
  5. 1 files (not in diff) in .../src/main/scala/cats were modified. more
    • Misses +1
    • Hits -1
  6. File ...ats/std/option.scala was modified. more
@@             master      #1029   diff @@
==========================================
  Files           215        215          
  Lines          2713       2720     +7   
  Methods        2649       2657     +8   
  Messages          0          0          
  Branches         59         58     -1   
==========================================
- Hits           2408       2400     -8   
- Misses          305        320    +15   
  Partials          0          0          

Sunburst

Powered by Codecov. Last updated by 15fc946...2068080

@kailuowang
Copy link
Contributor Author

Moved MonadError[Option, Unit] to main.

@kailuowang kailuowang changed the title Added an ApplicativeError instance for Kleisli Added an ApplicativeError instance for Kleisli and a MonadError[Option, Unit] to std.option May 13, 2016
implicit val iso = CartesianTests.Isomorphisms.invariant[Kleisli[Option, Int, ?]]

checkAll("ApplicativeError[Klesili[Option, Int, Int], Unit]", ApplicativeErrorTests[Kleisli[Option, Int, ?], Unit].applicativeError[Int, Int, Int])
Copy link
Contributor

Choose a reason for hiding this comment

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

Should probably run SerializableTests on this.

@@ -17,6 +17,9 @@ class OptionTests extends CatsSuite {
checkAll("Option[Int] with Option", TraverseTests[Option].traverse[Int, Int, Int, Int, Option, Option])
checkAll("Traverse[Option]", SerializableTests.serializable(Traverse[Option]))

checkAll("Option with Unit", MonadErrorTests[Option, Unit].monadError[Int, Int, Int])
checkAll("MonadError[Option, Unit]", SerializableTests.serializable(MonadErrorTests[Option, Unit]))
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 attempting to serialize MonadErrorTests instead of the MonadError instance for Option.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oopz

@ceedubs
Copy link
Contributor

ceedubs commented May 13, 2016

👍 thanks!

@adelbertc
Copy link
Contributor

LGTM 👍

@adelbertc adelbertc merged commit 53dc641 into typelevel:master May 13, 2016
@kailuowang kailuowang deleted the applicativeErrorKleisli branch May 13, 2016 16:33
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.

4 participants