Add ::: to NonEmptyList #1949

jcranky · 2017-10-04T22:25:26Z

No description provided.

codecov-io · 2017-10-04T23:03:29Z

Codecov Report

Merging #1949 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1949      +/-   ##
==========================================
+ Coverage   96.21%   96.21%   +<.01%     
==========================================
  Files         272      272              
  Lines        4627     4629       +2     
  Branches      124      119       -5     
==========================================
+ Hits         4452     4454       +2     
  Misses        175      175

Impacted Files	Coverage Δ
core/src/main/scala/cats/data/NonEmptyVector.scala	`100% <100%> (ø)`	⬆️
core/src/main/scala/cats/data/NonEmptyList.scala	`100% <100%> (ø)`	⬆️
core/src/main/scala/cats/instances/map.scala	`94.28% <0%> (-0.16%)`	⬇️
core/src/main/scala/cats/Foldable.scala	`100% <0%> (ø)`	⬆️
core/src/main/scala/cats/data/Kleisli.scala	`100% <0%> (ø)`	⬆️

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 8d7b394...d8b501d. Read the comment docs.

kailuowang · 2017-10-05T02:28:16Z

if you want, you can add a doctest to make the test coverage happy.

kailuowang · 2017-10-05T02:45:41Z

core/src/main/scala/cats/data/NonEmptyList.scala

@@ -82,6 +82,9 @@ final case class NonEmptyList[+A](head: A, tail: List[A]) {
  def ::[AA >: A](a: AA): NonEmptyList[AA] =
    NonEmptyList(a, head :: tail)

+  def :::[AA >: A](other: NonEmptyList[AA]): NonEmptyList[AA] =


Actually to maintain symmetry, can we add ++: to NonEmptyVector as well?

jcranky · 2017-10-05T04:20:34Z

Regarding doctest, where should I look at? I new to the cats codebase.

LukaJCB · 2017-10-05T07:11:03Z

Doc test is a test in the Scaladoc right above the method itself :) our build pipeline we automatically check if the result is the same.

An example can be found in NonEmptyVector#filter :)

LukaJCB · 2017-10-05T07:24:20Z

core/src/main/scala/cats/data/NonEmptyVector.scala

@@ -66,6 +66,11 @@ final class NonEmptyVector[+A] private (val toVector: Vector[A]) extends AnyVal
  def ++[AA >: A](other: Vector[AA]): NonEmptyVector[AA] = concat(other)

  /**
+    * Append this NEV to another NEV, producing a new `NonEmptyVector`.
+    */
+  def ++:[AA >: A](other: NonEmptyVector[AA]): NonEmptyVector[AA] = other.concatNev(this)


Shouldn't these be the other way around? ++ for two NEVs and ++: for an NEV and a Vector?

I'm not sure, I went with this #1838 (comment) - perhaps we should have both for both, since overloading is happening already anyway?

This is to mimic the API in Vector and List. Let's address #1838 first

jcranky · 2017-10-05T08:16:41Z

@LukaJCB thanks for the explanation on doc test, I'll have a look

jcranky · 2017-10-05T10:04:56Z

Tests added. This doctest thing is really cool, can you point me to somewhere where I can understand better how it works? Thanks!

LukaJCB · 2017-10-05T10:18:53Z

Check out sbt-doctest :)

jcranky · 2017-10-05T11:15:46Z

@LukaJCB thanks!

jcranky · 2017-10-06T09:48:04Z

In the meanwhile, please let me know if I should do something else here :)

jcranky · 2017-10-18T07:59:20Z

I guess we can now merge this as well? :)

kailuowang · 2017-10-18T12:08:44Z

core/src/main/scala/cats/data/NonEmptyList.scala

+    * }}}
+    */
+  def :::[AA >: A](other: NonEmptyList[AA]): NonEmptyList[AA] =
+    other.concat(this)


Shall we use concatNel here instead? concat is deprecated

jcranky · 2017-10-18T12:28:58Z

@kailuowang done

jcranky · 2017-10-24T15:23:32Z

@kailuowang anything else I should do here ? :)

kailuowang · 2017-10-24T15:32:11Z

The build is broken in scala doc generation

/home/travis/build/typelevel/cats/core/src/main/scala/cats/data/NonEmptyList.scala:102: The link target "concat" is ambiguous. Several members fit the target

Instead of linking it using [[]], just use concat should be fine.

jcranky · 2017-10-24T15:50:41Z

@kailuowang ok, my bad, I was blind and didn't see the build was broken. Fixed :)

kailuowang · 2017-10-24T16:36:19Z

@jcranky no worries at all. i probably should've pointed it out earlier.

jcranky · 2017-10-25T08:02:22Z

@kailuowang now the build is ok 👍 🗡️

tpolecat · 2017-11-01T23:53:25Z

It's not obvious to me what the use case is here. a ::: b/a ++: b are always equivalent to a ++ b right?

jcranky · 2017-11-02T09:30:56Z

@tpolecat my main reasoning for this was to keep symmetry with List. Plus, ++ / ++: are not there, or are they?

jcranky · 2017-11-02T09:31:39Z

