(WIP) Introduce Thrift.Serializable#441
Conversation
fishcakez
requested review from
dantswain,
jparise,
pguillory,
scohen and
thecodeboss
as code owners
| @doc """ | ||
| Serialize a struct to a payload | ||
| """ | ||
| @spec serialize(thrift_struct, payload) :: payload when thrift_struct: struct, payload: var |
There was a problem hiding this comment.
When is the payload not iodata?
There was a problem hiding this comment.
This is always a protocol struct, wrapping the payload. I use a variable for the type as the second arg type should always match the result type. Looks like some confusion in the code around payload and the wrapping struct due to variable naming. The structs should be renamed.
| @@ -133,36 +153,6 @@ defmodule Thrift.Protocol.Binary do | |||
| {:error, {:cant_decode_message, rest}} | |||
There was a problem hiding this comment.
Instead of :cant_decode_message, would :bad_message or :decode_error be better?
There was a problem hiding this comment.
Definitely should improve the error handling. I think I will do a second PR after this to introduce transport protocol that will complete the decoupling of code generation for and transports.
Yes, I can see that, but I agree with your overall conclusion that this is still the best multi-protocol abstraction. I think it would only make sense to do more aggressive things if it unlocked a performance improvement, for example. Let's proceed down the current path, and we can revisit once we have everything working. |
| defmodule BinaryProtocol do | ||
| @moduledoc false | ||
| unquote_splicing(binary_protocol_defs) | ||
| defprotocol SerDe do |
There was a problem hiding this comment.
Should we introduce a defserde SerDe, for: name macro in Thrift.Protocol that generates this SerDe protocol for a module? We could use it here and also where we define TApplicationException.SerDe. It would be used by additional protocol implementations, too.
|
hi @fishcakez, any update for this pull? |
3 participants
WIP for #343 and improve support for other protocols (#333). Note I did not implement the proposed
Thrift.Messageas it was not required for an initial step. The main goal is to do minimal work to getThrift.Serializableworking and a step towards making the code generation for a client or server protocol independent.Having to use %Thrift.Protocol.Binary{} to wrap binaries everywhere is awkward, perhaps we should implement
binaryforThrift.SerializableorStruct.SerDethat does binary protocol too? On the other hand this should be abstracted for most usecases. I think if we wish to have Binary and Compact on equal footing we should not allow that as we want to ensure to type the payloads...