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

Exclude default packers #55

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

elcritch
Copy link

@elcritch elcritch commented Oct 6, 2020

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, ...].

- update deprecated function in plug_parser
- setting up ability to override packers
@lexmag
Copy link
Owner

lexmag commented Jan 4, 2021

@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?

@elcritch
Copy link
Author

elcritch commented Jan 4, 2021

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. :/

@lexmag
Copy link
Owner

lexmag commented Jan 25, 2021

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 lexmag force-pushed the master branch 2 times, most recently from a157d5d to bcec987 Compare March 27, 2021 22:58
@falood
Copy link

falood commented Jul 27, 2022

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.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants