CoreOffice / XMLCoder

Easy XML parsing using Codable protocols in Swift
https://coreoffice.github.io/XMLCoder/
MIT License
795 stars 107 forks source link

Implement @Element and @Attribute property wrappers #189

Open MaxDesiatov opened 4 years ago

MaxDesiatov commented 4 years ago

Property wrappers could be a great fit to replace the DynamicNodeEncoding API. E.g.

struct Tag: Codable {
  @Element var element: String
  @Attribute var attribute: String
}

would be encoded as <Tag attribute="foo"><element>bar</element></Tag>.

There's some bike-shedding needed here with the names, I thought about XMLElement instead of Element, on the other hand it would be easy to disambiguate as XMLCoder.Element if needed. Same with XMLAttribute vs Attribute.

bwetherfield commented 4 years ago

I've been playing around with this idea. So far this test case seems especially problematic: https://github.com/MaxDesiatov/XMLCoder/blob/d502e63169cafca8490d9a2571255918e8949ef0/Tests/XMLCoderTests/AdvancedFeatures/AttributedEnumIntrinsicTest.swift#L47-L56

If you look at the rest of the tested type definitions, only the string/int label with key "type" is supposed to be an attribute. Essentially, the fact that this is an attribute has to be decided at the coding key (rather than property) level as far as I can tell. What do you think, @MaxDesiatov ? Can you think of a way around this?

MaxDesiatov commented 4 years ago

Thanks @bwetherfield, do you have this in a branch to check out? I think I need a closer look to form an opinion.

bwetherfield commented 4 years ago

Just created a PR with a very experimental branch! Basic dynamic encoding of attributes and elements is working. Still have to deal with node encoding strategies (owned by XMLEncoder) objects, as well as decoding and the above test case!

MaxDesiatov commented 3 years ago

Reopening this as a reminder to add basic docs for this to README.md.

wooj2 commented 3 years ago

Hi @MaxDesiatov !

This feature seems kind of cool, but I was trying to use it with a map, and ran into some trouble.

  1. Is it possible to use the @Attribute and @Element property wrappers with maps and/or lists? For example, uncommenting the two lines in the example below seem to give me two compilation errors:
class Cookbook: Codable {
    @Attribute var description: String?
    @Element   var language: String?
    //@Element var glossary: [String:[String]]? = nil

    private enum CodingKeys: String, CodingKey {
        case description = "cookbookDescription"
        case language
        //case glossary
    }
}
  1. Based on this comment it sounds like DynamicNodeEncoding is slated for deprecation. Is this still the plan, or will this protocol stick around for a while? Any chance you have any thoughts/updates about this?
MaxDesiatov commented 3 years ago

@wooj2 do you have to keep Cookbook a class instead of a struct? The problem I guess is caused by the fact that classes don't get implicit memberwise initializers.

As for DynamicNodeEncoding deprecation, when I'm 100% sure that @Attribute and @Element are able to cover all cases that DynamicNodeEncoding covers, I'd like to deprecate the latter. Providing two different ways to achieve the same thing is a maintenance burden for us, and also not ideal for users who get a more complex library because of that. But I don't have that 100% certainty in use case coverage.

If you can make your code work with @Attribute and @Element, then I'd recommend sticking to that, otherwise I'm happy to look into this in more details.

kneekey23 commented 3 years ago

@MaxDesiatov how can the property wrappers handle arbitrarily nested dictionaries that have attributes on them?

MaxDesiatov commented 3 years ago

Can you elaborate please? What XML are you trying to decode and how would you like your model types to look?

kneekey23 commented 3 years ago

In our codebase, we need to be able to write custom implementations for encode and decode. It does not compile when we try

import XMLCoder

public struct Book1 {
    @Attribute var attr: String?
    @Element   var foo: String?
}

extension Book1: Reflection, Codable{
    private enum CodingKeys: String, CodingKey {
        case attr = "testCustomKeyValue"
        case foo
    }

    public func encode(to: Encoder) throws {
        var container = to.container(keyedBy: CodingKeys.self)
        try container.encode(attr, forKey: .attr)
        try container.encode(foo, forKey: .foo)
    }

    public init(from decoder: Decoder) throws {
        let values = try decoder.container(keyedBy: CodingKeys.self)

        let attrDecoded = try values.decodeIfPresent(String.self, forKey: .attr)
        attr = attrDecoded
        let fooDecoded = try values.decodeIfPresent(String.self, forKey: .foo)
        foo = fooDecoded
    }
}

This code does not compile and we should be also able to set everything to nil if we wanted to in the decoder as well.

Screen Shot 2021-03-03 at 11 50 20 AM

We can do this with DynamicNodeEncoding just fine but not the new property wrappers. Please advise.

MaxDesiatov commented 3 years ago

Thanks for the detailed example! I'll need some time to have a closer look at this and come up with a solution. I don't have a good estimate for this right now, but I can say for sure that I won't deprecate anything until cases like this are covered and we have a clear migration path.