removed the Ref boolean #6654
removed the Ref boolean #6654
Conversation
|
Thanks for retargeting! At least according to #6325 (review) we do still need the |
geoffjohn11
force-pushed
the
RemoveRedundantRef
branch
from
676a453
to
cec7ec9
Compare
geoffjohn11
force-pushed
the
RemoveRedundantRef
branch
from
05ed774
to
d640a7c
Compare
Remove un-needed Context Bound Co-authored-by: Arman Bilge <armanbilge@gmail.com>
|
@armanbilge @danicheg @kubukoz I think the ClientSuite test is missing the case for Stream Request -> Strict Response, and Stream Request -> Stream Response. I'd like to add these tests, and then will this PR be good to go? |
|
@geoffjohn11 thanks for all your work on this! Yes please, adding tests is always great, good catch. |
|
@armanbilge Thanks for your feedback and reviews! Hopefully this is good to go. |
| refOp.fold { | ||
| Resource.suspend(Ref[F].of(false).map { disposed => | ||
| Resource | ||
| .eval(F.pure(resp.pipeBodyThrough(until(disposed)))) | ||
| .onFinalize(disposed.set(true)) | ||
| }) | ||
| } { disp => | ||
| Resource | ||
| .eval(F.pure(resp.pipeBodyThrough(until(disp)))) | ||
| .onFinalize(disp.set(true)) |
There was a problem hiding this comment.
I'm a little confused by these lines.
Resource.eval(F.pure(...))is equivalent toResource.pure(...)- Since the code is the same, and there is just the issue of whether the
Refis provided or not, I think you can dorefOp.fold(Ref[F].of(false))(F.pure).flatMap { disp => ... }and then share the code.
There was a problem hiding this comment.
@armanbilge I think I fixed the duplication, let me know if you'd like additional changes. Thanks for the review!
client/shared/src/test/scala/org/http4s/client/ClientSuite.scala
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
I think this is good. Thanks for all your work on it.
General comment (not related to your changes): personally I found run difficult to reason about (and thus this PR difficult to review), since it handles both requests/responses and calls itself recursively. I wonder if anyone else feels the same.
PR to address Ref comment in #6325 (review)