Asmod4n / mruby-simplemsgpack

mruby wrapper for msgpack (>= 1.0) / msgpack.org[mruby]
Apache License 2.0
5 stars 3 forks source link

Optional/configurable packing of procs #5

Closed christopheraue closed 7 years ago

christopheraue commented 7 years ago

Right now, packing of procs is enabled by default and packs it in a format only useful in mruby without a way to customize that.

I just stumbled across an "unexpected extension type" after a proc has been packed in mruby and I was trying to unpack it in Ruby. I want procs to be packed just as their string representation. Preferably without an ext type at all.

Proposed solution: 1) Remove hard coded packing of procs (like here and here). 2) Register a custom packer for procs with MessagePack#register_pack_type(proc_ext_type, Proc, &packer) like for other classes. To pack a proc as irep could be configured with something like MessagePack#register_pack_type(proc_ext_type, Proc, :irep_packer). 3) Similarly, MessagePack#register_unpack_type(proc_ext_type, :irep_unpacker).

What do you think?

Asmod4n commented 7 years ago

It can already be customised inside mrbgem.make and you cannot pack or unpack procs in ruby alone, it has to be done in a c extension.

I would say it could be done via #ifdefs in the c extension so it only gets enabled when they can be unpacked too. One could simply remove https://github.com/Asmod4n/mruby-simplemsgpack/blob/master/mrbgem.rake#L9 and make it a build_conf.rb setting.

Asmod4n commented 7 years ago

e.g.

conf.gem mgem: 'mruby-simplemessagepack' do |spec|
  spec.cc.defines << 'MRB_MSGPACK_PROC_EXT=127'
end
Asmod4n commented 7 years ago

Another way i could think about it is to create new module functions to enable packing/unpacking of procs, but not with the pack_type functions.

christopheraue commented 7 years ago

Yes, the extension type (the number between 0 and 127) can be changed. What I need is to have no ext type or to customize the packer. I'd prefer having no ext type.

The proposal to extend the pack_type methods was an attempt to set the proc packer (irep or different) with the known interface. I also thought about another set of methods to do that. But I find it inconsistent if the normal interface for all other classes suggests

MessagePack.register_pack_type(5, Proc) { |proc| proc.to_s }

but to set the packer for a proc you'd need to use a different method.

Hmm, thinking about it again... What about exposing the irep as Proc#to_irep? Then one could pack procs that way with

MessagePack.register_pack_type(5, Proc) { |proc| proc.to_irep }
Asmod4n commented 7 years ago

Sounds interesting to have Proc#to_irep and Proc.from_irep, i guess ill write a gem for it.

Asmod4n commented 7 years ago

https://github.com/Asmod4n/mruby-proc-irep-ext

What do you think? ^^

christopheraue commented 7 years ago

Looks good, I think. So now, if someone needs to pack procs as irep they include the irep mgem and register a pack type for procs the normal way. Which means everything special about packing procs in this mgem can be removed. Or am I missing something?

Asmod4n commented 7 years ago

yes, almost done.

Asmod4n commented 7 years ago

Done. Closed via 2155a6b

christopheraue commented 7 years ago

Great. Thank you!