Add some more OptionT instances. #1103

peterneyens · 2016-06-09T11:37:52Z

Added Eq, PartialOrder and Order.
Added Semigroup and Monoid.
Added SemigroupK and MonoidK
Added MonadError, MonadCombine and MonadFilter (the last two
instances are dissabled, because they violate the same laws as the
equivalent XorT instances).

codecov-io · 2016-06-09T12:17:31Z

Current coverage is 88.78%

Merging #1103 into master will increase coverage by 0.15%

2 files (not in diff) in ...main/scala/cats/data were modified. more
- Misses -1
- Hits +1
2 files (not in diff) in .../src/main/scala/cats were modified. more
- Misses -1
- Hits +1

@@             master      #1103   diff @@
==========================================
  Files           227        227          
  Lines          2946       2968    +22   
  Methods        2890       2910    +20   
  Messages          0          0          
  Branches         53         56     +3   
==========================================
+ Hits           2611       2635    +24   
+ Misses          335        333     -2   
  Partials          0          0

Powered by Codecov. Last updated by b149835...2183b75

peterneyens · 2016-06-10T11:14:46Z

tests/src/test/scala/cats/tests/OptionTTests.scala

+
+    implicit val monadError = OptionT.catsDataMonadErrorForOptionT[SXor, String]
+
+    // checkAll("OptionT[String Xor ?, Int]", MonadErrorTests[OptionT[SXor, ?], String].monadError[Int, Int, Int])


I need an Arbitrary[λ[(α, β) => OptionT[String Xor α, β]]] which I don't seem to get from the Arbitrary instances of Xor and OptionT.

I need to figure how to create this instance and if it has a place in laws.discipline.arbitrary.

peterneyens · 2016-06-10T11:48:55Z

The only instances currently still missing which can added for OptionT are Semigroup and Monoid, if I'm correct.

I'm currently failing to provide an Arbitrary to run the MonadErrorTests (see my comment in the code). If some might have some pointers ... they would be greatly appreciated 😉.

I am also not sure why the partialOrder lines don't seem to have test coverage, I thought they were included in OrderLaws ?

peterneyens · 2016-06-10T20:59:28Z

tests/src/test/scala/cats/tests/OptionTTests.scala

+
+    implicit val iso = CartesianTests.Isomorphisms.invariant[OptionT[SXor, ?]]
+
+    checkAll("OptionT[String Xor ?, Int]", MonadErrorTests[OptionT[SXor, ?], String].monadError[Int, Int, Int])


MonadErrorTests is working now, but I had to supply all the Arbitrary and Eq instances myself, I don't know if there is a better way ?

The better way is probably #1073 but let's hold off on that for this PR.

@yilinwei

Added `Eq`, `PartialOrder`, `Order`. Added `Semigroup`, `Monoid` 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 # 1106.

ceedubs · 2016-06-11T12:49:49Z

core/src/main/scala/cats/data/OptionT.scala

+  /* TODO violates right absorbtion, right distributivity, and left distributivity -- re-enable when MonadCombine laws are split in to weak/strong
+  implicit def catsDataMonadCombineForOptionT[F[_]](implicit F0: Monad[F]): MonadCombine[OptionT[F, ?]] =
+    new OptionTMonadCombine[F] { implicit val F = F0 }
+  */


I think we should probably track this (and the other below) with a GitHub issue instead of commented-out code.

I followed the example of XorT to include them disabled.

Do I drop the implicit functions, the traits OptionTMonadFilter and OptionTMonadCombine and their tests ?

@peterneyens that's what I'd be inclined to do. We always have git history if people want to pull them back out (and ideally the GitHub issue tracking this could link to the previous implementation).

ceedubs · 2016-06-11T12:53:53Z

Thanks a bunch @peterneyens, this is great!

I left a minor comment about commented-out code, but other than that 👍.

Clean up the test of Monoid and Semigroup.

xuwei-k · 2016-06-14T05:58:18Z

[error] /home/travis/build/xuwei-k/cats/tests/src/test/scala/cats/tests/OptionTTests.scala:12: ambiguous implicit values:
[error]  both method catsDataMonadForOptionT in trait OptionTInstances0 of type [F[_]](implicit F0: cats.Monad[F])cats.Monad[[β]cats.data.OptionT[F,β]]
[error]  and method catsDataMonadErrorForOptionT in trait OptionTInstances0 of type [F[_], E](implicit F0: cats.MonadError[F,E])cats.MonadError[[β]cats.data.OptionT[F,β],E]
[error]  match expected type cats.Monad[[β]cats.data.OptionT[scala.util.Try,β]]
[error]   Monad[OptionT[scala.util.Try, ?]]
[error]        ^
[error] one error found

peterneyens · 2016-06-14T07:57:55Z

@xuwei-k Thanks for pointing this out, I already found it kind of strange that this didn't give any problems and didn't really think this through.

ceedubs · 2016-06-14T10:58:03Z

core/src/main/scala/cats/data/OptionT.scala

