Change forEffect/followedBy to productL/productR #2083

Jacoby6000 · 2017-12-08T21:24:00Z

This should resolve #1983.

I picked apL and apR based on discussion in that issue.

The deprecation caused scalac to complain about the code generated by simulacrum, so I moved the deprecated method aliases in to syntax.apply.

… <*>.

emilypi

LGTM

Jacoby6000 · 2017-12-08T22:09:31Z

Just realized I missed a bunch of other methods. working on that.

kailuowang · 2017-12-08T22:26:58Z

Thanks! @Jacoby6000, a scalafix migration would be handy. Let me know if you are interested in giving it a try or I can add it in a follow-up PR. Here is a good example for such rename migrations.

… the new names.

kailuowang · 2017-12-08T22:33:23Z

also, we won't have RC2, so all the deprecation note should be 1.0.0

Jacoby6000 · 2017-12-08T23:54:50Z

@kailuowang I built the scalafix stuff, but it seems to cause some problems. forEffect and followedBy were introduced during 1.0.0, and I'm not sure how to split up the fix migrations. I've pushed my scalafix work in to #2083-scalafix.

kailuowang · 2017-12-09T01:51:45Z

The problem is that the Scalafix test depends on 0.9 right? I would say for this PR we simply don't test this migration. It should be low risk

ceedubs · 2017-12-09T17:08:34Z

👍 once there is resolution on the scalafix rules.

I don't think that any word-based name is going to be perfect, but apL and apR seem less likely to cause confusion with certain effect types.

Jacoby6000 · 2017-12-10T01:09:05Z

I've added in the scalafix stuff that I drafted. I can't test it, so please look it over 😅

kailuowang · 2017-12-10T03:40:18Z

for the scalafix stuff, if you include the input and output they will be compiled, which fails due to mis-dependency as we discussed earlier. Sorry I wasn't clear, by "don't test" I mean we shouldn't include them in the input and output of the scalafix tests. Also bin compact check failed, need to add mima exceptions ( you can check CI log to see which ones are needed, or you can leave that to us).

Qi77Qi · 2017-12-13T17:07:10Z

Think faq page needs to be updated as well?

kailuowang · 2017-12-13T21:13:35Z

close and reopen to trigger build

johnynek · 2017-12-14T15:57:56Z

I’m -1 on apL and apR the more I think about it. Note we already have ap. And these new methods are not that similar. I would say productR and productL are much better.

kailuowang · 2017-12-14T16:15:24Z

I am fine with productR and productL, but if people can't agree on it soon, I would propose we give up on bikeshedding this one and just leave as is (closing this PR).

Jacoby6000 · 2017-12-14T17:57:32Z

I'm not much of a fan of productL and productR, mainly because I don't really like product, either. To me, product intuitively means tuple.. So, that method makes me think a -> b -> (a, b), not f a -> f b -> f (a, b). Additionally product does not really convey the application of effects and the standard lib also has a product method which interferes.

As for the methods not being related, I think their relationship becomes more clear if you define map2 more traditionally, and then apL/apR in terms of that.

As an aside, product should probably be rewritten in terms of map2, not the other way around as it is now.

def apL[F[_]: Apply, A, B](fa: F[A])(fb: F[B]): F[A] =
  map2(fa, fb)((a, _) => a)

def apR[F[_]: Apply, A, B](fa: F[A])(fb: F[B]): F[B] =
  map2(fa, fb)((_, b) => b)

def map2[F[_], A, B, C](fa: F[A], fb: F[B])(f: (A, B) => C)(implicit F: Apply[F]): F[C] = 
  F.ap(F.map(fa)(a => (b: B) => f(a, b)))(fb)

Jacoby6000 · 2017-12-14T19:33:34Z

I'm willing to submit and change it to productL and productR to appease the community; I only ever use the proper notation anyway.

Jacoby6000 · 2017-12-14T19:43:07Z

Going deeper in to the relationship, and why I think apL and apR are better than productL and productR:

<*> is ap.
*> is apR which applies both effects, as ap does, but only retains the value on the right.
<* is apL which applies both effects, as ap does, but only retains the value on the left.

I think having names which are as analagous as the symbols does a great deal towards teaching the notation. If people never use <*> or *> or <*, but instead opt for ap, apR, and apL, they will be better off whenever they move to a system using more traditional notation (like, if they learn haskell for instance). Once they realize that <*> is ap, the understanding of *> and <* will come more easily, and vice versa.

johnynek · 2017-12-14T20:10:37Z

I agree with some of what you say, I feel like you are overlooking that ap is dealing with a A => B at some point: F[A => B] <*> F[A]: F[B]. The name is related to apply which is about (A => B) apply A: B. So, keeping ap about apply from A => B when there is no function in sight, to me is a bad name.

I hear you that you don't like product, but that is the name we have for F[A], F[B] => F[(A, B)]. To me, the types there are much closer to what we have at hand.

