Sync NEL and NEV methods (fixes #1832) #1838

durban · 2017-08-21T18:28:36Z

There is one notable change: NEL previously had a concat which accepted another NEL. To be similar to NEV, I changed this (and added concatNel).

The following differences are (intentionally) not addressed:

groupBy: NEL has one, but I didn't add it to NEV, because I think that it should have an Eq constraint, however implementing that would need a Map which works with Eq...
length/size: NEV has a length: Int and a size: Long (as an extension method). NEL had a size: Int and I've added length: Int. (Possibly size should be changed to Long, or simply removed, to be an extension method.)
a few Vector-specific methods, e.g., updated.

This fixes #1832 (cc @johnynek).

codecov-io · 2017-08-21T19:04:05Z

Codecov Report

Merging #1838 into master will increase coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master   #1838      +/-   ##
=========================================
+ Coverage   96.09%   96.1%   +0.01%     
=========================================
  Files         273     273              
  Lines        4554    4566      +12     
  Branches      129     122       -7     
=========================================
+ Hits         4376    4388      +12     
  Misses        178     178

Impacted Files	Coverage Δ
core/src/main/scala/cats/data/NonEmptyVector.scala	`100% <100%> (ø)`	⬆️
core/src/main/scala/cats/data/NonEmptyList.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 d7a0204...f18b06f. Read the comment docs.

johnynek

Thanks for sending the PR to fix this!

johnynek · 2017-08-21T19:07:08Z

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

+  /**
+   * Append another NonEmptyList
+   */
+  def concatNel[AA >: A](other: NonEmptyList[AA]): NonEmptyList[AA] =


why can't this just be concat? NonEmptyList and List are different types. I'm not sure why we took this route and if there is not a good reason I'd rather change it before 1.0.

johnynek · 2017-08-21T19:07:44Z

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

@@ -139,12 +153,6 @@ final case class NonEmptyList[+A](head: A, tail: List[A]) {
  }

  /**
-   * Append another NonEmptyList
-   */
-  def concat[AA >: A](other: NonEmptyList[AA]): NonEmptyList[AA] =


I like this naming better. We should add concat to NonEmptyVector in my opinion, not remove here.

durban · 2017-08-21T19:34:59Z

@johnynek I guess the concat/concatNel/concatNev is to avoid overloading. I don't feel very strong about it, I just followed NEV. I could live with it either way. (However, I think there are people who really don't like overloading, e.g., there is a wart in WartRemover, which forbids it. Maybe we should ask them...)

peterneyens · 2017-08-23T08:08:19Z

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

+  def reverse: NonEmptyVector[A] =
+    new NonEmptyVector(toVector.reverse)
+
+  def zipWithIndex: NonEmptyVector[(A, Int)] =


You could use this to override Traverse#zipWithIndex for NonEmptyVector.

durban · 2017-08-23T19:12:12Z

@johnynek Here is the issue and PR which removed concat overloading from NEV. This PR also has some discussion about overloading in general.

johnynek · 2017-09-01T18:27:37Z

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

@@ -237,6 +255,9 @@ private[data] sealed trait NonEmptyVectorInstances {
      override def traverse[G[_], A, B](fa: NonEmptyVector[A])(f: (A) => G[B])(implicit G: Applicative[G]): G[NonEmptyVector[B]] =
        G.map2Eval(f(fa.head), Always(Traverse[Vector].traverse(fa.tail)(f)))(NonEmptyVector(_, _)).value

+      override def zipWithIndex[A](fa: NonEmptyVector[A]): NonEmptyVector[(A, Int)] =
+        fa.zipWithIndex


this is untested, so I guess that means zipWithIndex is currently lawless.

We could write a test to check if zipWithIndex is consistent with zipWith(Range(0, size)) :)

LukaJCB · 2017-09-15T19:06:07Z

Hey @durban, are you still working on this? Or would you like some help? :)

durban · 2017-09-16T09:29:15Z

@LukaJCB I'm not sure what's left to do (if any) ...

LukaJCB · 2017-09-16T09:35:20Z

I agree there's not much left :)
However could you address the review comments? Some of the extra code isn't tested, you can check this in the codecov link above!
Also, you currently have some conflicts, so a rebase would be great :)

durban · 2017-09-16T13:28:41Z

I've rebased and also added more tests for zipWithIndex. Please let me know if I've missed anything else.

LukaJCB · 2017-09-16T14:22:01Z

LGTM (pending concensus on concat) thank you very much! :)

