Add `contains_`, `foldSmash` and `mkString_` to `FoldableOps` #2123

rsoeldner · 2017-12-20T06:47:33Z

#2113, Am I right, test should be added to the FoldableSuite ?

chuwy · 2017-12-20T07:07:27Z

core/src/main/scala/cats/Foldable.scala

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


Hey @rsoeldner,

Actually, for mkString I believe we can also generalize over String, so instead foldSmash would work with String as A. Something like:

def mkString[A: Monoid, B: Show](fa: F[A])(prefix: B, delim: B, suffix: B)(implicit F: Functor[F], S: Show[A]): String = foldSmash(F.map(fa)(S.show))(prefix.show, delim.show, suffix.show)

Maybe this is bit a bike-shedding, but probably can help if you want to have unusual Show[B] instance in scope - can imagine this helpful with debugging.

Right, I thought about this, but wasn't able to find a use-case. Do you have an Idea ?

I guess the main question here is "why I might want to pass different Bs into mkString". I think it has use-case for pretty printers to output some structure differently based on environment:

structure.mkString(LeftPad(0), LeftPad(5), LeftPad(0))

Maybe this is a completely made-up example, but why to constrain it?

I see your point, should we support different types for prefix, suffix and delim ?

Don't know, for me it looks like a skew into "over-generalized" approach, but let's see what community says - maybe even suggestion with Show[B] is unnecessary.

we shouldn't need Functor[F]. You can do this

def mkString[A:Show, B: Show](fa: F[A])(prefix: B, delim: B, suffix: B): String = foldLeft(fa, prefix.show) { (left, a) => left + (delim.show) + a.show } + (suffix.show)

of course, that is a O(N^2) implementation, but it could be fixed using a StringBuilder:

def mkString[A:Show, B: Show](fa: F[A])(prefix: B, delim: B, suffix: B): String = { val bldr = new java.lang.StringBuilder bldr.append(prefix.show) val b2 = foldLeft(fa, bldr) { (b, a) => b.append((delim.show) + a.show) } b2.append(suffix.show) b2.toString }

that said, I think B is overgeneralized. Users should just do prefix.show etc... and keep it to be String.

After some time, I think you are right. 👍

kailuowang · 2018-01-09T18:42:29Z

Thanks @rsoeldner will you be interested in taking this approach to avoid breaking binary compatibility?

rsoeldner · 2018-01-09T20:24:47Z

@kailuowang Yes, I'll try come up with a PR tomorrow ;)

rsoeldner · 2018-01-11T06:11:58Z

@kailuowang Is it possible to reformat the entire project ? I used IJ's format functionality and now everything looks different 😭

kailuowang · 2018-01-25T12:16:16Z

@rsoeldner sorry I wasn't sure if that's a question. Certainly we want to avoid any unnecessary reformat. This pr would be a great addition. Fyi now #2148 provides a good example on how to add functions without breaking bin compat.

kailuowang · 2018-03-07T21:29:24Z

@rsoeldner now I think about it, maybe we should just move the implementations into FoldableOps.
Since we can't let user override them in instances (due to binary compat, we can't add these methods to the trait), we probably shouldn't make them appear to be available from the type class itself anyway.
Let me know if you are still interested/available in continue on this, I'll be more than happy to continue it for you.

codecov-io · 2018-03-08T15:28:13Z

Codecov Report

Merging #2123 into master will increase coverage by 0.12%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #2123      +/-   ##
==========================================
+ Coverage   94.66%   94.79%   +0.12%     
==========================================
  Files         328      332       +4     
  Lines        5533     5702     +169     
  Branches      199      214      +15     
==========================================
+ Hits         5238     5405     +167     
- Misses        295      297       +2

