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

Test Decoding of Nested Arrays of Enums #126

Merged
merged 6 commits into from Oct 4, 2019

Conversation

bwetherfield
Copy link
Collaborator

@bwetherfield bwetherfield commented Aug 22, 2019

I believe the structure in issue #122 provides the right use case for a nestedUnkeyedContainer (for key Chapters). As such I add a test case with the full proposed usage and a fix for XMLKeyedDecodingContainer.nestedUnkeyedContainer on the implementation side that allows the decoding of wrapped arrays of enum types.

The key lines on the user-side are within the Decodable implementation for Book, with decoding initializer using nesting syntax as follows:

extension Book: Decodable {
    enum CodingKeys: String, CodingKey {
        case title
        case chapters
    }

    init(from decoder: Decoder) throws {
        let container = try decoder.container(keyedBy: CodingKeys.self)
        title = try container.decode(String.self, forKey: .title)

        var chapters = [Chapter]()

        if var chapterContainer = try? container.nestedUnkeyedContainer(forKey: .chapters) {
            while !chapterContainer.isAtEnd {
                chapters.append(try chapterContainer.decode(Chapter.self))
            }
        }

        self.chapters = chapters
    }
}

Book is defined as follows, wrapping an array of the enum type Chapter.

struct Book: Decodable {
    let title: String
    let chapters: [Chapter]

    init(title: String, chapters: [Chapter]) {
        self.title = title
        self.chapters = chapters
    }
}

More info on how the enum type gets decoded from XML choice elements can be found in #119.

@bwetherfield
Copy link
Collaborator Author

@MaxDesiatov, were you able to take a look at this? Thanks!

@MaxDesiatov MaxDesiatov self-assigned this Sep 25, 2019
@MaxDesiatov
Copy link
Collaborator

@bwetherfield I'm sorry for the delay, it's in my queue now!

@MaxDesiatov
Copy link
Collaborator

MaxDesiatov commented Sep 26, 2019

Hi @bwetherfield, I had a quick look and I'm a bit confused by the test case decoding initializer. At a first glance it looks like exactly something that auto-synthesized initializer would do, but if I remove it (init(from decoder: Decoder) on private struct Book to be exact), the synthesized one decodes only one element. Do you have any thoughts on why that happens? I wonder if achieving the expected behavior with synthesized initializers is possible at all, which would be ideal, wouldn't it?

@bwetherfield
Copy link
Collaborator Author

Hi @MaxDesiatov ! From what I understand the nestedUnkeyedContainer command only gets called by a user in Decodable usage, so I think that would mean that we would need to specify it in a custom initializer. I think that the default initializer does not add any nested structure, which causes the error from #122. My take is that the fix has to be a combination of usage and implementation... Basically I think having the intermediate <chapters> nesting is non-standard so the decoder needs to be told what to do (with a call to nestedUnkeyedContainer). What do you think?

@MaxDesiatov
Copy link
Collaborator

Hey @bwetherfield, I again apologize for the delay, August and September turned out to be months with some crazy workload.

It looks like the easiest fix is to add an intermediate Chapters struct, with a trivial decoder initializer that calls singleValueContainer(), similar to what we did in NestedChoiceTests. This doesn't require a more complex initializer that iterates over all of the items in a manually created unkeyed container and doesn't require changes in the library.

I'm going to merge the PR when CI passes, but if there's anything else missing, please feel free to submit new issues/PRs.

@MaxDesiatov MaxDesiatov merged commit 149aafe into CoreOffice:master Oct 4, 2019
@bwetherfield
Copy link
Collaborator Author

bwetherfield commented Oct 5, 2019 via email

bwetherfield added a commit to bwetherfield/XMLCoder that referenced this pull request Oct 16, 2019
Add `NestedChoiceArrayTest` based on the code shared in CoreOffice#122.
@MaxDesiatov MaxDesiatov changed the title Fix Decoding of Nested Arrays of Enums Test Decoding of Nested Arrays of Enums Oct 17, 2019
arjungupta0107 pushed a commit to salido/XMLCoder that referenced this pull request Jun 26, 2020
Add `NestedChoiceArrayTest` based on the code shared in CoreOffice#122.
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

2 participants