msgpax icon indicating copy to clipboard operation
msgpax copied to clipboard

Exclude default packers

Open elcritch opened this issue 5 years ago • 4 comments

Adds the option to exclude a set of default packers. The use case for this is rare, but can allow people to customize behavior as wanted in a global fashion. In this case, I need to be able to set the float serialization type to float32. I started with a branch allowing just that, but this seemed to be better and could let someone override other default packers should they want to (say serialize maps to ordered list pairs or something odd).

The config options is: config :msgpax, exclude_packers: [:atom, :map, ...].

elcritch avatar Oct 06 '20 20:10 elcritch

@elcritch thanks for bringing this up for discussion. I think we can split it into 2 aspects:

  1. Making default packers customizable
  2. Make possible to pack float32 (not necessary via 1.)

In regards to 1. I'm not sure it should be solved via conditional compilation: can lead to nasty and obscure application bugs, it also does not really make possible the customization in the umbrella applications. I think we can instead provide an extra option for pack/2 to facilitate the packing customization (I'll create an issue for that).

For now I suggest we focus on solving 2. With the existing approach users can pack float32 via wrapping such floats into a struct (%MyApp.Float32{} for example) and implement the packer protocol for it. To make that process more fluent we can provide Msgpax.Packer.pack_float32 (similarly to https://github.com/lexmag/msgpax/blob/master/lib/msgpax/packer.ex#L110.) that does all the actual packing:

defimpl Msgpax.Packer, for: MyApp.Float32 do
  def pack(%MyApp.Float32{value: value}) do
    @protocol.pack_float32(value)
  end
end

What are your thoughts on this?

lexmag avatar Jan 04 '21 17:01 lexmag

In regards to 1. I'm not sure it should be solved via conditional compilation: can lead to nasty and obscure application bugs, it also does not really make possible the customization in the umbrella applications.

Great, and I agree it'd be better to handle it without compile time logic. I'm doing it currently but it's fragile.

I think we can instead provide an extra option for pack/2 to facilitate the packing customization (I'll create an issue for that).

This would be the best option IMHO. It's much more elegant. Probably best to close this PR in favor of that issue. Not sure exactly what the API would look like?

With the existing approach users can pack float32 via wrapping such floats into a struct (%MyApp.Float32{} for example) and implement the packer protocol for it.

The Msgpax.Packer.pack_float32 would be helpful. This approach works but is a bit tedious when you have a big structure with lots of arrays which is my use case. Which is why I did the above. :/

elcritch avatar Jan 04 '21 22:01 elcritch

This would be the best option IMHO. It's much more elegant. Probably best to close this PR in favor of that issue. Not sure exactly what the API would look like?

I've created #60 that describes the approach I have in mind. Perhaps this PR can be updated to implement that approach.

The Msgpax.Packer.pack_float32 would be helpful. This approach works but is a bit tedious when you have a big structure with lots of arrays which is my use case. Which is why I did the above. :/

I see, I can imagine wrapping into a struct will be too bothersome when dealing with nesting and collections. Hopefully #61 combined with #60 should make the experience much nicer.

lexmag avatar Jan 25 '21 01:01 lexmag

can't wait this feature to support custom Atom. how about splitting it into 2 prs?

Making default packers customizable Make possible to pack float32 (not necessary via 1.)

falood avatar Jul 27 '22 00:07 falood