Add ::: to NonEmptyList - #1949
Conversation
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
|
if you want, you can add a doctest to make the test coverage happy. |
| def ::[AA >: A](a: AA): NonEmptyList[AA] = | ||
| NonEmptyList(a, head :: tail) | ||
|
|
||
| def :::[AA >: A](other: NonEmptyList[AA]): NonEmptyList[AA] = |
There was a problem hiding this comment.
Actually to maintain symmetry, can we add ++: to NonEmptyVector as well?
|
Regarding doctest, where should I look at? I new to the cats codebase. |
|
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 |
|
|
||
| /** | ||
| * Append this NEV to another NEV, producing a new `NonEmptyVector`. | ||
| */ |
There was a problem hiding this comment.
Shouldn't these be the other way around? ++ for two NEVs and ++: for an NEV and a Vector?
There was a problem hiding this comment.
I'm not sure, I went with this #1838 (comment) - perhaps we should have both for both, since overloading is happening already anyway?
There was a problem hiding this comment.
This is to mimic the API in Vector and List. Let's address #1838 first
|
@LukaJCB thanks for the explanation on doc test, I'll have a look |
|
Tests added. This doctest thing is really cool, can you point me to somewhere where I can understand better how it works? Thanks! |
|
Check out sbt-doctest :) |
|
@LukaJCB thanks! |
|
In the meanwhile, please let me know if I should do something else here :) |
|
I guess we can now merge this as well? :) |
| * res0: cats.data.NonEmptyList[Int] = NonEmptyList(1, 2, 3, 4, 5) | ||
| * }}} | ||
| */ | ||
| def :::[AA >: A](other: NonEmptyList[AA]): NonEmptyList[AA] = |
There was a problem hiding this comment.
Shall we use concatNel here instead? concat is deprecated
|
@kailuowang done |
|
@kailuowang anything else I should do here ? :) |
|
The build is broken in scala doc generation
Instead of linking it using |
|
@kailuowang ok, my bad, I was blind and didn't see the build was broken. Fixed :) |
|
@jcranky no worries at all. i probably should've pointed it out earlier. |
|
@kailuowang now the build is ok 👍 🗡️ |
|
It's not obvious to me what the use case is here. |
|
@tpolecat my main reasoning for this was to keep symmetry with |
|
(I found myself using |
|
@tpolecat |
|
thanks :) |
5 participants
No description provided.