Adding NonEmptyLazyList

[nathaniel-may]

Addresses issue 2903

Took code from @kailuowang PR 2905

First time contributor

[nathaniel-may]

Not completed. Before I spin too many cycles on it, I want to get some feedback on the approach and make sure I'm on the right track.

[kailuowang]

Thanks @nathaniel-may. I am so sorry, but I think I made a mistake in my issue that I didn't make it clear on which example to follow to create this new Data type. There are four types of NonEmptyXXX data types in Cats.

1. NonEmptyList a dedicated wrapper for a List tail and head.
2. NonEmptyStream developed using OneAnd abstraction
3. NonEmptyVector a value class wrapper for Vector
4. NonEmptySet, NonEmptyMap, NonEmptyChain, new type implementation of Set, Map, and Chain.

The 4th type is the most advanced one and I was planning for NonEmptyLazyList to be implemented in. It's a cool technique. It introduces a new behavior to a class with zero runtime cost. Would you still have the energy to do that? If not I totally understand. It's my fault.

P.S. , if you would like to continue, we can move all the NonEmptyStream related tests from OneAndSuite into the NonEmptyStream suite in the 2.12- folder.

PPS I am actually thinking maybe we should also take opportunity to move NonEmptyVector over to the new type way on Scala 2.13 as well, but that should be a separate PR.

[nathaniel-may]

No worries, that's why I wanted to check in with a PR 🙃. Later today I'll take a stab at modeling it like NonEmptyChain (which you did mention in the linked issue).

[nathaniel-may]

What's the timeline for this ticket?

[kailuowang]

I think we still have some time, but this is one of the more urgent issues.

[nathaniel-may]

Is this closer to what you were looking for?

[kailuowang]

I am going to loop in @LukaJCB since I am not sure I follow the implementation of instances in NonEmptyChain

Namely I think since this is just a new type, i.e. at runtime it's just exactly the same type as LazyList, we should take advantage of the existing instances of LazyList by simply coerce them into instance of NonEmptyList

We could just do

import cats.instances.lazyList._
sealed abstract private[data] class NonEmptyLazyListInstances  extends NonEmptyLazyListInstances1  {
   
   implicit val catsDataBimonadForNonEmptyLazyList: Bimonad[NonEmptyLazyList] = Bimonad[LazyList].asInstanceOf[Bimonad[NonEmptyLazyList]]

} 

sealed abstract private[data] class NonEmptyLazyListInstances1  extends NonEmptyLazyListInstances2  {

   implicit val catsDataSemigroupKForNonEmptyLazyList: SemigroupK[NonEmptyLazyList] = SemigroupK[LazyList].asInstanceOf[SemigroupK[NonEmptyLazyList]]
 
   implicit def catsDataPartialOrderForNonEmptyLazyList[A: PartialOrder]: PartialOrder[NonEmptyLazyList[A]] =
    PartialOrder[LazyList[A]].asInstanceOf[PartialOrder[NonEmptyLazyList]]
}

sealed abstract private[data] class NonEmptyLazyListInstances2 {
   implicit val catsDataNonEmptyTraverseForNonEmptyLazyList:  NonEmptyTraverse[LazyList] = new NonEmptyTraverse[LazyList] {
     val  traverseInstance = Traverse[LazyList].asInstanceOf[Traverse[NonEmptyLazyList]]
     //delegate all traverse methods to traverseInstance

     def nonEmptyTraverse[G[_]: Apply, A, B](fa: NonEmptyLazyList[A])(f: A => G[B]): G[NonEmptyLazyList[B]] =
        Foldable[LazyList].reduceRightToOption[A, G[LazyList[B]]](fa.tail)(a => Apply[G].map(f(a))(Lazylist.one)) { (a, lglb) =>
          Apply[G].map2Eval(f(a), lglb)(_ +: _)
        }.map {
           case None        => Apply[G].map(f(fa.head))(NonEmptyLazyList.apply)
           case Some(gtail) => Apply[G].map2(f(fa.head), gtail)((h, t) => create(NonEmptyLazyList(h) ++ t))
        } .value
}

  implicit def catsDataEqForNonEmptyLazyList[A: Eq]: Eq[NonEmptyLazyList[A]] = Eq[LazyList[A]].asInstanceOf[PartialOrder[NonEmptyLazyList]]
}

Right?

Update, I realized that Bimonad cannot be simply a recast of lazylist instance since it's not a comonad. It has to be a new trait like NonEmptyTraverse

[kailuowang]

Note that I distribute the instances into different traits so that they don't conflict with each other

[kailuowang]

also note that by "delegate all traverse methods to traverseInstance" I meant all Traverse methods that are implemented or overriden in the Traverse instance for LazyList. They are mostly https://github.com/typelevel/cats/blob/master/core/src/main/scala-2.13+/cats/instances/lazyList.scala#L44-L171, except tailRecM

[nathaniel-may]

Ok I didn't quite realize what this pattern was doing but this makes a lot of sense and it's really cool.

[nathaniel-may]

Looks like the only instances that aren't already included are BiMonad SemigroupK and PartialOrder. Although I'm not getting it to recognize Traverse Foldable even though they're defined.

[nathaniel-may]

Commit 5cee9d3 basically just slaps your suggested code on.

[kailuowang]

Thanks @nathaniel-may we are definitely in the right direction. I left some comment for the instances, I think we might be able to do it differently from existing NonEmptyChain

[nathaniel-may]

I might not be able to work on this for the next couple days, but I'll be watching the ticket.

[kailuowang]

forgot to mention that we should just move these NonEmptyStream tests into 2.12-/cats/tests/NonEmptyStreamSuite.scala instead of changing them.

[kailuowang]

@nathaniel-may I created a PR against your branch to implement the ideas nathaniel-may#1 I haven't run any tests there but it compiles.

[kailuowang]

@nathaniel-may after some thinking triggered by #2928, I think I am going to take this opportunity to fix all NonEmptyXXX data types for 2.13. The downside is that this is will no longer be good first time issue. I will continue the work on your PR.

[nathaniel-may]

Makes sense to me. Thanks for all the guidance! Let me know what I can do to make this PR more workable for you.

I'll definitely keep an eye on other good first issues in the future.

[kailuowang]

Replaced by #2941