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

Added Codable support #66

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

Conversation

andrew-eng
Copy link

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
Copy link
Contributor

cherrywoods commented Mar 28, 2018

  • 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
Copy link
Contributor

cherrywoods commented Mar 28, 2018

Anyway great implementation @andrew-eng!

@grosch
Copy link

grosch commented Jul 2, 2018

What's the status of this?

@pheki
Copy link

pheki commented Mar 5, 2020

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

@gsabran
Copy link

gsabran commented Oct 7, 2021

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

@enrico-querci
Copy link

Any news on this?

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

6 participants