Implement NonEmptyList#Collect

[xavier-fernandez]

• "collect" method is really useful when wanting to do a transformation in some of the elements from a collection, applying a partial function. This PR implements this method both for NonEmptyVector and NonEmptyList.

[codecov-io]

Codecov Report

Merging #1516 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1516      +/-   ##
==========================================
+ Coverage   92.33%   92.33%   +<.01%     
==========================================
  Files         247      247              
  Lines        3900     3903       +3     
  Branches      137      139       +2     
==========================================
+ Hits         3601     3604       +3     
  Misses        299      299

Impacted Files	Coverage Δ
core/src/main/scala/cats/data/NonEmptyList.scala	98.85% <100%> (+0.04%)	⬆️

---

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 f6ef91e...d0f6f0b. Read the comment docs.

[johnynek]

I'm weakly -1 on adding methods which are identical to doing .toList.foo or toVector.foo.

Methods that preserve nonemptiness are more interesting to me.

[xavier-fernandez]

What about returning: Option[NonEmptyList[B]]?

[kailuowang]

@xavier-fernandez, unless I am missing something, a Option[NonEmptyList[B]] is as powerful as a List[B] but less convenient. So you do want to return a List[B]. And if you going to lose the nonemptiness anyway why not x.toList.collect? To me it makes the lost of nonemptiness more explicit. Thus I favor that than x.collect that returns a List.
The filter method we have there is an existing example of such methods that don't preserve nonemptines. @ceedubs do you happen to remember the incentive of adding that filter method?

[xavier-fernandez]

I should close the PR?

[ceedubs]

The main arguments that I can think of for having the filter method is that for NonEmptyList it can be implemented every so slightly more efficiently than toList.filter and that it may be slightly more convenient for users. I do recall at least one time that a coworker has asked me how they should filter a scalaz NonEmptyList, as it isn't necessarily obvious that you should go through List. I'm pretty neutral as to whether or not we include the List-like methods that don't offer anything specific to being non-empty.

[xavier-fernandez]

NonEmptyList.collect will have a better performance than toList.collect...

On the other hand,NonEmptyVector.collect will not yield any performance improvement.

[johnynek]

Okay. On this argument I'm +1 on NonEmptyList.collect but not NonEmptyVector.

…

On Thu, Jan 5, 2017 at 23:58 Xavier Fernández Salas < ***@***.***> wrote: NonEmptyList.collect will have a better performance than toList.collect... On the other hand,NonEmptyVector.collect will not yield any performance improvement. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#1516 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAEJdhyLQCQk9tS8ORePNXVRx52xQIFvks5rPhBKgaJpZM4Lbksu> .

[xavier-fernandez]

If NonEmptyList is having a collect method, it would not be better that NonEmptyVector too, for consistency?

[johnynek]

I don't think so. Otherwise we'll have some other class justifying why we should have yet another method. Frankly, if consistency is the main driver I suggest we go back to only adding methods if they preserve non-emptiness.

…

On Sat, Jan 7, 2017 at 00:48 Xavier Fernández Salas < ***@***.***> wrote: If NonEmptyList is having a collect method, it would not be better that NonEmptyVector too, for consistency? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#1516 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAEJdvMOcd92tju5KLvjpL_hc-crW2fRks5rP21igaJpZM4Lbksu> .

[xavier-fernandez]

Modified the PR so the collect method is only implemented on NonEmptyList

[peterneyens]

Should we say "Builds a new List ..." ?

[peterneyens]

I think this should be :

/**
 * ...
 * scala> nel.collect {
 *      |   case v if v % 2 == 0 => "even"
 *      |   case _ => "odd"
 *      | }
 * ...
 */

for the doctest to be generated correctly.

[xavier-fernandez]

@peterneyens amend done :)

[johnynek]

👍

[peterneyens]

You need to use exactly

     | 

after a scala> for the test to be picked up.

Right now the generated doctest is

    property("example at line 203: nel.collect { case v if v < 3 => v }") = org.scalacheck.Prop.secure {
      sbtDoctestTypeEquals(nel.collect { case v if v < 3 => v })((nel.collect { case v if v < 3 => v }): scala.collection.immutable.List[Int])
      val actual = sbtDoctestReplString(nel.collect { case v if v < 3 => v })
      val expected = "List(1, 2)"
      (actual == expected) :| s"'$actual' is not equal to '$expected'"
    }

So the last "odd" and "even" test is not checked. If you change it to

   /**
    * scala> nel.collect {
    *      |   case v if v % 2 == 0 => "even"
    *      |   case _ => "odd"
    *      | }
    */

It will include this one as well :

    property("example at line 205: nel.collect { ...") = org.scalacheck.Prop.secure {
      sbtDoctestTypeEquals(nel.collect {
  case v if v % 2 == 0 => "even"
  case _ => "odd"
})((nel.collect {
  case v if v % 2 == 0 => "even"
  case _ => "odd"
}): scala.collection.immutable.List[Int])
      val actual = sbtDoctestReplString(nel.collect {
  case v if v % 2 == 0 => "even"
  case _ => "odd"
})
      val expected = "List(odd, even, odd, even, odd)"
      (actual == expected) :| s"'$actual' is not equal to '$expected'"
    }

It would be handy if sbt-doctest gave some kind of error message if it is ignoring lines.

[xavier-fernandez]

@peterneyens will fix tomorrow morning, thank you :)

[xavier-fernandez]

Did an ammend fixing the DocTest generation :)

[peterneyens]

Thanks for the doctest changes.

[kailuowang]

👍 merging with 2 sign-offs.