kailuowang · 2017-09-21T15:40:28Z

👍 I am fine with theconcat as in this PR. We should probably not go back and forth on overrides. @johnynek WDYT?

LukaJCB · 2017-09-28T14:36:00Z

I've thought a bit more about this and I think having concat concatenate a NEL and a List is fine for the most part, mainly because most concatenation will probably done using the Semigroup instance and |+| is as short as it gets.
However, I disagree on keeping ++ as to me, it implies symmetry between the two types. I'd expect something with the operator ++ to take two arguements of the same type on either side, so I think we should just get rid of ++. People can use concat instead.

I'd also be fine with having concat be the concatenation between two same types and using concatList and concatVector accordingly.

kailuowang · 2017-10-04T14:29:36Z

@durban do you still have time to work on this? if you want, I am happy to continue it for you.

durban · 2017-10-04T17:19:22Z

I'd argue that ++ only accepting a NEL/NEV would make it less useful. ++ as it is, has a less "restrictive" signature (it's possible to make a List/Vector from a NEL/NEV; the other direction is not possible). (Please also note, that I absolutely did not touch ++ in this PR.)

kailuowang · 2017-10-05T02:55:17Z

Here is my final proposal, it looks that we have to make some trade off or we are going to have to break either NEL or NEV. The trade off I am willing to make here is to allow overload in this particular case.

So in NEL we are going to have

concat(l: List) //new
concat(nel: NEL)
concatNel(nel: NEL) //new
++(l: List)

the concat(nel: NEL) is the overload to allow backward comp.
in NEV we are going to have

concat(v: Vector)
concat(nev: NEV)  //new
concatNev(nev: NEV)
++(l: Vector)

the concat(nev: NEV) is the overload to maintain symmetry.

In PR #1949
we will add to NEL

def :::(other: NEL)

and add to NEV

def ++:(other: NEV)

This might mitigate the @LukaJCB 's concern over an asymmetric symbol concatenation.
WDYT? @johnynek @durban @LukaJCB

jcranky · 2017-10-05T04:34:35Z

For NEV, you mean concat(v: Vector), not concat(l: List), right?

LukaJCB · 2017-10-05T07:03:35Z

I think that's a good idea 👍

Edit: Actually I'm not sure right now, the way you put it, it has the symmetric operators for different types and the ++: asymmetric operator for two same types, shouldn't we switch that around? :)
And on another note, do we really need another operator for List concatenation?
Through semigroup and SemigroupK we already have |+| and <+>, so I don't necessarily think we need a third one. That said, I'd be fine with that to avoid breakage

durban · 2017-10-05T11:29:20Z

@kailuowang I'm ok with your proposal. If others also agree, I can add it to this PR.

kailuowang · 2017-10-05T12:41:32Z

@LukaJCB the idea is to avoid unnecessary breaking changes. ++: and ::: is kind of inherited from Vector and List. We keep ++ the same to maintain backward compat. I can see arguments on both side for ++ to take a NEL or List. So I am not convinced that we need a breaking change to chose one over the other. If we want ++ to take NEL and NEV I incline to add an override in another PR.

LukaJCB · 2017-10-05T13:09:55Z

I'm good with asymmetry when it means as little breaking changes as possible 👍

durban · 2017-10-05T16:47:35Z

@kailuowang This is not gonna work:

[error] def concat[AA >: A](other: Vector[AA]): cats.data.NonEmptyVector[AA] at line 77 and
[error] def concat[AA >: A](other: cats.data.NonEmptyVector[AA]): cats.data.NonEmptyVector[AA] at line 79
[error] have same type after erasure: (other: scala.collection.immutable.Vector)scala.collection.immutable.Vector

(For NEL it would work, because that isn't a value class, but at that point we'd lose the consistency.)

There are various ugly workarounds for this, but I think they're more ugly than only having concatNel/concatNev.

ceedubs · 2017-10-05T16:55:26Z

In previous PRs we've discussed overloading and the general consensus seemed to be that the downsides outweigh the benefits. Personally I'm 👍 for concatNel and concatNev.

kailuowang · 2017-10-05T17:48:28Z

Okay, can we add concat(nel: NEL) back as a deprecated overload.
We will need a scalafix migration. concat(nel) -> concatNel(nel), doesn't need to be this PR though, unless if you are interested in trying out the scalafix stuff.

durban · 2017-10-06T17:11:03Z

@kailuowang Done. I'd skip learning scalafix for now.

