Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add NonEmptyList#prependAll #4267

Merged
merged 16 commits into from
Aug 24, 2022
Merged

Add NonEmptyList#prependAll #4267

merged 16 commits into from
Aug 24, 2022

Conversation

saeltz
Copy link
Contributor

@saeltz saeltz commented Jul 6, 2022

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.

@armanbilge armanbilge added this to the 2.9.0 milestone Jul 6, 2022
@armanbilge armanbilge changed the title Add NonEmptyList.prependAll Add NonEmptyList#prependAll Jul 6, 2022
@satorg
Copy link
Contributor

satorg commented Jul 7, 2022

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).

@saeltz
Copy link
Contributor Author

saeltz commented Jul 11, 2022

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!

Copy link

@Baro1905 Baro1905 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@satorg
Copy link
Contributor

satorg commented Jul 11, 2022

@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?

@saeltz saeltz marked this pull request as ready for review July 11, 2022 19:54
@armanbilge
Copy link
Member

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: #1838 (comment)

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

Copy link
Member

@armanbilge armanbilge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, just a couple thoughts about tests :)

@saeltz saeltz requested a review from armanbilge July 19, 2022 07:22
Copy link
Member

@armanbilge armanbilge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for all your work on this, I know the scope expanded quite a bit from your original PR. Very much appreciated!

@johnynek johnynek merged commit 3c98865 into typelevel:main Aug 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants