-
Notifications
You must be signed in to change notification settings - Fork 49
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
Remove Par typeclass #166
Remove Par typeclass #166
Conversation
Codecov Report
@@ Coverage Diff @@
## master #166 +/- ##
======================================
Coverage 95.2% 95.2%
======================================
Files 5 6 +1
Lines 167 167
Branches 4 7 +3
======================================
Hits 159 159
Misses 8 8
Continue to review full report at Codecov.
|
I'm not sure if we can support running parallel computations and making them cancelable, since the import cats.effect.{Async, Fiber, IO, CancelToken}
trait Concurrent[F[_]] extends Async[F] {
// ...
def cancelable[A](k: (Either[Throwable, A] => Unit) => CancelToken[F]): F[A]
} For now I've made the fibers we spawn uncancelable, so in case the fetch computation is cancelled, it will terminate as soon as all concurrent fibers have finished executing. |
I have some ideas for supporting cancellation of parallel computations, I'll update the PR when I manage to implement it. |
Looks great so far, let me know when ready for final review. 🍻 |
@raulraja not sure why the coverage doesn't cover the newly introduced |
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 looks great, if @peterneyens has no additional comments I think this is good to go. kudos and great work!
|
||
def countFetches(r: Request): Int = | ||
r.request match { | ||
case FetchOne(_, _) => 1 |
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.
Formatting seems off in a few match cases here. Are we enforcing auto formatting with Scalafmt or similar?
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.
Not right now, I can open a separate PR for adding scalafmt and not polluting this one
Cleaned this up following @peterneyens suggestions, let me know if ✔️ |
any chance to make a release (RC or milestone) with this PR included? Would like to try this for a small project... |
Remove Par typeclass
After discussing with @peterneyens how we could get rid of the
Par
typeclass he suggested usingConcurrent#start
andFiber
s to achieve parallelism whenever we usedparTraverse
. This is my first approach to implementing it.cats-par
dependencyPar
implicit requirementConcurrrent#start
andFiber
's