a2 / MessagePack.swift

It's like JSON, but fast and small…and Swift! – msgpack.org[Swift]
http://msgpack.org
MIT License
283 stars 60 forks source link

Added Codable support #66

Open andrew-eng opened 6 years ago

andrew-eng commented 6 years ago

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.

cherrywoods commented 6 years ago

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 commented 6 years ago

Anyway great implementation @andrew-eng!

grosch commented 6 years ago

What's the status of this?

pheki commented 4 years ago

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

gsabran commented 3 years ago

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

enrico-querci commented 2 years ago

Any news on this?