Skip to content
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

(WIP) Replace `Chunk` For `Segment` where appropriate #1588

Merged
merged 7 commits into from Dec 11, 2017

Conversation

@jmcardon
Copy link
Member

@jmcardon jmcardon commented Dec 8, 2017

This is to improve on the previous proliferation of chunks everywhere, especially after the fs2 M9 changes.

@@ -22,7 +22,7 @@ trait CirceInstances {

private def jsonDecoderByteBufferImpl[F[_]: Effect](msg: Message[F]): DecodeResult[F, Json] =
EntityDecoder.collectBinary(msg).flatMap { chunk =>
val bb = ByteBuffer.wrap(chunk.toBytes.values)
val bb = ByteBuffer.wrap(chunk.force.toArray)

This comment has been minimized.

@jmcardon

jmcardon Dec 8, 2017
Author Member

the variable should be called segment

This comment has been minimized.

EntityDecoder.decodeBy(MediaRange.`*/*`)(collectBinary[F])

implicit def binaryChunk[F[_]: Effect]: EntityDecoder[F, Chunk[Byte]] =
EntityDecoder.decodeBy(MediaRange.`*/*`)(collectBinary[F]).map(_.force.toChunk)

This comment has been minimized.

@jmcardon

jmcardon Dec 8, 2017
Author Member

this should just map on the previous entity decoder.

This comment has been minimized.

@rossabaker

rossabaker Dec 9, 2017
Member

Does this even need to exist?

@@ -70,7 +70,7 @@ object EntityEncoder extends EntityEncoderInstances {
*
* This constructor is a helper for types that can be serialized synchronously, for example a String.
*/
def simple[F[_], A](hs: Header*)(toChunk: A => Chunk[Byte])(
def simple[F[_], A](hs: Header*)(toChunk: A => Chunk[Byte])( //Fishy but I'm wondering wat to do

This comment has been minimized.

@jmcardon

jmcardon Dec 8, 2017
Author Member

Unsure about what to do re: A => Chunk.

This comment has been minimized.

@rossabaker

rossabaker Dec 9, 2017
Member

What is a use case for creating a segment here instead of a chunk? This encoder is strict.

This comment has been minimized.

@jmcardon

jmcardon Dec 9, 2017
Author Member

hmm, the only reason I was wondering if there was anything I could do, is that toChunk currently makes me do a segment.force further down.

checkAll("EntityCodec[IO, String]", EntityCodecTests[IO, String].entityCodec)
checkAll("EntityCodec[IO, Array[Char]]", EntityCodecTests[IO, Array[Char]].entityCodec)

checkAll("EntityCodec[IO, Chunk[Byte]]", EntityCodecTests[IO, Chunk[Byte]].entityCodec)
checkAll("EntityCodec[IO, Chunk[Byte]]", EntityCodecTests[IO, Segment[Byte, Unit]].entityCodec)

This comment has been minimized.

@jmcardon

jmcardon Dec 8, 2017
Author Member

Should be Segment

Future.successful(false)
}

override protected def writeBodyChunk(chunk: Chunk[Byte], flush: Boolean): Future[Unit] = {
buffers += chunk
buffer = buffer ++ Segment.chunk(chunk)

This comment has been minimized.

@rossabaker

rossabaker Dec 9, 2017
Member

This should be synchronized if the others are.

@@ -22,7 +22,7 @@ trait CirceInstances {

private def jsonDecoderByteBufferImpl[F[_]: Effect](msg: Message[F]): DecodeResult[F, Json] =
EntityDecoder.collectBinary(msg).flatMap { chunk =>
val bb = ByteBuffer.wrap(chunk.toBytes.values)
val bb = ByteBuffer.wrap(chunk.force.toArray)

This comment has been minimized.

EntityDecoder.decodeBy(MediaRange.`*/*`)(collectBinary[F])

implicit def binaryChunk[F[_]: Effect]: EntityDecoder[F, Chunk[Byte]] =
EntityDecoder.decodeBy(MediaRange.`*/*`)(collectBinary[F]).map(_.force.toChunk)

This comment has been minimized.

@rossabaker

rossabaker Dec 9, 2017
Member

Does this even need to exist?

@@ -70,7 +70,7 @@ object EntityEncoder extends EntityEncoderInstances {
*
* This constructor is a helper for types that can be serialized synchronously, for example a String.
*/
def simple[F[_], A](hs: Header*)(toChunk: A => Chunk[Byte])(
def simple[F[_], A](hs: Header*)(toChunk: A => Chunk[Byte])( //Fishy but I'm wondering wat to do

This comment has been minimized.

@rossabaker

rossabaker Dec 9, 2017
Member

What is a use case for creating a segment here instead of a chunk? This encoder is strict.

@@ -148,6 +148,9 @@ trait EntityEncoderInstances extends EntityEncoderInstances0 {
charset: Charset = DefaultCharset): EntityEncoder[F, Array[Char]] =
stringEncoder[F].contramap(new String(_))

implicit def segmentEncoder[F[_]: Applicative]: EntityEncoder[F, Segment[Byte, Unit]] =
chunkEncoder[F].contramap[Segment[Byte, Unit]](_.force.toChunk) //TODO: Seems fishy

This comment has been minimized.

@rossabaker

rossabaker Dec 9, 2017
Member

Maybe this is what sucks because of the definition of simple.

I don't yet have a strong opinion on whether we need an encoder for both Segment and Chunk.

This comment has been minimized.

@jmcardon

jmcardon Dec 9, 2017
Author Member

If we define both a decoder and an encoder, we get an extra test for free in the entitycodec spec is all, so I figured this would be okay.

charset: Charset = StandardCharsets.UTF_8
) extends Writer {

override def append(s: String): this.type = {
// This smells wrong
toChunk = (toChunk.toSegment ++ Segment.array(s.getBytes(charset))).force.toChunk
toByteSegment = toByteSegment ++ Segment.array(s.getBytes(charset))

This comment has been minimized.

@rossabaker

rossabaker Dec 9, 2017
Member

Does this still smell wrong?

This comment has been minimized.

@jmcardon

jmcardon Dec 9, 2017
Author Member

should be fine now.

override def combine(x: Chunk[A], y: Chunk[A]): Chunk[A] =
(x.toSegment ++ y.toSegment).force.toChunk
//Maybe this belongs in fs2?
implicit def http4sMonoidForChunk[A]: Monoid[Segment[A, Unit]] =

This comment has been minimized.

@rossabaker

rossabaker Dec 9, 2017
Member

The name is now wrong.

If this is still used, it belongs in fs2. It's not our types.

This comment has been minimized.

@jmcardon

jmcardon Dec 9, 2017
Author Member

I think I can PR this onto fs2.

This comment has been minimized.

@jmcardon

jmcardon Dec 9, 2017
Author Member

Also yeah, it's still used, we use runFoldMonoid a lot

jmcardon added 5 commits Dec 9, 2017
@aeons
aeons approved these changes Dec 11, 2017
@aeons aeons merged commit 34efac7 into http4s:master Dec 11, 2017
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jmcardon jmcardon mentioned this pull request Dec 15, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.