kailuowang · 2017-10-10T14:09:37Z

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

@@ -12,6 +12,7 @@ import cats.data.{NonEmptyList, NonEmptyVector}
 import cats.laws.discipline.arbitrary._
 import cats.laws.discipline.{ComonadTests, NonEmptyTraverseTests, MonadTests, ReducibleTests, SemigroupKTests, SerializableTests}

+@deprecated("to be able to test deprecated methods", since = "1.0.0-RC1")


Sorry I hate to drag on this but I wouldn't deprecate the whole suite for one test. I'd rather we duplicated the test suite class with just the concat test in it and deprecate that one.

LukaJCB

Thanks for this! :)

LukaJCB · 2017-10-16T22:28:03Z

Hey @durban, sorry to bother you again, but we just need to fix those conflicts, and then we're good to merge! :) If you're too busy, or don't want to invest more time, that's completely okay!

LukaJCB

LGTM, thanks a lot! :) Will merge when Travis is done

kailuowang · 2017-10-17T19:48:44Z

haha I beat you @LukaJCB

peterneyens added the Binary Breaking label Aug 21, 2017

johnynek reviewed Aug 21, 2017

View reviewed changes

peterneyens reviewed Aug 23, 2017

View reviewed changes

kailuowang added this to the 1.0.0-RC1 milestone Aug 28, 2017

johnynek reviewed Sep 1, 2017

View reviewed changes

kailuowang added the Source Breaking label Sep 14, 2017

jcranky mentioned this pull request Oct 5, 2017

Add ::: to NonEmptyList #1949

Merged

kailuowang reviewed Oct 10, 2017

View reviewed changes

kailuowang previously approved these changes Oct 11, 2017

View reviewed changes

LukaJCB previously approved these changes Oct 14, 2017

View reviewed changes

kailuowang mentioned this pull request Oct 16, 2017

Getting ready for RC1 #1974

Merged

12 tasks

durban added 6 commits October 17, 2017 20:55

Sync NEL and NEV methods (fixes #1832)

f43228c

Override Traverse#zipWithIndex for NonEmptyVector

1bf60ee

More tests for NonEmptyVector#zipWithIndex

d21fa9d

Add deprecated overload of NonEmptyList#concat

9a5d4ff

Try to appease CI

7a41804

Deprecating separately

f18b06f

durban dismissed stale reviews from LukaJCB and kailuowang via f18b06f October 17, 2017 19:06

kailuowang approved these changes Oct 17, 2017

View reviewed changes

LukaJCB approved these changes Oct 17, 2017

View reviewed changes

kailuowang merged commit 8d94237 into typelevel:master Oct 17, 2017

kailuowang added the enhancement label Dec 14, 2017

armanbilge mentioned this pull request Jul 13, 2022

Add NonEmptyList#prependAll #4267

Merged

Sync NEL and NEV methods (fixes #1832) #1838

Sync NEL and NEV methods (fixes #1832) #1838

Conversation

durban commented Aug 21, 2017

codecov-io commented Aug 21, 2017 • edited Loading

Codecov Report

johnynek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

durban commented Aug 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

durban commented Aug 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LukaJCB commented Sep 15, 2017

durban commented Sep 16, 2017

LukaJCB commented Sep 16, 2017

durban commented Sep 16, 2017

LukaJCB commented Sep 16, 2017

kailuowang commented Sep 21, 2017 • edited Loading

LukaJCB commented Sep 28, 2017

kailuowang commented Oct 4, 2017

durban commented Oct 4, 2017

kailuowang commented Oct 5, 2017 • edited Loading

jcranky commented Oct 5, 2017

LukaJCB commented Oct 5, 2017 • edited Loading

durban commented Oct 5, 2017

kailuowang commented Oct 5, 2017

LukaJCB commented Oct 5, 2017

durban commented Oct 5, 2017

ceedubs commented Oct 5, 2017

kailuowang commented Oct 5, 2017

durban commented Oct 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LukaJCB left a comment

Choose a reason for hiding this comment

LukaJCB commented Oct 16, 2017

LukaJCB left a comment

Choose a reason for hiding this comment

kailuowang commented Oct 17, 2017

codecov-io commented Aug 21, 2017 •

edited

Loading

kailuowang commented Sep 21, 2017 •

edited

Loading

kailuowang commented Oct 5, 2017 •

edited

Loading

LukaJCB commented Oct 5, 2017 •

edited

Loading