Add a memoizedAcquire method to Resource
#3890
Conversation
| Concurrent[Resource[F, *]].memoize[A](self).map { | ||
| case Resource.Eval(fa) => fa | ||
| case unexpected => | ||
| throw new IllegalStateException( | ||
| s"Memoized Resource is not Resource.Eval: $unexpected") | ||
| } |
There was a problem hiding this comment.
Actually, we can avoid this silliness if we go the other way: put the juicy implementation in here, and then memoize can delegate to memoizedAcquire and wrap it up in Resource.eval.
There was a problem hiding this comment.
Also then, the existing memoize tests seem like sufficient coverage to me.
armanbilge
changed the title
memoizedAcquire method to Resource
|
To test this I would write to Refs in your acquire and use, and assert on the state of those refs to check that it's behaving as you expect. I wouldnt worry about concurrent testing since memoize is pulling most of the weight |
|
I don't think we need to add any new tests specifically because of this PR, unless there are existing gaps in the existing To be clear, what I'm proposing is that we refactor so it looks like this: def memoizedAcquire(ra: Resource[F, A]): Resource[F, F[A]] =
/* move existing memoize implementation here */
def memoize(ra: Resource[F, A]): Resource[F, Resource[F, A]] =
memoizedAcquire.map(Resource.eval(_))Then all the existing |
|
Yeah fair enough, with that refactor I would agree that we don't need any new tests |
|
I'm a huge fan of not having to add new tests. I will get to that refactor when/as I have time but if anyone wants to beat me to it, the branch is open for maintainer commits ;) |
Ref #3513
I'm not sure how to unit test this exactly, I'm open to guidance or commits for that