(I found myself using .toList a few times just to use :::)

kailuowang · 2017-11-02T12:25:15Z

@tpolecat ++ was added to concat with List rather than NEL, for maintaining compatibility we didn't want to change that.

tpolecat

Ok, seems fine. 👍

jcranky · 2017-11-03T07:53:39Z

thanks :)

jcranky force-pushed the master branch from 7d5f92f to 6ea2425 Compare October 4, 2017 23:03

kailuowang approved these changes Oct 5, 2017

View reviewed changes

kailuowang suggested changes Oct 5, 2017

View reviewed changes

kailuowang mentioned this pull request Oct 5, 2017

Sync NEL and NEV methods (fixes #1832) #1838

Merged

LukaJCB reviewed Oct 5, 2017

View reviewed changes

kailuowang reviewed Oct 18, 2017

View reviewed changes

jcranky and others added 4 commits October 18, 2017 14:26

Add ::: to NonEmptyList

1a1ddb2

Add ++: to NEV and some doc to ::: in NEL

852e495

Add doctest for NEL.::: and NEV.++:

76cb985

Use concatNel instead of concat

497f4a2

jcranky force-pushed the master branch from e1096ff to 497f4a2 Compare October 18, 2017 12:28

Fix concat reference in scaladoc

d8b501d

kailuowang approved these changes Oct 26, 2017

View reviewed changes

LukaJCB added this to the 1.0.0 milestone Nov 1, 2017

kailuowang requested a review from tpolecat November 1, 2017 19:28

tpolecat approved these changes Nov 2, 2017

View reviewed changes

kailuowang merged commit 9912a8e into typelevel:master Nov 2, 2017

kailuowang added the enhancement label Dec 14, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ::: to NonEmptyList #1949

Add ::: to NonEmptyList #1949

jcranky commented Oct 4, 2017

codecov-io commented Oct 4, 2017 •

edited

Loading

kailuowang commented Oct 5, 2017

kailuowang Oct 5, 2017

jcranky Oct 5, 2017

jcranky commented Oct 5, 2017

LukaJCB commented Oct 5, 2017 •

edited

Loading

LukaJCB Oct 5, 2017

jcranky Oct 5, 2017

kailuowang Oct 5, 2017

jcranky commented Oct 5, 2017

jcranky commented Oct 5, 2017

LukaJCB commented Oct 5, 2017

jcranky commented Oct 5, 2017

jcranky commented Oct 6, 2017

jcranky commented Oct 18, 2017

kailuowang Oct 18, 2017

jcranky Oct 18, 2017

jcranky commented Oct 18, 2017

jcranky commented Oct 24, 2017 •

edited

Loading

kailuowang commented Oct 24, 2017 •

edited

Loading

jcranky commented Oct 24, 2017

kailuowang commented Oct 24, 2017

jcranky commented Oct 25, 2017

tpolecat commented Nov 1, 2017

jcranky commented Nov 2, 2017

jcranky commented Nov 2, 2017 •

edited

Loading

kailuowang commented Nov 2, 2017

tpolecat left a comment

jcranky commented Nov 3, 2017

Add ::: to NonEmptyList #1949

Add ::: to NonEmptyList #1949

Conversation

jcranky commented Oct 4, 2017

codecov-io commented Oct 4, 2017 • edited Loading

Codecov Report

kailuowang commented Oct 5, 2017

kailuowang Oct 5, 2017

Choose a reason for hiding this comment

jcranky Oct 5, 2017

Choose a reason for hiding this comment

jcranky commented Oct 5, 2017

LukaJCB commented Oct 5, 2017 • edited Loading

LukaJCB Oct 5, 2017

Choose a reason for hiding this comment

jcranky Oct 5, 2017

Choose a reason for hiding this comment

kailuowang Oct 5, 2017

Choose a reason for hiding this comment

jcranky commented Oct 5, 2017

jcranky commented Oct 5, 2017

LukaJCB commented Oct 5, 2017

jcranky commented Oct 5, 2017

jcranky commented Oct 6, 2017

jcranky commented Oct 18, 2017

kailuowang Oct 18, 2017

Choose a reason for hiding this comment

jcranky Oct 18, 2017

Choose a reason for hiding this comment

jcranky commented Oct 18, 2017

jcranky commented Oct 24, 2017 • edited Loading

kailuowang commented Oct 24, 2017 • edited Loading

jcranky commented Oct 24, 2017

kailuowang commented Oct 24, 2017

jcranky commented Oct 25, 2017

tpolecat commented Nov 1, 2017

jcranky commented Nov 2, 2017

jcranky commented Nov 2, 2017 • edited Loading

kailuowang commented Nov 2, 2017

tpolecat left a comment

Choose a reason for hiding this comment

jcranky commented Nov 3, 2017

codecov-io commented Oct 4, 2017 •

edited

Loading

LukaJCB commented Oct 5, 2017 •

edited

Loading

jcranky commented Oct 24, 2017 •

edited

Loading

kailuowang commented Oct 24, 2017 •

edited

Loading

jcranky commented Nov 2, 2017 •

edited

Loading