Deprecate FlatMap's >> and << #1955

LukaJCB · 2017-10-07T15:51:25Z

To do so, I moved *> and <* from Cartesian syntax to Cartesian type class. I also moved followedBy and forEffect to Apply. :)

Should fix #1954

codecov-io · 2017-10-07T16:29:13Z

Codecov Report

Merging #1955 into master will decrease coverage by 0.04%.
The diff coverage is 55.55%.

@@            Coverage Diff             @@
##           master    #1955      +/-   ##
==========================================
- Coverage   96.14%   96.09%   -0.05%     
==========================================
  Files         273      273              
  Lines        4535     4536       +1     
  Branches      117      132      +15     
==========================================
- Hits         4360     4359       -1     
- Misses        175      177       +2

Impacted Files	Coverage Δ
laws/src/main/scala/cats/laws/FlatMapLaws.scala	`100% <ø> (ø)`	⬆️
core/src/main/scala/cats/syntax/cartesian.scala	`50% <ø> (ø)`	⬆️
core/src/main/scala/cats/FlatMap.scala	`100% <ø> (+7.69%)`	⬆️
core/src/main/scala/cats/syntax/flatMap.scala	`66.66% <0%> (-33.34%)`	⬇️
laws/src/main/scala/cats/laws/ApplyLaws.scala	`100% <100%> (ø)`	⬆️
core/src/main/scala/cats/Apply.scala	`84.61% <75%> (-4.28%)`	⬇️

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 29faf62...95ac1ab. Read the comment docs.

johnynek · 2017-10-07T17:00:30Z

core/src/main/scala/cats/Apply.scala

