DataSourceCache parameterised to F[_] - #160
Conversation
Codecov Report
@@ Coverage Diff @@
## master #160 +/- ##
=========================================
+ Coverage 91.12% 95.2% +4.08%
=========================================
Files 5 5
Lines 169 167 -2
Branches 5 4 -1
=========================================
+ Hits 154 159 +5
+ Misses 15 8 -7
Continue to review full report at Codecov.
|
purrgrammer
changed the title
| }) | ||
| trait DataSourceCache[F[_]] { | ||
| def lookup[I, A](i: I, ds: DataSource[I, A])( | ||
| implicit C: ConcurrentEffect[F], P: Par[F] |
There was a problem hiding this comment.
It would be nice if we didn't put these constraints on the methods anymore.
Now that we have parametrized DataSourceCache on F, the implementation could specify the constraints.
For example for InMemoryCache we would only need Applicative (Monad for foldLeftM).
There was a problem hiding this comment.
Good point, I'm going to try to remove these constraints from the trait.
|
@juanpedromoreno @peterneyens Updated with Peter's feedback, let me know if I can go ahead and merge this. |
| )( | ||
| implicit | ||
| P: Par[F], | ||
| C: ConcurrentEffect[F], |
There was a problem hiding this comment.
Could be this replaced by Monad?
There was a problem hiding this comment.
I'm afraid is not. The effect type F[_] of the DataSource instances requires a ConcurrentEffect[F] and Par[F]. For running F we need ContextShift[F] and Timer[F]so these are the less restrictive implicits.
There was a problem hiding this comment.
I think we need Sync for Ref.of and the DataSource methods still require ConcurrentEffect at the moment.
So performRun and co need ConcurrentEffect as well.
| trait DataSourceCache { | ||
| trait DataSourceCache[F[_]] { | ||
| def insert[F[_] : ConcurrentEffect, I, A](i: I, ds: DataSource[I, A], v: A): DataSourceIdentity, v: A): F[DataSourceCache] | ||
| def lookup[F[_] : ConcurrentEffect, I, A](i: I, ds: DataSource[I, A]): F[Option[A]] |
There was a problem hiding this comment.
These are no longer correct, right?
| )( | ||
| implicit | ||
| P: Par[F], | ||
| C: ConcurrentEffect[F], |
There was a problem hiding this comment.
I think we need Sync for Ref.of and the DataSource methods still require ConcurrentEffect at the moment.
So performRun and co need ConcurrentEffect as well.
|
@peterneyens @juanpedromoreno i opened #163 to discuss about the implicits required when implementing data sources, let me know your thoughts in there. Can I merge this |
?
peterneyens
left a comment
peterneyens
left a comment
There was a problem hiding this comment.
Nice improvement of DataSourceCache
DataSourceCache parameterised to F[_]
3 participants
I changed
DataSourceCacheso we can parameterise it toF[_] : Monad. It also helps for implementing custom caches that make use of io for serialization/deserialization.