Skip to content

Conversation

@felixbr
Copy link
Contributor

@felixbr felixbr commented Sep 23, 2019

This is an attempt to do a similar implementation as with Future to complete issue #90.

I want to stress that I've never used Rerunnable. My guess is the semantics are similar to cats.effect.IO (referentially transparent, lazy, no memoization), but you might want to check if my implementation is what you want from Rerunnable.

I've used Future.join as a way to run two Rerunnable types in parallel. I haven't seen a direct way without Future. Please check if this is ok.

As mentioned in the last review the Newtype1 helper is only needed for 2.10, so I tried to use a more direct encoding this time. Maybe it's possible to do it with even less indirection, but I kept the code close to Future and cats.effect.IO for the time being.

Cheers
~ Felix

@codecov-io
Copy link

codecov-io commented Sep 23, 2019

Codecov Report

Merging #161 into master will decrease coverage by 0.79%.
The diff coverage is 88.88%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #161     +/-   ##
=========================================
- Coverage   94.78%   93.98%   -0.8%     
=========================================
  Files           5        5             
  Lines         115      133     +18     
  Branches        3        3             
=========================================
+ Hits          109      125     +16     
- Misses          6        8      +2
Impacted Files Coverage Δ
...il/src/main/scala/io/catbird/util/Rerunnable.scala 90.24% <88.88%> (-0.39%) ⬇️

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 6d027cc...ab7f79d. Read the comment docs.

@felixbr
Copy link
Contributor Author

felixbr commented Sep 23, 2019

codecov complains about rerunnableParEq which I only added for consistency with Future. It's not used currently, so I could also remove it...

@travisbrown
Copy link
Contributor

Sorry for the long delay in review, but thanks for doing this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants