Implement NonEmptyList#mkShowString methods

[xavier-fernandez]

No description provided.

[codecov-io]

Current coverage is 92.42% (diff: 100%)

Merging #1518 into master will increase coverage by 0.01%

@@             master      #1518   diff @@
==========================================
  Files           246        246          
  Lines          3743       3751     +8   
  Methods        3622       3627     +5   
  Messages          0          0          
  Branches        121        124     +3   
==========================================
+ Hits           3459       3467     +8   
  Misses          284        284          
  Partials          0          0          

Powered by Codecov. Last update fd15e07...6f769e7

[kailuowang]

We already have a Show instance, which is an overlap with the mkString().
Do we need the two other overloads in NonEmptyList? I am not sure. If we could design from scratch I would probably leave those in a type class. I am leaning slightly towards to not adding them here inside the NonEmptyList but I am biased mainly due to my distaste against the scala standard collection library design.

[johnynek]

Yeah I'm -1 on mkString without using Show. Not the intent of cats. I would be +1 on a mkShowString or something that uses Show instead of toString on the intermediate parts. Maybe only the three argument version is needed.

[non]

I'd support something like this, FWIW (name is just to distinguish from proposed mkString impl):

def intercalate(sep: String)(implicit ev: A =:= String): String =
  ev(head) + sep + tail.mkString(sep)

I agree that we shouldn't implement this method using .toString, but it wouldn't be a bad idea to support cases where you already have strings.

[travisbrown]

I'm -1 on either version of this—both just seem much too specific to deserve inclusion when the syntax / runtime cost of .toList.mkString is going to be pretty small.

I think this is another specific case of the "is this library focused on abstractions or just convenient stuff" question.

[xavier-fernandez]

Following the feedback from @kailuowang , @johnynek and @non I updated the PR adding an implementation using the Show typeclass, instead of using the method toString.

[xavier-fernandez]

Rebased so this changes are compatible with: #1424

[johnynek]

Given the number of allocations mkString already requires, I think toList.mkString would be my vote and not add this method.

…

On Sat, Jan 7, 2017 at 01:42 Xavier Fernández Salas < ***@***.***> wrote: Rebased so this changes are compatible with: #1424 <#1424> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1518 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAEJdr-TmLGLhGXzBqDDfV55iM_V1u8fks5rP3ojgaJpZM4LcHCR> .

[xavier-fernandez]

@johnynek The one without input parameters?

[johnynek]

actually, I am concerned about many ways to do something here.

As of #1520 if we wanted a mkString on NonEmptyList we could do:

import cats.data.NonEmptyList
import cats.implicits._

val ne = NonEmptyList.of(1, 2, 3)
ne.map(_.toString).intercalate(", ")
//
or
ne.map(_.show).intercalate(", ")
// or
ne.toList.mkString(", ")

I'm not sure we really need to add another way to do this.