-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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 NonEmptyChain #2406
Add NonEmptyChain #2406
Conversation
Urgh, I'm apparently really bad at using git 🤦🏻♂️ |
|
||
|
||
sealed class NonEmptyChainOps[A](val value: NonEmptyChain[A]) { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this extend AnyVal?
val iter: Iterator[AA] = toChain.iterator | ||
var result = iter.next | ||
while (iter.hasNext) { result = S.combine(result, iter.next) } | ||
result |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should use combineAllOption.get so we don’t deoptimize combineAll
@@ -76,6 +76,12 @@ object arbitrary extends ArbitraryInstances0 { | |||
implicit def catsLawsCogenForNonEmptyList[A](implicit A: Cogen[A]): Cogen[NonEmptyList[A]] = | |||
Cogen[List[A]].contramap(_.toList) | |||
|
|||
implicit def catsLawsArbitraryForNonEmptyChain[A](implicit A: Arbitrary[A]): Arbitrary[NonEmptyChain[A]] = | |||
Arbitrary(implicitly[Arbitrary[List[A]]].arbitrary.flatMap(fa => A.arbitrary.map(a => NonEmptyChain(a, fa: _*)))) | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use the Arbitrary for Chain and add one more to the front or back also as a variant?
@LukaJCB it's not you are bad at using git, it's just the other PR is merged squashed. Since the changes look good, I think if we squash this one as well it will be okay. |
Codecov Report
@@ Coverage Diff @@
## master #2406 +/- ##
=========================================
+ Coverage 94.88% 95.2% +0.32%
=========================================
Files 350 351 +1
Lines 6258 6363 +105
Branches 284 285 +1
=========================================
+ Hits 5938 6058 +120
+ Misses 320 305 -15
Continue to review full report at Codecov.
|
fa.toNonEmptyList | ||
} | ||
|
||
implicit def catsDataEqForNonEmptyChain[A: Eq]: Eq[NonEmptyChain[A]] = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we only have Eq
here, no Order, PartialOrder, right? Can we add those so we get the priority right there?
9f0954f
to
e5b414c
Compare
val iter = iterator | ||
var i: Int = 0 | ||
var i: Long = 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just curious, did we actually tried one instance with more than 2 billion items?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No I don't think so, I just wanted to be consistent with NonEmptyList and NonEmptyVector :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is really great. Nice work getting the test coverage up so high!
Merging with two signoffs :) |
* Add Catenable * Add law tests and simple arbitrary and eq instances * More tests * Add benchmarks * Add chain benchmarks * Add Iterator * More chain benchmarks * Add Paul and Pavel to Authors and change COPYING to Cats Contributors * More Tests * Add reverse, groupBy and zipWith * Add Collection Wrapping optimization * Add traverse and foldRight implementations that don't convert to List * Rename to Chain; add reverseIterator * More efficient implementations * Use Vector for reversing * Add NonEmptyChain * Add more fine grained operations to interop with Chain * Add NonEmptyChain * Use Chain For Arbitrary, don't deoptimize combineAll and syntax extends AnyVal * Add PartialOrder and Order * Add more tests * Even more tests
Based on the work in #2371, though we might still need to change a couple of things first, including the
Arbitrary
Generator.