Skip to content

Conversation

@DanielAsher
Copy link
Contributor

and add "xml parsing - should be able to iterate over mixed content" test

implements #82

…d be able to iterate over mixed content" test
@drmohundro
Copy link
Owner

Hey @DanielAsher, thanks for the PR!

Quick request for comment... what are your thoughts on this children property (off of XMLElement) versus the children property off of XMLIndexer?

The problem of course is that XMLIndexer's children property only enumerates the xmlChildren (see SWXMLHash.swift#L393 in your branch) and changing the behavior of the existing public property could break existing code.

I'm not opposed to making that property public, but I just wanted to get some feedback on this before merging since you've run into it already.

@DanielAsher
Copy link
Contributor Author

DanielAsher commented Jul 11, 2016

I came to the same conclusion and believe this PR to have minimal impact. It allows the framework to offer the good ordering of mixed-content xml.

Longer term, replacing the XMLElement and TextElement subclasses with an enum, or perhaps extending the XMLIndexer, are possible ways to support this unique facility.

@drmohundro: thanks for a great framework.

@gca3020
Copy link
Contributor

gca3020 commented Jul 11, 2016

Has there been any thought of combining the XMLElement and XMLIndexer into a single class? It was confusing when extending my own classes with the XMLIndexerDeserializable and XMLElementDeserializable protocols to determine exactly when a child would be an Element vs. an Indexer.

You have done an admirable job of not breaking API compatibility, but there will be a pretty major breaking change coming with Swift 3 I assume, so it might be something to look in to at that point.

@drmohundro drmohundro merged commit 2da96a0 into drmohundro:master Jul 12, 2016
@drmohundro
Copy link
Owner

I pushed 2.4.0 to support this!

Thanks for the thoughts @DanielAsher and @gca3020 - I created #84 to track discussion around version 3.0.0 that will come out with Swift 3.0. I'd welcome any additional thoughts or suggestions there!

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.

3 participants