Conversation
New rho types. Step 1 - rewrite normalizer types
| def serialize(par: RhoTypeN): Eval[Array[Byte]] = { | ||
|
|
||
| /** Wrapper for protobuf serialization of primitive types. */ | ||
| class ProtobufPrimitiveWriter(output: CodedOutputStream) extends PrimitiveWriter { |
There was a problem hiding this comment.
This class connects two worlds. Our local code with external library and it's basically the only connection that serialization has with protobuf.
So it should be in a separate folder together with the constructor (apply).
Another function should provide instantiation of writer and output stream, but not mentioning Serialization.serialize because we are now in protobuf folder!
| /** Wrapper for protobuf serialization of primitive types. */ | ||
| private class ProtobufPrimitiveWriter(output: CodedOutputStream) { | ||
| def write(x: Byte): Eval[Unit] = Eval.later(output.writeRawByte(x)) | ||
| trait PrimitiveWriter { |
There was a problem hiding this comment.
Trait should have generic parameter (F[_]). Eval is used in implementation.
| // Recursive traversal of the whole object at once | ||
| implicit val rec: RhoTypeN => Eval[Unit] = writeRec | ||
| // Recursive traversal using memoized values | ||
| // implicit val rec: RhoTypeN => Eval[Unit] = _.serialized.flatMap(primitiveWriter.writeRaw) |
There was a problem hiding this comment.
Why all this complication with implicit function?
The point of different ways to serialize is to be able to select it on top call. serialize function should have control how serialization is done. We want to be able to call both in the same source code.
There was a problem hiding this comment.
Might be using implicit is not the best way here, I'll make an explicit dependency.
What I'm trying to so is to separate ParN traversal from writing to wire. So traversal is separate and serialisation is an implementation for processing of primitives while traversing.
There was a problem hiding this comment.
It's already separated by PrimitiveWriter and PrimitiveReader.
| Serialization.serialize(par, writer).map { _ => | ||
| baos.flush() | ||
| val r = baos.toByteArray | ||
| cos.flush() |
There was a problem hiding this comment.
After baos.toByteArray the read from output stream is done. So call to flush should be before getting final bytes, and to be 100% correct before outer ByteArrayOutputStream.
| baos.flush() | ||
| baos.toByteArray | ||
| } | ||
| }.get |
There was a problem hiding this comment.
CodedOutputStream is internal to protobuf, caller should not need to know when returning bytes.
We have serializer which needs Writer and Reader so this is what protobuf here should provide.
There was a problem hiding this comment.
Not sure why caller should not know about CodedOutputStream. This file is only about protobuf and I did not want to pull here anything related to serialisation, or ParN. If I have here only PrimitiveWriter - I have to also pull and instance of ParN that I want to serialise. But this is what should be avoided? ?
This function basically creates a CodedOutputStream of specific size and manages the lifecycle. Everything else is provided by user.
| write(ELIST) *> writeSeq(eList.ps, writePar) *> writeOpt(eList.remainder, writePar) | ||
| case eTuple: ETupleN => write(ETUPLE) *> writeSeq(eTuple.ps, writePar) | ||
| case eSet: ESetN => | ||
| write(ESET) *> writeSeq(eSet.sortedPs, writePar) *> writeOpt(eSet.remainder, writePar) |
There was a problem hiding this comment.
Can list and set be in one line as before?
There was a problem hiding this comment.
writeSeq is now parametrised over type so accepts one more argument. I can only do this if remove this argument somehow. But I don't want to pull ParN type into the PrimitiveWriterOps
def writeSeq[T](seq: Seq[T], writeT: T => F[Unit])(
implicit applicativeF: Applicative[F]
): F[Unit] =
writer.write(seq.size) *> seq.traverse_(writeT)| object PrimitiveWriterSyntax { | ||
| implicit final def primitiveWriterSyntax[F[_]: Applicative]( | ||
| writer: PrimitiveWriter[F] | ||
| ): PrimitiveWriterOps[F] = new PrimitiveWriterOps[F](writer) |
There was a problem hiding this comment.
Kudos for defining these functions as extensions.
3 participants
Overview
Notes
Please make sure that this PR:
Bors cheat-sheet:
bors r+runs integration tests and merges the PR (if it's approved),bors tryruns integration tests for the PR,bors delegate+enables non-maintainer PR authors to run the above.