Replace Split with Commutative Arrow. Continuing #1719 #1766

kailuowang · 2017-07-13T12:58:50Z

per conversation with @diesalbla, this it the PR attempting to complete #1719

edmundnoble · 2017-07-14T05:41:32Z

My review is at the moment to use locally on line 42 of CokleisliTests.

codecov-io · 2017-07-14T14:13:18Z

Codecov Report

Merging #1766 into master will decrease coverage by 0.15%.
The diff coverage is 85.96%.

@@            Coverage Diff             @@
##           master    #1766      +/-   ##
==========================================
- Coverage   95.41%   95.26%   -0.16%     
==========================================
  Files         256      265       +9     
  Lines        4207     4283      +76     
  Branches       90       97       +7     
==========================================
+ Hits         4014     4080      +66     
- Misses        193      203      +10

Impacted Files	Coverage Δ
core/src/main/scala/cats/CommutativeFlatMap.scala	`0% <0%> (ø)`
core/src/main/scala/cats/CommutativeMonad.scala	`0% <0%> (ø)`
...e/src/main/scala/cats/arrow/CommutativeArrow.scala	`0% <0%> (ø)`
...rc/main/scala/cats/laws/CommutativeMonadLaws.scala	`100% <100%> (ø)`
core/src/main/scala/cats/instances/option.scala	`100% <100%> (ø)`	⬆️
...a/cats/laws/discipline/CommutativeMonadTests.scala	`100% <100%> (ø)`
core/src/main/scala/cats/data/WriterT.scala	`93.54% <100%> (+0.07%)`	⬆️
core/src/main/scala/cats/instances/function.scala	`100% <100%> (ø)`	⬆️
core/src/main/scala/cats/package.scala	`100% <100%> (ø)`	⬆️
laws/src/main/scala/cats/laws/ArrowLaws.scala	`100% <100%> (ø)`	⬆️
... and 26 more

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 8e38d9d...f34c6e5. Read the comment docs.

edmundnoble

Overall this is excellent, though I fear we're missing an instance (or two, see my other comment); CommutativeMonad[WriterT[F, L, ?]] where there's a CommutativeMonad[F] and CommutativeMonoid[L].

If we introduce CommutativeCartesian and friends we'll also have an instance of that Validated[E, ?] given CommutativeSemigroup[E]... but I don't see it being necessary in this PR.

edmundnoble · 2017-07-14T15:56:47Z

core/src/main/scala/cats/arrow/Arrow.scala

+/**
+ * Must obey the laws defined in cats.laws.ArrowLaws.
+ */
+@typeclass trait Arrow[F[_, _]] extends Category[F] with Strong[F] { self =>


edmundnoble · 2017-07-14T15:56:54Z

core/src/main/scala/cats/arrow/Arrow.scala

+   * Lift a function into the context of an Arrow.
+   *
+   * In the reference articles "Arrows are Promiscuous...", and in the corresponding Haskell
+   * library `Control.Arrow`, this function is called `arr`.


edmundnoble · 2017-07-14T15:57:01Z

core/src/main/scala/cats/arrow/Arrow.scala

@@ -20,6 +26,25 @@ import simulacrum.typeclass
    compose(swap, compose(first[A, B, C](fa), swap))
  }