+  implicit def catsDataMonadErrorForOptionT[F[_], E](implicit F0: MonadError[F, E]): MonadError[OptionT[F, ?], E] =
+    new OptionTMonadError[F, E] { implicit val F = F0 }
+
+  implicit def catsDataSemigroupK[F[_]](implicit F0: Monad[F]): SemigroupK[OptionT[F, ?]] =


I think you want a forOptionT at the end of this name.

ceedubs · 2016-06-14T11:02:05Z

One more minor comment, then I think this is good to go!

Add a "test" to check the implicit resolution of OptionT instances.

ceedubs · 2016-06-14T12:58:06Z

👍

kailuowang · 2016-06-15T14:34:18Z

This is awesome. LGTM 👍

peterneyens force-pushed the optiont-instances branch 2 times, most recently from 7ba501a to 67c7f39 Compare June 10, 2016 11:02

peterneyens reviewed Jun 10, 2016
View reviewed changes

peterneyens mentioned this pull request Jun 10, 2016

Applicative/Functor instance missing for OptionT #1107

Closed

peterneyens force-pushed the optiont-instances branch 2 times, most recently from 26a99a9 to bf91788 Compare June 10, 2016 20:57

peterneyens reviewed Jun 10, 2016
View reviewed changes

peterneyens force-pushed the optiont-instances branch from bf91788 to 66332e5 Compare June 10, 2016 21:47

peterneyens force-pushed the optiont-instances branch from 66332e5 to bbd3e12 Compare June 10, 2016 22:43

ceedubs reviewed Jun 11, 2016
View reviewed changes

ceedubs mentioned this pull request Jun 11, 2016

XorTInstances + cleanup tests #1106

Merged

peterneyens force-pushed the optiont-instances branch from 01d8fd0 to c20a25b Compare June 13, 2016 21:58

Remove unlawful MonadFilter and MonadCombine instances of OptionT.

c3cae9e

Clean up the test of Monoid and Semigroup.

peterneyens force-pushed the optiont-instances branch from c20a25b to c3cae9e Compare June 13, 2016 22:58

ceedubs reviewed Jun 14, 2016
View reviewed changes

Fix some implicit resolution issues in OptionT.

2183b75

Add a "test" to check the implicit resolution of OptionT instances.

peterneyens force-pushed the optiont-instances branch from 1dffb84 to 2183b75 Compare June 14, 2016 11:27

kailuowang merged commit 8e1861d into typelevel:master Jun 15, 2016

peterneyens deleted the optiont-instances branch July 18, 2016 12:06

peterneyens mentioned this pull request Jun 2, 2017

More instances for monad transformers #1467

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add some more OptionT instances. #1103

Add some more OptionT instances. #1103

peterneyens commented Jun 9, 2016 •

edited

Loading

codecov-io commented Jun 9, 2016 •

edited

Loading

peterneyens Jun 10, 2016

peterneyens commented Jun 10, 2016 •

edited

Loading

peterneyens Jun 10, 2016

ceedubs Jun 11, 2016

ceedubs Jun 11, 2016

peterneyens Jun 11, 2016

ceedubs Jun 13, 2016

ceedubs commented Jun 11, 2016

xuwei-k commented Jun 14, 2016

peterneyens commented Jun 14, 2016

ceedubs Jun 14, 2016

ceedubs commented Jun 14, 2016

ceedubs commented Jun 14, 2016

kailuowang commented Jun 15, 2016


		implicit val monadError = OptionT.catsDataMonadErrorForOptionT[SXor, String]

		// checkAll("OptionT[String Xor ?, Int]", MonadErrorTests[OptionT[SXor, ?], String].monadError[Int, Int, Int])


		implicit val iso = CartesianTests.Isomorphisms.invariant[OptionT[SXor, ?]]

		checkAll("OptionT[String Xor ?, Int]", MonadErrorTests[OptionT[SXor, ?], String].monadError[Int, Int, Int])

Add some more OptionT instances. #1103

Add some more OptionT instances. #1103

Conversation

peterneyens commented Jun 9, 2016 • edited Loading

codecov-io commented Jun 9, 2016 • edited Loading

Current coverage is 88.78%

peterneyens Jun 10, 2016

Choose a reason for hiding this comment

peterneyens commented Jun 10, 2016 • edited Loading

peterneyens Jun 10, 2016

Choose a reason for hiding this comment

ceedubs Jun 11, 2016

Choose a reason for hiding this comment

ceedubs Jun 11, 2016

Choose a reason for hiding this comment

peterneyens Jun 11, 2016

Choose a reason for hiding this comment

ceedubs Jun 13, 2016

Choose a reason for hiding this comment

ceedubs commented Jun 11, 2016

xuwei-k commented Jun 14, 2016

peterneyens commented Jun 14, 2016

ceedubs Jun 14, 2016

Choose a reason for hiding this comment

ceedubs commented Jun 14, 2016

ceedubs commented Jun 14, 2016

kailuowang commented Jun 15, 2016

peterneyens commented Jun 9, 2016 •

edited

Loading

codecov-io commented Jun 9, 2016 •

edited

Loading

peterneyens commented Jun 10, 2016 •

edited

Loading