@@ -19,6 +19,14 @@ trait Apply[F[_]] extends Functor[F] with Cartesian[F] with ApplyArityFunctions[
  override def product[A, B](fa: F[A], fb: F[B]): F[(A, B)] =
    ap(map(fa)(a => (b: B) => (a, b)))(fb)

+  /** Sequentially compose two actions, discarding any value produced by the first. */
+  def followedBy[A, B](fa: F[A])(fb: F[B]): F[B] =
+    map(product(fa, fb)) { case (_, b) => b }


Shouldn’t we use map2 here since that can sometimes be more efficient than product + map

Yes that's a great idea

also I think "sequentially" is confusing now. It is sequential if we have a monad, if we have an applicative it is not necessarily "sequential"

johnynek · 2017-10-07T17:02:32Z

core/src/main/scala/cats/Cartesian.scala

@@ -14,6 +14,12 @@ import simulacrum.typeclass
 */
 @typeclass trait Cartesian[F[_]] {
  def product[A, B](fa: F[A], fb: F[B]): F[(A, B)]
+
+  @inline final def *>[A, B](fa: F[A])(fb: F[B])(implicit F: Functor[F]): F[B] =
+    F.map(product(fa, fb)) { case (_, b) => b }


I don’t like this. If you have a Functor for Cartesian you have an Apply, no?

I’m -1 in accepting other type classes on F in methods for a typeclass of F in most cases.

Also, I don’t see why these aren’t exactly symbols for the methods you moved to Apply.

I'll add these as aliases for those in apply :)

johnynek · 2017-10-07T17:04:07Z

core/src/main/scala/cats/syntax/cartesian.scala

@@ -17,10 +17,4 @@ abstract class CartesianOps[F[_], A] extends Cartesian.Ops[F, A] {
  final def |@|[B](fb: F[B]): CartesianBuilder[F]#CartesianBuilder2[A, B] =
    new CartesianBuilder[F] |@| self |@| fb

-  final def *>[B](fb: F[B])(implicit F: Functor[F]): F[B] =


Ahh. I see someone put them here originally. This was an error in my view. They should be on Apply.

Yeah +1 on that :)

johnynek · 2017-10-07T17:04:38Z

docs/src/main/tut/faq.md

@@ -215,8 +215,6 @@ All other symbols can be imported with `import cats.implicits._`
 | `x === y`                        | equals                 |                  | `Eq[A]`                 | `eqv(x: A, y: A): Boolean`                                |
 | `x =!= y`                        | not equals             |                  | `Eq[A]`                 | `neqv(x: A, y: A): Boolean`                               |
 | `fa >>= f`                       | flatMap                |                  | `FlatMap[F[_]]`         | `flatMap(fa: F[A])(f: A => F[B]): F[B]`                   |
-| `fa >> fb`                       | followed by            |                  | `FlatMap[F[_]]`         | `followedBy(fa: F[A])(fb: F[B]): F[B]`                    |
-| `fa << fb`                       | for effect             |                  | `FlatMap[F[_]]`         | `forEffect(fa: F[A])(fb: F[B]): F[A]`                     |


Should we not add them back to another place place in the doc?

I added them at the bottom with a deprecated notice

johnynek · 2017-10-07T17:05:36Z

free/src/test/scala/cats/free/FreeTests.scala

@@ -189,7 +189,7 @@ class FreeTests extends CatsSuite {
    forAll { (x: Int, y: Int) =>
      val expr1: Free[T, Int] = Free.injectRoll[T, Test1Algebra, Int](Test1(x, Free.pure))
      val expr2: Free[T, Int] = Free.injectRoll[T, Test2Algebra, Int](Test2(y, Free.pure))
-      val res = distr[T, Int](expr1 >> expr2)
+      val res = distr[T, Int](expr1 >>= (_ => expr2))


Why won’t *> work here?

There was something odd going on with the tests so that simply replacing >> with *> did not work.

johnynek · 2017-10-08T17:26:04Z

Looks good to me except for coverage. I’m on the phone now but can we exercise each of the lines to make sure there isn’t any issue that we don’t notice?

Also, I wonder if this is really the last example where we have methods on the wrong type classes. Hard to catch except by audit.

LukaJCB · 2017-10-08T20:20:42Z

core/src/main/scala/cats/syntax/flatMap.scala

@@ -11,6 +11,18 @@ trait FlatMapSyntax extends FlatMap.ToFlatMapOps {

  implicit final def catsSyntaxFlatMapIdOps[A](a: A): FlatMapIdOps[A] =
    new FlatMapIdOps[A](a)
+
+  implicit final def catsSyntaxFlatMapOps[F[_]: FlatMap, A](fa: F[A]): FlatMapOps[F, A] =
+    new FlatMapOps[F, A](fa)


These are untested, because of deprecation

LukaJCB · 2017-10-08T21:28:24Z

core/src/main/scala/cats/Apply.scala

+
+  /** Alias for [[forEffect]]. */
+  @inline final def <*[A, B](fa: F[A])(fb: F[B]): F[A] =
+    forEffect(fa)(fb)


This is untested because <* and << were before this PR.

edmundnoble

<3 <3 <3

johnynek · 2017-10-10T08:07:04Z

👍

All changes addressed

Deprecate >> and <<

e44e3d0

stew added the in progress label Oct 7, 2017

kailuowang added the Source Breaking label Oct 7, 2017

johnynek previously requested changes Oct 7, 2017

View reviewed changes

Move *> and <* to Apply and make them aliases

b8a302d

LukaJCB commented Oct 8, 2017

View reviewed changes

Add syntaxTests for *> and <*

8f35654

LukaJCB commented Oct 8, 2017

View reviewed changes

Move laws

95ac1ab

edmundnoble approved these changes Oct 10, 2017

View reviewed changes

LukaJCB merged commit b7f15a1 into typelevel:master Oct 10, 2017

stew removed the in progress label Oct 10, 2017

LukaJCB deleted the remove-redundant-flatmap-operators branch October 10, 2017 08:18

peterneyens mentioned this pull request Oct 11, 2017

Move followedBy/forEffect tests to ApplyTests #1963

Merged

kailuowang added this to the 1.0.0-RC1 milestone Oct 13, 2017

mpilquist mentioned this pull request Oct 27, 2017

>> vs *> typelevel/fs2#961

Closed

kailuowang added the enhancement label Dec 14, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deprecate FlatMap's >> and << #1955

Deprecate FlatMap's >> and << #1955

LukaJCB commented Oct 7, 2017

codecov-io commented Oct 7, 2017 •

edited

Loading

johnynek Oct 7, 2017

LukaJCB Oct 7, 2017

johnynek Oct 7, 2017

johnynek Oct 7, 2017

johnynek Oct 7, 2017

LukaJCB Oct 7, 2017

johnynek Oct 7, 2017

LukaJCB Oct 7, 2017

johnynek Oct 7, 2017

LukaJCB Oct 8, 2017

johnynek Oct 7, 2017

LukaJCB Oct 8, 2017

johnynek commented Oct 8, 2017

LukaJCB Oct 8, 2017

LukaJCB Oct 8, 2017

edmundnoble left a comment

johnynek commented Oct 10, 2017

Deprecate FlatMap's >> and << #1955

Deprecate FlatMap's >> and << #1955

Conversation

LukaJCB commented Oct 7, 2017

codecov-io commented Oct 7, 2017 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnynek commented Oct 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edmundnoble left a comment

Choose a reason for hiding this comment

johnynek commented Oct 10, 2017

codecov-io commented Oct 7, 2017 •

edited

Loading