-  override def split[A, B, C, D](f: F[A, B], g: F[C, D]): F[(A, C), (B, D)] =
+  /**


edmundnoble · 2017-07-14T16:01:04Z

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

-  implicit val catsDataArrowForKleisliId: Arrow[Kleisli[Id, ?, ?]] =
-    catsDataArrowForKleisli[Id]
+  implicit val catsDataCommutativeArrowForKleisliId: CommutativeArrow[Kleisli[Id, ?, ?]] =
+    catsDataCommutativeArrowForKleisli[Id]

  implicit def catsDataMonadReaderForKleisliId[A]: MonadReader[Kleisli[Id, A, ?], A] =


Kleisli[F, A, B] is seemingly a commutative monad now too, given F[_]: CommutativeMonad. ReaderT has no added effects, so I think this has to be the case.

This commit removes from the `core` module the `Split` typeclass. This includes the following changes: * The `Arrow` typeclass is no longer a subtype of `Split`. * The `split` operation, previously in the `Split` typeclass, is now integrated in the `Arrow` typeclass, with a default implementation. * Following notation from the `Control.Arrow` library for Haskell, we use the operator `***` as an alias for `split`. * `SplitSyntax` is replaced with `ArrowSyntax`. * We remove the `SplitLaws` and the `SplitTests`. In consequence, ArrowLaws does not inherit from SplitLaws anymore. * We modify the instances for `Kleisli`. We remove the trait `KleisliSpli`, and we replace the _factory_ method that was generating it to generate a `KleisliCompose` instead. * We remove the tests on the `SplitLaws` for Kleisli and CoKleisli

We add a type-class of commutative arrows, which are those in which the `split` operation is commutative (can pair in any order).

We introduce a Commutative Monad Type-class, a subclass of Monad in which the flatMap of independent operations is commutative.

We introduce some instances of CommutativeArrow for Kleisli, which are based on CommutativeMonad.

We introduce a new type class, CommutativeFlatMap, to load the commutativity law one level up.

We introduce the duals of Commutative Comonads and CoflatMap, as the duals of commutative Flatmaps and Monads. This is done to generate and test CommutativeArrow instances for the Cokleisli datatype.

We complete the tests for the CommutativeArrow instance of Cokleisli. To use it, we mark the Monad instance of `NonEmptyList` as a Commutative Monad.

kailuowang · 2017-07-17T14:38:44Z

@edmundnoble you are right, both are CommutativeMonad, instances added.

peterneyens · 2017-07-18T16:24:12Z

Do we now if there are other CommutativeCoflatMap and/or CommutativeComonad instances than for Id? Otherwise I am not sure if these are worth adding ... ?

We can also make the Arrow[Function1] instance a CommutativeArrow[Function1], right?

peterneyens · 2017-07-18T16:27:40Z

tests/src/test/scala/cats/tests/NonEmptyListTests.scala

 import cats.data.{NonEmptyList, NonEmptyVector}
-import cats.laws.discipline.{ComonadTests, MonadTests, NonEmptyTraverseTests, ReducibleTests, SemigroupKTests, SerializableTests}
+import cats.laws.discipline.{SemigroupKTests, MonadTests, SerializableTests, NonEmptyTraverseTests, ReducibleTests, ComonadTests}


Maybe we can leave these alphabetically?

peterneyens · 2017-07-18T16:29:07Z

tests/src/test/scala/cats/tests/KleisliTests.scala

@@ -238,9 +254,8 @@ class KleisliTests extends CatsSuite {
    MonadReader[Reader[Int, ?], Int]
    Monoid[Reader[Int, String]]
    MonoidK[λ[α => Reader[α, α]]]
-    Arrow[Reader[?, ?]]
+    CommutativeArrow[Reader[?, ?]]


We can leave Arrow[Reader[?, ?]] here (move it below CommutativeArrow).

We can add Arrow[Kleisli[Id, ?, ?]] above as well.

kailuowang · 2017-07-18T18:37:01Z

@peterneyens I made function1 also commutative and correct the tests.
The CommutativeComonad was introduced because Reader is a CommutativeArrow thanks to Id being a CommutativeComonad. Other than that I am not aware any other instances of CommutativeComonad, @diesalbla thought NonEmptyList is one but it turns out it isn't (or at least the law doesn't think so). I think there is value for Reader to be CommutativeArrow. We could remove CommutativeComonad and make a dedicated CommutativeArrow instance for Reader, not sure if that's a better solution though.

peterneyens · 2017-07-18T20:48:55Z

The CommunativeComonad is used in the CommutativeArrow instance of Cokleisli, right?
Reader has a CommutativeArrow because Id has a CommutativeMonad.

I was just thinking that if we can't come up with other instances, adding CommunativeCoflatMap and CommutativeComonad might not be worth it, given that it currently only gives us a CommutativeArrow for Cokleisli[Id, ?, ?] (also see #1364).

I think this PR is good to be merged 👍 (thanks @diesalbla and @kailuowang) ; I just wanted to hear other opinions if CommutativeCoflatMap and CommutativeComonad are useful enough to merit inclusion.

edmundnoble · 2017-07-19T05:10:14Z

Agreed with @peterneyens, I'd need at least one example of a commutative comonad/coflatmap before adding that but otherwise 👍 for merging.

kailuowang · 2017-07-19T12:42:33Z

@peterneyens you are right, it was Cokleisli that is benefiting from it not Reader, I agree with you and @edmundnoble , will remove CommutativeComonad and CommutativeCoflatmap for now

Update: Removed

peterneyens · 2017-07-19T15:44:44Z

tests/src/test/scala/cats/tests/CokleisliTests.scala

+
+    checkAll("Cokleisli[Id, Int, Int]", CommutativeArrowTests[Cokleisli[Id, ?, ?]].commutativeArrow[Int, Int, Int, Int, Int, Int])
+    checkAll("CommutativeArrow[Cokleisli[Id, ?, ?]]", SerializableTests.serializable(CommutativeArrow[Cokleisli[Id, ?, ?]]))
+  }


I think we need to add a test for Cokleisli[F, ?, ?] as well (maybe the existing one using NonEmptyList)?

Nitpicking, can we put these CommutativeArrow tests not between the MonoidK and SemigroupK tests?

diesalbla · 2017-07-19T17:47:03Z

Regarding CommutativeCoflatmap and CommutativeComonad, the only reason I introduced these classes in the first case was to preserve the similarity between the Arrow (and relatives) instances for Kleisli and Cokleisli. I could not come up with too many instances for them.

kailuowang · 2017-07-19T19:16:27Z

merging with 2 sign-offs

kailuowang changed the title ~~Continuing #1719~~ Replace Split with Commutative Arrow. Continuing #1719 Jul 13, 2017

This was referenced Jul 13, 2017

Prepare for the 1.0.0-MF release #1702

Merged

Cats-1567 Replace Split with Commutative Arrow. Introduces Commutative Monad. #1719

Closed

kailuowang force-pushed the diesalbla-DA_1567_Commutative_Arrow branch from 5cfef8c to 23425dc Compare July 14, 2017 02:15

kailuowang requested review from peterneyens and edmundnoble July 14, 2017 02:18

edmundnoble suggested changes Jul 14, 2017

View reviewed changes

diesalbla and others added 10 commits July 17, 2017 10:32

Cats-1567 Add Commutative Arrows

a8806ee

We add a type-class of commutative arrows, which are those in which the `split` operation is commutative (can pair in any order).

Cats-1567 Introduce Commutative Monad

495ad23

We introduce a Commutative Monad Type-class, a subclass of Monad in which the flatMap of independent operations is commutative.

Cats-1567 Kleisli Instances

21fc3fa

We introduce some instances of CommutativeArrow for Kleisli, which are based on CommutativeMonad.

Cats-1567 Split CommutativeMonad into Commutative FlatMap

ea5a064

We introduce a new type class, CommutativeFlatMap, to load the commutativity law one level up.

Cats-1567 Commutative Comonad - CoflatMap

64965c2

We introduce the duals of Commutative Comonads and CoflatMap, as the duals of commutative Flatmaps and Monads. This is done to generate and test CommutativeArrow instances for the Cokleisli datatype.

1567 Complete tests for Cokleisli

8fe26df

We complete the tests for the CommutativeArrow instance of Cokleisli. To use it, we mark the Monad instance of `NonEmptyList` as a Commutative Monad.

NonEmptyList comonad is not commutative

9603c82

fix unmoored statement

bc0f309

added CommutativeMonad instances for Kleisli and WriterT

c5ab4a5

kailuowang force-pushed the diesalbla-DA_1567_Commutative_Arrow branch from ebf2e0e to c5ab4a5 Compare July 17, 2017 14:37

kailuowang added 2 commits July 17, 2017 11:39

fix import error

b77427b

Merge branch 'master' into diesalbla-DA_1567_Commutative_Arrow

914c564

peterneyens reviewed Jul 18, 2017

View reviewed changes

made function1 commutativeArrow

3e975bd

removed CommutativeComonad and CommutativeCoflatmap

41edd56

peterneyens reviewed Jul 19, 2017

View reviewed changes

added back arrow tests

f34c6e5

peterneyens approved these changes Jul 19, 2017

View reviewed changes

edmundnoble approved these changes Jul 19, 2017

View reviewed changes

kailuowang merged commit 16ea2ed into typelevel:master Jul 19, 2017

peterneyens mentioned this pull request Jul 19, 2017

SplitLaw doesn't hold in general #1567

Closed

kailuowang deleted the diesalbla-DA_1567_Commutative_Arrow branch July 19, 2017 23:03

kailuowang modified the milestone: 1.0.0-MF Jul 24, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace Split with Commutative Arrow. Continuing #1719 #1766

Replace Split with Commutative Arrow. Continuing #1719 #1766

kailuowang commented Jul 13, 2017

edmundnoble commented Jul 14, 2017

codecov-io commented Jul 14, 2017 •

edited

Loading

edmundnoble left a comment •

edited

Loading

edmundnoble Jul 14, 2017

edmundnoble Jul 14, 2017

edmundnoble Jul 14, 2017

edmundnoble Jul 14, 2017 •

edited

Loading

kailuowang commented Jul 17, 2017

peterneyens commented Jul 18, 2017

peterneyens Jul 18, 2017

peterneyens Jul 18, 2017 •

edited

Loading

kailuowang commented Jul 18, 2017

peterneyens commented Jul 18, 2017

edmundnoble commented Jul 19, 2017

kailuowang commented Jul 19, 2017 •

edited

Loading

peterneyens Jul 19, 2017

kailuowang Jul 19, 2017

diesalbla commented Jul 19, 2017

kailuowang commented Jul 19, 2017

Replace Split with Commutative Arrow. Continuing #1719 #1766

Replace Split with Commutative Arrow. Continuing #1719 #1766

Conversation

kailuowang commented Jul 13, 2017

edmundnoble commented Jul 14, 2017

codecov-io commented Jul 14, 2017 • edited Loading

Codecov Report

edmundnoble left a comment • edited Loading

Choose a reason for hiding this comment

edmundnoble Jul 14, 2017

Choose a reason for hiding this comment

edmundnoble Jul 14, 2017

Choose a reason for hiding this comment

edmundnoble Jul 14, 2017

Choose a reason for hiding this comment

edmundnoble Jul 14, 2017 • edited Loading

Choose a reason for hiding this comment

kailuowang commented Jul 17, 2017

peterneyens commented Jul 18, 2017

peterneyens Jul 18, 2017

Choose a reason for hiding this comment

peterneyens Jul 18, 2017 • edited Loading

Choose a reason for hiding this comment

kailuowang commented Jul 18, 2017

peterneyens commented Jul 18, 2017

edmundnoble commented Jul 19, 2017

kailuowang commented Jul 19, 2017 • edited Loading

peterneyens Jul 19, 2017

Choose a reason for hiding this comment

kailuowang Jul 19, 2017

Choose a reason for hiding this comment

diesalbla commented Jul 19, 2017

kailuowang commented Jul 19, 2017

codecov-io commented Jul 14, 2017 •

edited

Loading

edmundnoble left a comment •

edited

Loading

edmundnoble Jul 14, 2017 •

edited

Loading

peterneyens Jul 18, 2017 •

edited

Loading

kailuowang commented Jul 19, 2017 •

edited

Loading