Deprecate CartesianBuilder, finish up #1487 #1745

kailuowang · 2017-06-27T14:34:03Z

This PR is to finish up #1487. I deprecated the CartesianBuilder, merged the master and one small correction in the build file.

kailuowang · 2017-06-27T14:35:28Z

build.sbt

@@ -67,7 +67,7 @@ lazy val commonJsSettings = Seq(
  scalaJSStage in Global := FastOptStage,
  parallelExecution := false,
  requiresDOM := false,
-  jsEnv := NodeJSEnv().value,
+  jsEnv := new org.scalajs.jsenv.nodejs.NodeJSEnv(),


old one is deprecated.

kailuowang · 2017-06-27T14:35:47Z

.jvmopts

 -XX:+CMSClassUnloadingEnabled
-# must be enabled for CMSClassUnloadingEnabled to work


@DavidGregory084 pointed out that these comments causes problem on Windows.

peterneyens · 2017-06-27T15:38:55Z

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

@@ -175,7 +175,7 @@ final case class EitherT[F[_], A, B](value: F[Either[A, B]]) {
   * scala> val v1: Validated[NonEmptyList[Error], Int] = Validated.invalidNel("error 1")
   * scala> val v2: Validated[NonEmptyList[Error], Int] = Validated.invalidNel("error 2")
   * scala> val eithert: EitherT[Option, Error, Int] = EitherT.leftT[Option, Int]("error 3")
-   * scala> eithert.withValidated { v3 => (v1 |@| v2 |@| v3.toValidatedNel).map { case (i, j, k) => i + j + k } }
+   * scala> eithert.withValidated { v3 => (v1, v2, v3.leftMap(NonEmptyList.of(_))).mapN { case (i, j, k) => i + j + k } }


I think we can keep the toValidatedNel (instead of .leftMap(NonEmptyList.of(_)))

missed that during the merge. will fix.

peterneyens · 2017-06-27T15:39:13Z

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

@@ -160,7 +160,7 @@ private[data] sealed trait Tuple2KTraverse[F[_], G[_]] extends Traverse[λ[α =>
  def G: Traverse[G]

  override def traverse[H[_]: Applicative, A, B](fa: Tuple2K[F, G, A])(f: A => H[B]): H[Tuple2K[F, G, B]] =
-    (F.traverse(fa.first)(f) |@| G.traverse(fa.second)(f)).map(Tuple2K(_, _))
+    (F.traverse(fa.first)(f), G.traverse(fa.second)(f)).mapN(Tuple2K(_, _))


Maybe we can just write H.map2 instead of the syntax version?

peterneyens

Is there a reason we call it "cartesian tuple syntax", but put it under the cats.syntax.apply._ import?

kailuowang · 2017-06-27T18:00:43Z

@johnynek please allow me to reply you here since any further change is probably going to be through this PR. I think @non first suggested the possibility of deprecating the |@| syntax once the tuple syntax is proven here. There wasn't any objections ATM. The cartesian syntax on tuple has been released since 0.7.0 for almost a year, I think it's about time to deprecate the |@| syntax. Unlike Xor, this change should be rather straightforward and mechanical (I just did it in cats code base, took me 10 minutes). If we still want to keep it, for how long?

codecov-io · 2017-06-27T18:19:29Z

Codecov Report

Merging #1745 into master will decrease coverage by 0.02%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master   #1745      +/-   ##
=========================================
- Coverage   94.02%     94%   -0.03%     
=========================================
  Files         256     256              
  Lines        4201    4201              
  Branches       84      89       +5     
=========================================
- Hits         3950    3949       -1     
- Misses        251     252       +1

Impacted Files	Coverage Δ
core/src/main/scala/cats/syntax/apply.scala	`100% <ø> (ø)`	⬆️
core/src/main/scala/cats/syntax/cartesian.scala	`50% <ø> (-25%)`	⬇️
core/src/main/scala/cats/data/EitherT.scala	`96.52% <ø> (ø)`	⬆️
core/src/main/scala/cats/data/OneAnd.scala	`98.36% <100%> (ø)`	⬆️
core/src/main/scala/cats/data/Tuple2K.scala	`100% <100%> (ø)`	⬆️

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 605e9b5...9656199. Read the comment docs.

johnynek · 2017-06-27T19:18:05Z

Okay. Thanks for linking to the relevant discussion.

kailuowang · 2017-06-29T16:34:29Z

@peterneyens I think I addressed all your comments, have time to take another look?

kailuowang · 2017-07-02T22:43:17Z

merge with 2 sign-offs given this has been discussed in depth in the previous related PRs.

fosskers · 2017-07-07T15:52:41Z

If |@| is being deprecated, is the tuple thing the new recommended syntax for Applicative-style?

kailuowang · 2017-07-07T15:56:53Z

@fosskers yes. I gave an example in the migration doc. Let me know if that works for you. https://github.com/kailuowang/cats/blob/8df835fe4700455135140c2e3d02b6555328985c/CHANGES.md

DavidGregory084 and others added 2 commits May 20, 2017 16:36

Apply syntax for tuples, fixes typelevel#1363

eefd70b

merge master/ deprecate caetesian builder

1a481f7

kailuowang mentioned this pull request Jun 27, 2017

Apply syntax for tuples #1487

Closed

kailuowang commented Jun 27, 2017

View reviewed changes

kailuowang requested review from djspiewak and peterneyens June 27, 2017 14:36

kailuowang added the Source Breaking label Jun 27, 2017

peterneyens reviewed Jun 27, 2017

View reviewed changes

fix feedback and build error

9656199

kailuowang added this to the 1.0.0-MF milestone Jun 29, 2017

peterneyens approved these changes Jun 29, 2017

View reviewed changes

kailuowang mentioned this pull request Jun 30, 2017

Prepare for the 1.0.0-MF release #1702

Merged

26 tasks

fommil mentioned this pull request Jun 30, 2017

cartesian builder should support >22 args via shapeless #1753

Closed

djspiewak approved these changes Jul 1, 2017

View reviewed changes

kailuowang merged commit 99b543b into typelevel:master Jul 2, 2017

kailuowang deleted the DavidGregory084-tuple-unapply branch July 2, 2017 22:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deprecate CartesianBuilder, finish up #1487 #1745

Deprecate CartesianBuilder, finish up #1487 #1745

kailuowang commented Jun 27, 2017 •

edited

Loading

kailuowang Jun 27, 2017

kailuowang Jun 27, 2017 •

edited

Loading

peterneyens Jun 27, 2017

kailuowang Jun 27, 2017

peterneyens Jun 27, 2017

peterneyens left a comment

kailuowang commented Jun 27, 2017

codecov-io commented Jun 27, 2017 •

edited

Loading

johnynek commented Jun 27, 2017

kailuowang commented Jun 29, 2017

kailuowang commented Jul 2, 2017 •

edited

Loading

fosskers commented Jul 7, 2017

kailuowang commented Jul 7, 2017 •

edited

Loading

		-XX:+CMSClassUnloadingEnabled
		# must be enabled for CMSClassUnloadingEnabled to work

Deprecate CartesianBuilder, finish up #1487 #1745

Deprecate CartesianBuilder, finish up #1487 #1745

Conversation

kailuowang commented Jun 27, 2017 • edited Loading

kailuowang Jun 27, 2017

Choose a reason for hiding this comment

kailuowang Jun 27, 2017 • edited Loading

Choose a reason for hiding this comment

peterneyens Jun 27, 2017

Choose a reason for hiding this comment

kailuowang Jun 27, 2017

Choose a reason for hiding this comment

peterneyens Jun 27, 2017

Choose a reason for hiding this comment

peterneyens left a comment

Choose a reason for hiding this comment

kailuowang commented Jun 27, 2017

codecov-io commented Jun 27, 2017 • edited Loading

Codecov Report

johnynek commented Jun 27, 2017

kailuowang commented Jun 29, 2017

kailuowang commented Jul 2, 2017 • edited Loading

fosskers commented Jul 7, 2017

kailuowang commented Jul 7, 2017 • edited Loading

kailuowang commented Jun 27, 2017 •

edited

Loading

kailuowang Jun 27, 2017 •

edited

Loading

codecov-io commented Jun 27, 2017 •

edited

Loading

kailuowang commented Jul 2, 2017 •

edited

Loading

kailuowang commented Jul 7, 2017 •

edited

Loading