Finish up work on NonEmptyList #1231
Conversation
- Use toList instead of head::tail - Use tail ::: other.toList, instead of MonadCombine - Define apply method instead of using default paramter for tail - Use Nil instead of monad.empty - Simplify find and filter
- update this branch with latest changes from upstream master
This continues work started by @WarFox in typelevel#1120 (see [this comment](typelevel#1120 (comment))). At this point, I have left `OneAnd` in place. However, I think that after merging this we may want to delete it. In practice it's pretty awkward to use and sometimes prevents performant operations. See also typelevel#1089.
Current coverage is 89.86% (diff: 97.36%)@@ master #1231 diff @@
==========================================
Files 234 235 +1
Lines 3143 3208 +65
Methods 3089 3150 +61
Messages 0 0
Branches 51 55 +4
==========================================
+ Hits 2817 2883 +66
+ Misses 326 325 -1
Partials 0 0
|
|
It looks like I have some unit tests that I should add before this gets merged. I can probably add them tonight or tomorrow (Eastern time). I welcome code reviews before then. |
| * A data type which represents a non empty list of A, with | ||
| * single element (head) and optional structure (tail). | ||
| */ | ||
| final case class NonEmptyList[A](head: A, tail: List[A]) { |
There was a problem hiding this comment.
what if we did something similar to NonEmptyVector and made this a value class around ::[A]? Then we would not need to allocate in some cases.
And also unit test improvements
|
Thanks for the great review, @johnynek! I've addressed many of your points. So far I haven't changed the representation from a case class with a |
| @@ -113,6 +115,11 @@ import simulacrum.typeclass | |||
| def sequence1_[G[_], A](fga: F[G[A]])(implicit G: Apply[G]): G[Unit] = | |||
| G.map(reduceLeft(fga)((x, y) => G.map2(x, y)((_, b) => b)))(_ => ()) | |||
|
|
|||
| def toNonEmptyList[A](fa: F[A]): NonEmptyList[A] = | |||
| reduceRightTo(fa)(a => NonEmptyList(a, Nil)) { (a, lnel) => | |||
| lnel.map { case NonEmptyList(h, t) => NonEmptyList(a, h :: t) } | |||
There was a problem hiding this comment.
should we add a :: nel to make a new NonEmptyList?
def ::(a: A): NonEmptyList[A] = NonEmptyList(a, head :: tail)
|
👍 |
|
This looks great! Thanks! 👍 |
|
👍 from me! Looking forward to this. |
|
Nice! LGTM 👍 |
This continues work started by @WarFox in #1120 (see this comment).
At this point, I have left
OneAndin place. However, I think thatafter merging this we may want to delete it. In practice it's pretty
awkward to use and sometimes prevents performant operations. See also #1089.