Impacted Files	Coverage Δ
core/src/main/scala/cats/syntax/foldable.scala	`100% <100%> (ø)`	⬆️
core/src/main/scala/cats/data/WriterT.scala	`91.37% <0%> (-1.76%)`	⬇️
core/src/main/scala/cats/data/Ior.scala	`98.48% <0%> (-1.52%)`	⬇️
.../src/main/scala/cats/syntax/applicativeError.scala	`100% <0%> (ø)`	⬆️
core/src/main/scala/cats/syntax/option.scala	`100% <0%> (ø)`	⬆️
core/src/main/scala/cats/data/package.scala	`87.5% <0%> (ø)`	⬆️
core/src/main/scala/cats/syntax/TrySyntax.scala	`100% <0%> (ø)`
...c/main/scala/cats/free/ContravariantCoyoneda.scala	`100% <0%> (ø)`
core/src/main/scala/cats/data/AndThen.scala	`96.96% <0%> (ø)`
core/src/main/scala/cats/data/NonEmptySet.scala	`97.29% <0%> (ø)`
... and 7 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5051b36...3907443. Read the comment docs.

johnynek

Elsewhere in cats, namely Traverse, we use the _ to indicate we want the contained type to be Unit. Can we instead name these differently. Maybe mkShowString and containsEq? Just an idea?

johnynek · 2018-03-08T16:51:56Z

core/src/main/scala/cats/syntax/foldable.scala

+   * res0: String = List(1,2,3)
+   * }}}
+   */
+  def mkString_[A: Monoid](prefix: String, delim: String, suffix: String)(implicit F: Functor[F], S: Show[A], ev1: Monoid[String], ev2: Foldable[F]): String =


Can we not take Functor on this? If we implemented directly from foldLeft we could. Shame to take an additional constraint just to make implementation easier.

kailuowang · 2018-03-09T03:25:36Z

Elsewhere in cats, namely Traverse, we use the _ to indicate we want the contained type to be Unit

I am aware of that as well. However I also noticed that there are already filter_, takeWhile_, dropWhile_ in Foldable , so at least in Foldable there is already this mini convention of using _ to avoid conflict. I don't mind renaming them, however mkShowString, containsEq sounds a bit awkward. How about makeString or mkShow and has ? The downside is that they are harder to discover.

kailuowang · 2018-03-14T14:57:52Z

@johnynek WDYT about my comment above?

rsoeldner · 2018-03-14T15:10:04Z

@kailuowang had a small break 👍 thanks for pushing this forward

johnynek

these changes look good to me with one comment.

johnynek · 2018-03-14T17:22:39Z

core/src/main/scala/cats/syntax/foldable.scala

+      builder append A.show(a) append delim
+    }
+    if (b.isEmpty)
+      ""


don't we need the prefix and suffix here? Can we have a test that exercises this against scala's mkString in the same case?

oops. yeah, I meant to change that but slipped. will update now.

johnynek · 2018-03-14T17:57:58Z

tests/src/test/scala/cats/tests/FoldableSuite.scala

