Skip to content

Support Codable without implementing the full Encoder and Decoder #63

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

Closed
wants to merge 10 commits into from
Closed

Support Codable without implementing the full Encoder and Decoder #63

wants to merge 10 commits into from

Conversation

cherrywoods
Copy link
Contributor

Hey,
I don't know what's up on the other pull request about this topic, but I would like to submit another approach to this.

This implementation also supports Codable without copying all the code of JSONEncoder (directly). However it adds a dependency to the project: it depends on MetaSerialization (https://github.com/cherrywoods/swift-meta-serialization) that implements the Encoder and Decoder and KeyedEncodingContainer, and so on.

The code for adding this support in this repository is pretty short (but it's a bit ugly, because it's 70% type checking code), for the work it does.

The other thing I'd liked to suggest is providing such an interface to encode and decode Codable types to serialise to MessagePackValue, instead then to Data. This would allow to mix both ways of creating msg pack with the framework, by e.g. encoding some custom type and then using the returned MessagePackValue in another (array, e.g.) MessagePackValue. The fork also contains code to do this the other way around, for example having a MessagePackValue property and encoding this one, so it is embedded into the other MessagePackValues that are created on converting this custom type.

Anyway, I don't know if it would be better to create another repository for this project, or to merge it into yours.

@cherrywoods
Copy link
Contributor Author

Well however, now also three tests for the new CodableConverter are included.
The test run fine on my computer...

One of the included tests checks, whether the new class performs implicit loosely conversions from Double to Float. (It does) This behaviour can easily be changed to the opposite, because the implementation uses the floatValue computed property of MessagePackValue. If this implementation is changed from Float(_) to Float(exactly:), but changing this makes another test fail. The implementation in MessagePackValueTranslator can as easily be changed, I suppose.

Waiting for comments on this...

@a2
Copy link
Owner

a2 commented Feb 22, 2018

Hey @cherrywoods. I love the idea of using a "universal" Codable solution but I'm hesitant to bring a dependency into MessagePack.swift. Good catch on the implicit lossy conversion between floating point values. Merged #62 to address this.

@cherrywoods
Copy link
Contributor Author

Thanks. I do understand your hesitation. I do also think, that it would be advantageous to provide this functionality as a separate framework.

@a2
Copy link
Owner

a2 commented Feb 23, 2018

@cherrywoods Agreed

@cherrywoods
Copy link
Contributor Author

Ok. I'll wait for andrew-eng's suggestion and create a new repository then if necessary. (but I already have a repository that partially supports this, if someone is out there is watching us and needs this functionality right now to get a spaceship flying 🚀😄)

@a2
Copy link
Owner

a2 commented Feb 28, 2018

Going to close this PR since I think we agree that, although this PR achieves the goals of adding Codable support to MessagePack.swift, it isn't exactly what the project needs right now.

@a2 a2 closed this Feb 28, 2018
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.

2 participants