MessagePack.swift icon indicating copy to clipboard operation
MessagePack.swift copied to clipboard

Added Codable support

Open andrew-eng opened this issue 6 years ago • 6 comments

This is a lightweight support for Codable completed with tests.

A few features are missing:

  1. "codingPath" is missing in DecodingError.Context when decoding error is thrown
  2. "userInfo" is not propagated
  3. "superDecoder" and variants are not supported

So far in my daily work, we haven't encountered use cases that require the above functionality.

andrew-eng avatar Mar 28 '18 08:03 andrew-eng

  • The coding path is helpful to find the failing class if such an error is thrown.
  • As fas as I know userInfo is just for the user to mark some decoders for whatever (It anyway not a big thing to implement this, it isn't with the coding path either, so...)
  • I don't know about the super decoder either. It seems to be meant to, well, (de|en)code a super class in init(from:) and encode(to:), and because you can't call container.encode(super), I guess. However, I think the implementation of superDecoder is finished with returning just a new _MessagePackDecoder and for the superEncoder it would be best to just nest a storage and give this storage to a new _MessagePackEncoder. I think that's it.

Not using such a stack like storage (like JSONEncoder does) is brilliant! It also solves a lot of issues JSONEncoder has (like decode after an error was thrown, which didn't work in the beginning but works now), I don't know whether these were also you're thoughts, but just don't using a "stack storage" is in my opinion really 💎.

There is just one issue with your implementation, I think Data will not encode to .binary, but to .array (the way it does by default). I saw the test for that, but I think you're new test just didn't run neither on linux, nor on macOS (?). Of course, I may be totally mistaken, but I could bet I'm not (Have your run the test yourself?). The thing is, if a Data instance is passed to a MessagePackEncoder and encodeToMessagePackis called, first a new _MessagePackEncoder is constructed and then value.encode(to:) is called. Not .encode(to:) of Data is called. This does not encode Data again (as String, Bool, Int, etc. do), so in this case the encode of one of the containers is never called with Data. encode(to:) of Data request an unkeyed container and encodes a list of UInt8 values (I don't know how this implementation actually looks, but this is what it does in the end). My approach to fix this is, not to call value.encode(to:), but create a internal function that encodes and does the stuff that is done in all three containers (By the way, I think there is quite massive code duplication in the implementation that could be avoided with more "shared" global functions), checking for Data, converting it to msgpack, or calling .encode(to:) if the value is something else.

cherrywoods avatar Mar 28 '18 15:03 cherrywoods

Anyway great implementation @andrew-eng!

cherrywoods avatar Mar 28 '18 15:03 cherrywoods

What's the status of this?

grosch avatar Jul 02 '18 04:07 grosch

@a2 what's blocking this PR? I can help if needed

pheki avatar Mar 05 '20 22:03 pheki

What's the status of this? This would be a great addition!

gsabran avatar Oct 07 '21 01:10 gsabran

Any news on this?

enrico-querci avatar Feb 11 '22 10:02 enrico-querci