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 contains, collectFirst and mkString to NonEmptyList #2113

Closed
rsoeldner opened this issue Dec 16, 2017 · 13 comments
Closed

Add contains, collectFirst and mkString to NonEmptyList #2113

rsoeldner opened this issue Dec 16, 2017 · 13 comments

Comments

@rsoeldner
Copy link
Contributor

I would like to add these functionality to NonEmptyList, what do you think ?

 def contains[AA >: A](a: AA): Boolean = exists(_ == a)

  def collectFirst[B](pf: PartialFunction[A, B]): Option[B] =
    pf.lift(head).orElse(tail.collectFirst(pf))

  def mkString(s: String): String = mkString("", s, "")

  def mkString(begin: String, s: String, end: String): String = {
    val builder = new StringBuilder()
    builder.append(begin)
    builder.append(head)
    tail.foreach(elem => builder.append(s"$s$elem"))
    builder.append(end)
    builder.toString()
  }

I'm quite unsure about my mkString 😄

@johnynek
Copy link
Contributor

Contains would need an Eq and mkString should take a Show.

@rossabaker
Copy link
Member

collectFirst is already on Foldable. mkString isn't, but some use cases are handled by intercalate.

I've needed contains and mkString on NonEmptyList before. My instinct is to provide these for all Foldable, but our Eq- and Show-based versions would have a name clash with the standard library's foldables.

@kailuowang
Copy link
Contributor

I would prefer to add contains and mkstring to Foldable too, but we already passed the deadline for non bug bin compat breaking PRs...

@rossabaker
Copy link
Member

Yeah, I failed to think about that. 😞

To speak to the usefulness of the request, I have a syntax extension with the same three methods, and feel like a jerk for not creating a PR in time.

@tpolecat
Copy link
Member

mkString can be generalized to any monoid in the same way intercalate is. I call it foldSmash in doobie and would be pleased to see it in cats with a better name.

@johnynek
Copy link
Contributor

@tpolecat actually Semigroups can be generalized in this way: if a+b is monoid, then a+z+b is a semigroup for all z. That’s one way to think about this: you are transforming the Monoid and then doing a combine.

@rsoeldner
Copy link
Contributor Author

rsoeldner commented Dec 18, 2017

I ended up with

   def mkString[A](fa: F[A], prefix: String, delim: String, suffix: String)(implicit F: Functor[F], A: Show[A], M: Monoid[String]): String =
    M.combine(prefix, M.combine(intercalate(F.map(fa)(A.show), delim), suffix))

  def mkString[A](fa: F[A], delim: String)(implicit ev1: Functor[F], ev2: Show[A], M: Monoid[String]): String =
    mkString(fa, M.empty, delim, M.empty)

And

  def contains[A](fa: F[A])(v: A)(implicit ev: Eq[A]): Boolean =
    exists(fa)(ev.eqv(_, v))

But it feels a bit strange. I couldn't find a use-case to abstract out the String. What do you think?

@LukaJCB
Copy link
Member

LukaJCB commented Dec 19, 2017

Hey @rsoeldner, first of all thanks a lot for bringing this up!
I think it would look something like this if I understood Rob correctly:

def foldSmash[A: Monoid](fa: F[A], prefix: A, delim: A, suffix: A): A =
  prefix |+| intercalate(fa, delim) |+| suffix

Not sure if these would be binary compatible, if so, we could easily bring them in for the next release :)

@ceedubs
Copy link
Contributor

ceedubs commented Dec 19, 2017

I'm guessing that there aren't many instances that would actually override these values. So putting some of these methods in the Foldable companion object might provide 90% of the benefit without binary incompatibilities.

@rsoeldner
Copy link
Contributor Author

@LukaJCB Thank you for the help.
But with this implementation we are limited to the same type of A (prefix/suffix/delim and F[A]).
I mean for example, if you have a List[Int] and Strings, with Show we are able to get every showable smashed together.

@chuwy
Copy link

chuwy commented Dec 20, 2017

For this use-case we can always coerce to necessary type:

val ints = List(1,2,3,4)
ints.map(_.show).foldSmash("prefix", "-", "suffix")
ints.map(YourMonoid.fromInt).foldSmash(prefix, delim, suffix)

I think this is right balance between too generic and special implementations.

@ceedubs
Copy link
Contributor

ceedubs commented Mar 24, 2018

@rsoeldner should this be closed out now that #2123 is merged?

@rsoeldner
Copy link
Contributor Author

@ceedubs yes 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

8 participants