Instances for com.twitter.concurrent.AsyncStream #62
Conversation
Codecov Report
@@ Coverage Diff @@
## master #62 +/- ##
==========================================
- Coverage 52.84% 49.46% -3.38%
==========================================
Files 8 9 +1
Lines 176 188 +12
Branches 2 2
==========================================
Hits 93 93
- Misses 83 95 +12
Continue to review full report at Codecov.
|
|
Thanks! And yep, that's the only way to do Did you look into writing a stack-safe |
|
I haven't yet, I just cribbed the current unsafe one from |
|
@travisbrown I switched |
|
@travisbrown I'm at the point where I've gotten the code I want in and ready. Not sure why it's being reported as not tested, it should be fully covered by the law checks. |
|
Sorry about the delay in review—this looks generally good to me, but I think we should be sure to create non-singleton arbitrary instances (which should improve test coverage as well). |
courtesy of StackSafeMonad. Tests now assume stack safety and continue to pass.
for AsyncStream
|
I'm not sure what you mean by "non-singleton arbitrary instances" |
|
Right now you have |
|
What about the following: implicit def asyncStreamArbitrary[A](implicit A: Arbitrary[A]): Arbitrary[AsyncStream[A]] =
Arbitrary(
Arbitrary.arbitrary[Stream[A]].map(AsyncStream.fromSeq)
)The current version using |
|
Thanks! Just restarted the build to see if that resolves the timeouts. |
see if bumping the heap available to sbt fixes things. It's a valid technique with sbt and travis
| /** | ||
| * Note that this implementation is not stack-safe. | ||
| */ | ||
| final def tailRecM[A, B](a: A)(f: A => Var[Either[A, B]]): Var[B] = f(a).flatMap { |
There was a problem hiding this comment.
It looks like the law-checking for Var is still using StackUnsafeMonad, though?
There was a problem hiding this comment.
Yes, it still is. The easy fix of using StackSafeMonad failed and rather than leave the code seeming to be be stack safe I left it unchanged (and still commented). I didn't have the time at hand to dive into Var to understand why it's not behaving. I think I should early this week
double heap for sbt/build
|
I'd be happy to see this merged with both |
|
Alright, I'll revert back to the explicit unsafe version of |
since even the StackSafeMonad version threw stack overflow errors.
In order to test it it requires Cogen which needs Comonad which I can't figure out how to implement. Return arbitrary instances to use `AsyncStream.of` allowing the testing of only one item at a time. This is due to the fact that `fromSeq` causes testing hangs locally.
|
Looking at the codecov report and comparing it to what scoverage says makes me wonder if there's something missing in the integration. scoverage says the code coverage for the |
|
@crispywalrus Yeah, I don't know what's up with the codecov reporting. I've seen something similar in other repos—e.g.: circe/circe-derivation#4 |
Something like a year ago
AsyncStreamwas added into twitter util. Up till now it was a mystery to me. Now that I've met it I'd like it to play nicely with the rest of cats and in order to do that it needs some instance magic. This aims to provide that for those type classes I currently need. It bothers me that I had toAwaitforEqto work but since that's what was done forFutureI assume it's acceptable.I hope.
It does in fact test out nicely.