-
Notifications
You must be signed in to change notification settings - Fork 221
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
Abstract over Stream type in the outbound path #1056
Conversation
/** | ||
* | ||
*/ | ||
trait EncodeStreamToReader[S[_[_], _], F[_], A] { |
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 is essentially a slam of @sergeykolbasov 's DecodeStream
and FromReader
as we don't really need both in the outbound path (it's way simpler).
|
||
trait IterateeInstances extends LowPriorityIterateeInstances { | ||
|
||
implicit def encodeJsonEnumeratorToReader[F[_]: Effect, A](implicit |
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.
What I was also thinking is that streaming libraries doesn't all have to do that. We can somewhat define this "interleave new lines" logic for JSON generally in finch-core
(we have access to Encode.Json
there). I'll keep this in mind and maybe revisit later.
dcd3371
to
eeee229
Compare
Codecov Report
@@ Coverage Diff @@
## master #1056 +/- ##
=========================================
Coverage ? 84.53%
=========================================
Files ? 50
Lines ? 957
Branches ? 61
=========================================
Hits ? 809
Misses ? 148
Partials ? 0
Continue to review full report at Codecov.
|
eeee229
to
001cee4
Compare
ab0c26b
to
fb0bfa0
Compare
@sergeykolbasov Do you prefer this or #1042 being merged this? |
c0296f0
to
5ca183b
Compare
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.
I like it.
Do you think it's okay if I merge this one into #1042 so we can have one PR about streaming?
@@ -85,6 +85,17 @@ trait HighPriorityToResponseInstances extends LowPriorityToResponseInstances { | |||
|
|||
rep | |||
} | |||
|
|||
implicit def streamToResponse[S[_[_], _], F[_], A, CT <: String](implicit |
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.
👍
@sergeykolbasov I'm thinking we should probably go the opposite way - merge smaller pieces when possible. I was worried you'd need to resolve a lot of conflicts in your PR hence kept it open. If you're up for a challenge (and it sounds like you're), I'd say we merge this first and rebase your work. I wouldn't even mind if we split your PR into couple of independent changes if possible. |
Given #1042 is blocked on a new circe-fs2 release (am I right, @sergeykolbasov ?), should we merge this first and, perhaps, cut a release? |
@BenFradet Now that circe/circe-fs2#47 and circe/circe-fs2#50 are merged do you want to run a circe-fs2 release? I'm also happy to do it later this evening. |
I don't think I can do so myself but I'm all for it 👍 |
Okay, circe-fs2 is ready: https://github.com/circe/circe-fs2/releases/tag/v0.11.0 |
Thank you, Travis! |
@vkostyukov FYI: just merged #1042 |
Awesome @sergeykolbasov! I will rebase this tonight. |
5ca183b
to
7ae8c09
Compare
7ae8c09
to
2e841ff
Compare
Rebased. Some pieces feel repetitive but it's workable. I think we should merge this in and keep iterating. |
This to address #1038 as well as make it more pluggable to adopt new streaming libraries (eg FS2). The type-class name is inspired by what Sergey is doing in #1042. The new machinery allows us to reuse the same instances between
ToResponse
(when serving streaming content) andInput.withStream
allowing users to created chunked requests for testing.Here is how it looks and feels:
Also possible to encode JSON (as you would imagine). This does the right thing and encodes ND (newline-delimited) JSON stream, reusing the same code that we use on the outbound path:
As a bonus, this addresses #1040.