msgpack-crystal icon indicating copy to clipboard operation
msgpack-crystal copied to clipboard

have Serializable override inspect?

Open will opened this issue 5 years ago • 2 comments

I'm not sure, but would it be a good idea to have it so including Serializable also overwrites inspect(io : IO) to include the properties?

The upside, is as a client instead of getting something like this

server

record Details, name : String, count : Int32 do
    include MessagePack::Serializable
end

def add_thing(id : String, details : Details) : Nil
end

client

c.add_thing  "hi", {name: "will"}
Traceback (most recent call last):
        6: from ./repl:32:in `<main>'
        5: from (irb):2
        4: from (irb):2:in `rescue in irb_binding'
        3: from ./repl:12:in `method_missing'
        2: from /Users/will/.asdf/installs/ruby/2.6.6/lib/ruby/gems/2.6.0/gems/msgpack-rpc-0.6.0/lib/msgpack/rpc/session.rb:54:in `call'
        1: from /Users/will/.asdf/installs/ruby/2.6.6/lib/ruby/gems/2.6.0/gems/msgpack-rpc-0.6.0/lib/msgpack/rpc/future.rb:54:in `get'
MessagePack::RPC::RuntimeError (bad arguments, expected [id : String, details : Details], but got details: HashT[1])

the error could maybe be nicer to better see what you were missing

MessagePack::RPC::RuntimeError (bad arguments, expected [id : String, details : Details[name : String, count : Int32], but got details: HashT[1])

There might be downsides though, but I'm not aware of what they would be, so I wanted to check first if this would be good.

I tried doing a PR to show this, but I can't figure out the macros :( in the macro included section, @type.instance_variables seems to be empty, and I don’t have a ton of experience with macros

will avatar Aug 04 '20 13:08 will

this is issue for simple_rpc, also this is quite hard, because type can have subtypes, or array of subtypes, and inspect it property is not easy.

kostya avatar Aug 04 '20 15:08 kostya

Yeah I'm using simple_rpc, but I opened it here, since when I was trying to solve it, this was where I thought the change would go.

I wonder if for rpc, it's better off in general to use NamedTuples instead of record macro structs?

will avatar Aug 04 '20 15:08 will