If you don't like them, I'm pretty happy to keep the current names since I think as long as names don't mislead (and I feel apR and apL do), I don't think they matter too much, so forEffect is just a pointer to the docs which often has a good mnemonic. To me, that's fine.

Jacoby6000 · 2017-12-14T20:55:31Z

I'm going to update this PR to use productL and productR, specifically because cats already has product (which I have said before). Ultimately my agreement is of little consequence. Besides that, just about anything is better than what is there currently.

kailuowang · 2017-12-14T21:08:09Z

@Jacoby6000 thanks very much! Hate to ask you one more thing, we ended up deciding that we are going to have a quick 1.0.0-RC2 release, so if it's easy for you, would you update the deprecate versions? I'd be happy to change those for you if you don't have the time.

Jacoby6000 · 2017-12-14T21:10:58Z

I'll have time in a few hours, if it can wait.

kailuowang · 2017-12-15T14:34:19Z

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

@@ -62,7 +63,7 @@ trait Apply[F[_]] extends Functor[F] with Semigroupal[F] with ApplyArityFunction
   * map2 can be seen as a binary version of [[cats.Functor]]#map.
   */
  def map2[A, B, Z](fa: F[A], fb: F[B])(f: (A, B) => Z): F[Z] =
-    map(product(fa, fb)) { case (a, b) => f(a, b) }
+    ap(map(fa)(a => (b: B) => f(a, b)))(fb)


This is a sensible improvement if we only consider the default implementation, but many instances overrides product for better performance. So we might want to keep the original implementation of product and map2

actually the law is broken with Const instance which overrides product

ConstSuite:
[info]
[info] - Apply[Const[NonEmptyList[String], Int]].apply.map2/product-map consistency *** FAILED ***
[info] GeneratorDrivenPropertyCheckFailedException was thrown during property evaluation.
[info] (Discipline.scala:14)
[info] Falsified after 3 successful property evaluations.
[info] Location: (Discipline.scala:14)
[info] Occurred when passed generated values (
[info] arg0 = Const(NonEmptyList(ꙫ, 鰓)),
[info] arg1 = Const(NonEmptyList(, )),
[info] arg2 =
[info] )
[info] Label of failing property:
[info] Expected: Const(NonEmptyList(, , ꙫ, 鰓))
[info] Received: Const(NonEmptyList(ꙫ, 鰓, , ))

Ah Yeah, that was a bit short sighted of me. I didn't consider overrides.

kailuowang · 2017-12-15T18:07:10Z

Thanks very much! @Jacoby6000

LukaJCB · 2017-12-15T20:36:59Z

Yes, thanks a lot, Jacob! :)

peterneyens · 2017-12-18T12:04:53Z

scalafix/rules/src/main/scala/fix/Cats_v1_0_0.scala

+      "_root_.cats.NonEmptyParallel.parFollowedBy." -> "parApR",
+      "_root_.cats.FlatMap.forEffectEval." -> "apLEval",
+      "_root_.cats.FlatMap.followedByEval." -> "apREval",
+    )


These rewrite rules still need to be updated to productR/productL, right?

Ah yep, they sure do. I missed those.

fix submitted #2117

Change forEffect/followedBy to apL/apR. Depricated old methods, added…

e54119d

… <*>.

emilypi approved these changes Dec 8, 2017

View reviewed changes

Fix compatibility problem I introduced when I moved *> and <*

40bc6d3

kailuowang added this to the 1.0.0 milestone Dec 8, 2017

kailuowang added the Source Breaking label Dec 8, 2017

updated parFollwedBy/parForEffect/followedByEval/forEffectEval to use…

3ae37e8

… the new names.

Jacoby6000 added 3 commits December 8, 2017 18:00

Create scalafix rules for PR 2083

265ffca

Minor fixes and adjustments to remain consistent with the existing api

091fd3f

Merge branch '#1983' into #2083-scalafix

6848b70

kailuowang mentioned this pull request Dec 9, 2017

Add doctest examples for Apply #2086

Merged

Un-comment the parApL/R tests.

171538b

Jacoby6000 and others added 3 commits December 10, 2017 14:01

Remove scalafix tests for apL/apR

5f6a0bb

added Mima exceptions

5943875

Merge branch 'master' into #1983

afe5632

kailuowang requested a review from johnynek December 13, 2017 15:28

kailuowang closed this Dec 13, 2017

kailuowang reopened this Dec 13, 2017

kailuowang added the in progress label Dec 13, 2017

Jacoby6000 closed this Dec 13, 2017

Jacoby6000 reopened this Dec 13, 2017

LukaJCB closed this Dec 14, 2017

LukaJCB reopened this Dec 14, 2017

stew added the in progress label Dec 14, 2017

LukaJCB previously approved these changes Dec 14, 2017

View reviewed changes

LukaJCB removed the in progress label Dec 14, 2017

Update deprecations, method name

eb7edd9

