cats icon indicating copy to clipboard operation
cats copied to clipboard

Add `NonEmptyList#prependAll`

Open saeltz opened this issue 3 years ago • 4 comments

NonEmptyList was missing a prependAll/++:. This adds the method and the alias and tests. The implementation may not be most efficient.

I also added an override for concat to append an Option to a NonEmptyList. I would have used IterableOnce as in the standard library for Scala 2.13 with Seq. But I didn't know how to do that cross-version with Scala 2.12 and TraversableOnce. Maybe I could use Foldable? Any help appreciated.

saeltz avatar Jul 06 '22 19:07 saeltz

A couple of thoughts from my side.

First of all, just to note: NonEmptyList also lacks appendAll method. So I think that it would be nice to add them both simultaneously – appendAll and prependAll. The same applies to other NE-collections: NonEmptyVector, NonEmptyLazyList, NonEmptySeq.

The second thing I'd like to call out is naming. On one hand, Scala library kinda reserves prependAll for mutable collections whereas uses prependedAll/appendedAll for immutables. Not to mention that these methods in Scala usually take IterableOnce as a parameter. On the other hand, in Cats itself there's NonEmptyChain which has appendChain and prependChain for appending/prepending a whole Chain correspondingly.

Therefore, IMO to keep Cats consistent with itself, it'd be more correct to name these methods appendList/prependList (if we didn't like to mess with IterableOnce).

satorg avatar Jul 07 '22 06:07 satorg

Thanks for your comments!

NonEmptyList also lacks appendAll method. So I think that it would be nice to add them both simultaneously – appendAll and prependAll.

Added that.

NonEmptyVector, NonEmptyLazyList, NonEmptySeq

Added appendVector to NonEmtpyVector. NonEmtpyLazyList already has prependLazyList and appendLazyList. Added appendSeq to NonEmptySeq.

to keep Cats consistent with itself, it'd be more correct to name these methods appendList/prependList

Renamed that.

Please review again. Looking forward to your feedback!

saeltz avatar Jul 11 '22 11:07 saeltz

@saeltz looks good to me, thanks for your efforts! If you believe your PR is pretty much ready, would you mind pulling it out of the "Draft" state please?

satorg avatar Jul 11 '22 19:07 satorg

Thanks for addressing all the feedback! For anyone interested, here's a thread about the origin of the method naming that Sergey pointed out we should be consistent with: https://github.com/typelevel/cats/pull/1838#issuecomment-334344911

After reading that I'm a bit lost in the weeds but I promise I'll circle back soon :)

armanbilge avatar Jul 13 '22 19:07 armanbilge