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
[WIP / Proof of concept] Add Decoding support for enums with associated values #113
Conversation
…field/decoding-enums
Hi @jsbean, thank you for having a closer look into this! In my previous attempts to implement this, I came to the conclusion that this needs a public API change. It seems that the approach with intrinsic coding keys could work. Similarly to what we have with a The reason is that the decoder of the inner value needs to know the name of the element to be able to switch on it and then decode an appropriate enum case. I'm very interested to know if you were able to make this stuff work without any public API changes or new intrinsic coding keys? |
Having a closer look at this PR, I'm not sure this is an ideal construct: do {
self = .int(try container.decode(IntWrapper.self, forKey: .int))
} catch {
self = .string(try container.decode(StringWrapper.self, forKey: .string))
} This looks ok just for 2 enum cases, but say you have 5 of them and you get a pyramid of doom of nested switch xmlElementName {
case "int":
self = .int(try container.decode(IntWrapper.self, forKey: .int))
case "string"
self = .string(try container.decode(StringWrapper.self, forKey: .string))
} This also potentially could be more performant, since you wouldn't need to create a new decoding container and try the next one if that fails. |
BTW, I had a test for this in #106, but that was blocked for a while by the changes I had to merge for ordered containers. Now that's done, adding this element name intrinsic shouldn't be that hard. |
Thanks for lookin' over! Yup, this is all done internally with no public API change. I very much agree that the While this doesn't scale well from a syntactical perspective, the implementation of There are a couple downsides that I see to the First, it requires the user to add the Second, while switching over the I hear your concerns about performance, though my initial goal is to mirror the patterns already used in JSON coding from a logical perspective. My intuition is that improvements could be made internally to this approach which would improve performance, and perhaps some optimizations could be made within the Would it be worthwhile for me to add some benchmark tests to see where we are from a performance perspective? I am definitely not tied to this implementation, though I would prefer a solution that does not require Looking forward to hearing your thoughts! |
All good points, and I don't think any of your work is conflicting with my I don't think any additional benchmarks are needed, although it would be interesting how the existing benchmarks behave on your branch when compared to |
A few things are on my the roadmap before I would consider this worthy for merge:
|
(I also do want to point out that the test suite has been immensely helpful throughout this process. I am glad that effort was put in by you all to make it feel solid.) |
Yes, definitely. Initially, I found ways of breaking the Are the baselines for the benchmark tests that get shipped around with the |
those do get shipped, but they're tied to a Mac model, i.e. its CPU core count, memory etc, so you probably need to run |
…field/decoding-enums
@MaxDesiatov, I am going to close this WIP branch for a bit. In working on the encode side of things, we found some ambiguities. We think we might have found a nice solution that would address with encode and decode sides in a more elegant fashion. Will come back with a PR shortly. |
This PR is an initial step towards adding support for Decoding union-type–like enums-with-associated-values (as presented in #25 and #91).
There is a single regression test which is currently failing (
NestingTests.testDecodeUnkeyedWithinUnkeyed()
), though it doesn't seem out of reach to fix it.I have added test cases for both #25 and #91, which are passing. For the case of #91, I haven't had success decoding the empty
Break
type.The changes here seem both a bit magical as well as grotesque, but it seems like it is at a good place to stop and talk about further directions!
There are three changes made to the source here:
XMLCoderElement.transformToBoxTree()
, a special case is made to prevent enums-with-associated-value–like types getting represented as attributed unkeyed values.XMLDecoderImplementation.unkeyedContainer()
, keyed boxes get transformed into unkeyed boxes wherein each key-value pair from the original is packaged up into its ownKeyedBox
carrying a single element representing the key and value. This allows the key to be retrievable downstream in theXMLUnkeyedDecodingContainer
.XMLUnkeyedDecodingContainer.decode(...)
, the container packaged up byXMLDecoderImplementation.unkeyedContainer()
gets unpackaged in the case it may hold the contents of an enum-with-associated-value.