Jacoby6000 dismissed LukaJCB’s stale review via eb7edd9 December 15, 2017 03:18

johnynek previously approved these changes Dec 15, 2017

View reviewed changes

Jacoby6000 changed the title ~~Change forEffect/followedBy to apL/apR~~ Change forEffect/followedBy to productL/productR Dec 15, 2017

Update doc with product(L/R)

6dca0cc

Jacoby6000 dismissed johnynek’s stale review via 6dca0cc December 15, 2017 04:30

Jacoby6000 added 2 commits December 14, 2017 22:32

Update mima for product(L/R)

1ec6706

Remove accidental whitespace

278b36e

kailuowang reviewed Dec 15, 2017

View reviewed changes

Undo changes I made to map2/product

0153f3d

kailuowang approved these changes Dec 15, 2017

View reviewed changes

LukaJCB approved these changes Dec 15, 2017

View reviewed changes

LukaJCB merged commit 8120890 into typelevel:master Dec 15, 2017

peterneyens reviewed Dec 18, 2017

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change forEffect/followedBy to productL/productR #2083

Change forEffect/followedBy to productL/productR #2083

Jacoby6000 commented Dec 8, 2017 •

edited

Loading

emilypi left a comment

Jacoby6000 commented Dec 8, 2017

kailuowang commented Dec 8, 2017

kailuowang commented Dec 8, 2017

Jacoby6000 commented Dec 8, 2017 •

edited

Loading

kailuowang commented Dec 9, 2017

ceedubs commented Dec 9, 2017

Jacoby6000 commented Dec 10, 2017

kailuowang commented Dec 10, 2017 •

edited

Loading

Qi77Qi commented Dec 13, 2017

kailuowang commented Dec 13, 2017

johnynek commented Dec 14, 2017

kailuowang commented Dec 14, 2017 •

edited

Loading

Jacoby6000 commented Dec 14, 2017 •

edited

Loading

Jacoby6000 commented Dec 14, 2017 •

edited

Loading

Jacoby6000 commented Dec 14, 2017 •

edited

Loading

johnynek commented Dec 14, 2017

Jacoby6000 commented Dec 14, 2017

kailuowang commented Dec 14, 2017 •

edited

Loading

Jacoby6000 commented Dec 14, 2017

kailuowang Dec 15, 2017

kailuowang Dec 15, 2017

Jacoby6000 Dec 15, 2017

kailuowang commented Dec 15, 2017

LukaJCB commented Dec 15, 2017

peterneyens Dec 18, 2017

Jacoby6000 Dec 18, 2017

kailuowang Dec 18, 2017

Change forEffect/followedBy to productL/productR #2083

Change forEffect/followedBy to productL/productR #2083

Conversation

Jacoby6000 commented Dec 8, 2017 • edited Loading

emilypi left a comment

Choose a reason for hiding this comment

Jacoby6000 commented Dec 8, 2017

kailuowang commented Dec 8, 2017

kailuowang commented Dec 8, 2017

Jacoby6000 commented Dec 8, 2017 • edited Loading

kailuowang commented Dec 9, 2017

ceedubs commented Dec 9, 2017

Jacoby6000 commented Dec 10, 2017

kailuowang commented Dec 10, 2017 • edited Loading

Qi77Qi commented Dec 13, 2017

kailuowang commented Dec 13, 2017

johnynek commented Dec 14, 2017

kailuowang commented Dec 14, 2017 • edited Loading

Jacoby6000 commented Dec 14, 2017 • edited Loading

Jacoby6000 commented Dec 14, 2017 • edited Loading

Jacoby6000 commented Dec 14, 2017 • edited Loading

johnynek commented Dec 14, 2017

Jacoby6000 commented Dec 14, 2017

kailuowang commented Dec 14, 2017 • edited Loading

Jacoby6000 commented Dec 14, 2017

kailuowang Dec 15, 2017

Choose a reason for hiding this comment

kailuowang Dec 15, 2017

Choose a reason for hiding this comment

Jacoby6000 Dec 15, 2017

Choose a reason for hiding this comment

kailuowang commented Dec 15, 2017

LukaJCB commented Dec 15, 2017

peterneyens Dec 18, 2017

Choose a reason for hiding this comment

Jacoby6000 Dec 18, 2017

Choose a reason for hiding this comment

kailuowang Dec 18, 2017

Choose a reason for hiding this comment

Jacoby6000 commented Dec 8, 2017 •

edited

Loading

Jacoby6000 commented Dec 8, 2017 •

edited

Loading

kailuowang commented Dec 10, 2017 •

edited

Loading

kailuowang commented Dec 14, 2017 •

edited

Loading

Jacoby6000 commented Dec 14, 2017 •

edited

Loading

Jacoby6000 commented Dec 14, 2017 •

edited

Loading

Jacoby6000 commented Dec 14, 2017 •

edited

Loading

kailuowang commented Dec 14, 2017 •

edited

Loading