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

make XMLElement children property public #83

Merged

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