lunatic-rs icon indicating copy to clipboard operation
lunatic-rs copied to clipboard

Add serializer generic to types

Open tqwewe opened this issue 2 years ago • 3 comments

Closes #63

This PR removes the internal use of Bincode and instead allows for the serializer to be generic. Some common types such as AbstractProcess now take a generic parameter S for the serializer.

All structs and traits with a serializer generic default to Bincode, including some which were present before this PR.

The #[abstract_process] macro also supports a new attribute for using a custom serializer. #[abstract_process(serializer = Json)]

Todo:

  • [x] Figure out why the impl_supervisable! macro now conflicts with the other implementation.
  • [x] ~Add some tests to ensure other serializers work.~ no longer required
  • [x] ~Encode message length in other serializers (currently only added to Json).~ no longer required

There were a lot of changes here guided by the Rust compiler. It's possible some trait bounds are incorrect for usage, but still compiles. I tried my best to remove any unecessary trait bounds.

This PR should not be a breaking change, just adds some generics, but keeps them defaulted to Bincode.

tqwewe avatar Aug 16 '22 07:08 tqwewe

https://github.com/lunatic-solutions/lunatic-rs/blob/76b8230c7c1726ff4c27fbe89595a9f5a372fd38/src/supervisor.rs#L169-L175

I have implemented Supervisable for all the in-built serializers as a single item (not in a tuple), except ProtocolBuffers, because there were some requirement issues with types needing to be protobuf::Message.

tqwewe avatar Aug 17 '22 08:08 tqwewe

I believe that I have been overthinking this. You could express the same dependency without a M parameter. Something like this:

pub trait AbstractProcess<S = Bincode>
where
    S: Serializer<Self::Arg>,
{
    type Arg;
    // rest ...
}

That way you force Self::Arg to be serializable with S, but default to Bincode. I think that should work fine with any serializer.

bkolobara avatar Aug 18 '22 18:08 bkolobara

I've added this here now: https://github.com/lunatic-solutions/lunatic-rs/pull/64/commits/c2bec31d1e2e099a24cea0ec8848a6860cafeb7f

tqwewe avatar Aug 19 '22 05:08 tqwewe

I'm going to close this for now since it's quite a large change and main branch has become quite out of sync. If we need to come back to this, we can reopen this PR.

tqwewe avatar Oct 21 '22 06:10 tqwewe