+
+  test(s"Foldable[$name] mkString_") {
+    forAll { (fa: F[Int]) =>
+      fa.mkString_("L[", ";", "]") should === fa.mkString("L[", ";", "]")


now is the right hand side working? don't we need a toList or something?

good catch. Since this is not my repo I can only edit files through github and my mental compiler is clearly not good enough.

johnynek

Thanks! looks good.

ceedubs

👍 LGTM thanks @rsoeldner!

rsoeldner changed the title ~~#2113 Add contains, foldSmash and mkString~~ Add contains, foldSmash and mkString to Foldable Dec 20, 2017

Add contains, foldSmash and mkString

8376f71

chuwy reviewed Dec 20, 2017

View reviewed changes

kailuowang added this to the 1.1 milestone Jan 31, 2018

reset change to Foldable

e5ec50f

kailuowang added 2 commits March 8, 2018 10:28

moved implementations to FoldableOps

6d7ded9

added doc tests

4aa6096

johnynek reviewed Mar 8, 2018

View reviewed changes

kailuowang changed the title ~~Add contains, foldSmash and mkString to Foldable~~ Add contains_, foldSmash and mkString_ to FoldableOps Mar 9, 2018

updated

c74ac49

johnynek reviewed Mar 14, 2018

View reviewed changes

kailuowang added 2 commits March 14, 2018 13:27

use prefix and suffix when F is empty

22d2d40

added a test to mkString

d60aeca

johnynek reviewed Mar 14, 2018

View reviewed changes

fix build

25bf299

johnynek previously approved these changes Mar 14, 2018

View reviewed changes

fixing build

3907443

kailuowang dismissed johnynek’s stale review via 3907443 March 14, 2018 20:23

kailuowang requested review from LukaJCB and ceedubs March 15, 2018 15:32

johnynek approved these changes Mar 16, 2018

View reviewed changes

ceedubs approved these changes Mar 16, 2018

View reviewed changes

kailuowang merged commit bf1cba2 into typelevel:master Mar 16, 2018

kailuowang added the enhancement label Mar 16, 2018

ceedubs mentioned this pull request Mar 24, 2018

Add contains, collectFirst and mkString to NonEmptyList #2113

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `contains_`, `foldSmash` and `mkString_` to `FoldableOps` #2123

Add `contains_`, `foldSmash` and `mkString_` to `FoldableOps` #2123

rsoeldner commented Dec 20, 2017 •

edited

Loading

chuwy Dec 20, 2017 •

edited

Loading

rsoeldner Dec 20, 2017

chuwy Dec 20, 2017

rsoeldner Dec 20, 2017

chuwy Dec 20, 2017

johnynek Jan 10, 2018 •

edited

Loading

johnynek Jan 10, 2018

rsoeldner Jan 11, 2018

kailuowang commented Jan 9, 2018

rsoeldner commented Jan 9, 2018

rsoeldner commented Jan 11, 2018 •

edited

Loading

kailuowang commented Jan 25, 2018 •

edited

Loading

kailuowang commented Mar 7, 2018

codecov-io commented Mar 8, 2018 •

edited

Loading

johnynek left a comment

johnynek Mar 8, 2018

kailuowang commented Mar 9, 2018

kailuowang commented Mar 14, 2018

rsoeldner commented Mar 14, 2018

johnynek left a comment

johnynek Mar 14, 2018

kailuowang Mar 14, 2018

johnynek Mar 14, 2018

kailuowang Mar 14, 2018

johnynek left a comment

ceedubs left a comment

Add contains_, foldSmash and mkString_ to FoldableOps #2123

Add contains_, foldSmash and mkString_ to FoldableOps #2123

Conversation

rsoeldner commented Dec 20, 2017 • edited Loading

chuwy Dec 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnynek Jan 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kailuowang commented Jan 9, 2018

rsoeldner commented Jan 9, 2018

rsoeldner commented Jan 11, 2018 • edited Loading

kailuowang commented Jan 25, 2018 • edited Loading

kailuowang commented Mar 7, 2018

codecov-io commented Mar 8, 2018 • edited Loading

Codecov Report

johnynek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kailuowang commented Mar 9, 2018

kailuowang commented Mar 14, 2018

rsoeldner commented Mar 14, 2018

johnynek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnynek left a comment

Choose a reason for hiding this comment

ceedubs left a comment

Choose a reason for hiding this comment

Add `contains_`, `foldSmash` and `mkString_` to `FoldableOps` #2123

Add `contains_`, `foldSmash` and `mkString_` to `FoldableOps` #2123

rsoeldner commented Dec 20, 2017 •

edited

Loading

chuwy Dec 20, 2017 •

edited

Loading

johnynek Jan 10, 2018 •

edited

Loading

rsoeldner commented Jan 11, 2018 •

edited

Loading

kailuowang commented Jan 25, 2018 •

edited

Loading

codecov-io commented Mar 8, 2018 •

edited

Loading