NonEmptyList work in progress [do-not-merge-yet] #1120
Conversation
|
Thanks @WarFox -- this looks like a great start! |
| /** | ||
| * Return the head and tail into a single list | ||
| */ | ||
| def unwrap: List[A] = head :: tail |
There was a problem hiding this comment.
what do you think of us simply calling this toList?
There was a problem hiding this comment.
Thanks for the comments @ceedubs.. I'm learning!
I think it makes sense to call it toList. I was just trying to follow OneAnd.scala, thought it was a convention
There was a problem hiding this comment.
I think in OneAnd it's called unwrap since F is abstract so there's not a straightforward name to call it. Since we are fixed to List here, it's easier to come up with a good name :)
There was a problem hiding this comment.
makes sense, I've pushed that change
| * Append another NonEmptyList | ||
| */ | ||
| def combine(other: NonEmptyList[A]): NonEmptyList[A] = | ||
| NonEmptyList(head, MonadCombine[List].combineK(tail, other.head :: other.tail)) |
There was a problem hiding this comment.
I think this would be simpler as:
NonEmptyList(head, tail ::: other.toList)
|
Great job, thanks! This is looking really good. Feel free to remove the commented-out type aliases. |
| fa map f | ||
|
|
||
| def pure[A](x: A): NonEmptyList[F, A] = | ||
| NonEmptyList(x, monad.empty) |
There was a problem hiding this comment.
Let's just use Nil instead of monad.empty here, since we know we are talking about List.
There was a problem hiding this comment.
thanks for the detailed review :)
this is motivational
I shall do this changes tonight
| */ | ||
| def filter(p: A => Boolean): List[A] = { | ||
| val rest = tail.filter(p) | ||
| if (p(head)) head :: rest else rest |
There was a problem hiding this comment.
Would be tempted to go with (head :: tail).filter(p) which I think is toList.filter(p)
There was a problem hiding this comment.
I actually prefer being explicit and not creating a list when not necessary. I'll leave it to @WarFox to decide which way to go.
|
Thanks for finishing the issue from hack the tower and putting it up for a pull request! 👍 |
| new Monad[NonEmptyList[?]] { | ||
| override def map[A, B](fa: NonEmptyList[F, A])(f: A => B): NonEmptyList[F, B] = | ||
| fa map f | ||
|
|
There was a problem hiding this comment.
NonEmptyList should have only one type parameter, so this should be NonEmptyList[B] instead of NonEmptyList[F, B]. This should cause compilation errors.
- 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
| checkAll("Reducible[NonEmptyList[List, ?]]", SerializableTests.serializable(Reducible[NonEmptyList[List, ?]])) | ||
|
|
||
| implicit val iso = CartesianTests.Isomorphisms.invariant[NonEmptyList[ListWrapper, ?]](NonEmptyList.catsDataFunctorForNonEmptyList(ListWrapper.functor)) | ||
|
|
There was a problem hiding this comment.
You don't need 2 iso's if the second one resolves.
|
Thanks guys. I've been busy this week with work and other things. Please allow me time to update this pull requests with these review comments, and please add more comments. I can't promise a time frame now. |
- update this branch with latest changes from upstream master
|
Thanks for all of your work on this, @WarFox. I understand that you are busy. I'm hoping to get the |
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.
|
@ceedubs Hi Cody, I am sorry about this. I couldn't work on this due to various reasons. It is great of you to pickup the work. Appreciate it, thank you. |
This is a work in progress branch for #1087 created during Hack the tower meet up in London on Saturday, 11 June 2016.