-
Notifications
You must be signed in to change notification settings - Fork 203
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
Adds subrange indexer for XMLElement #174
Adds subrange indexer for XMLElement #174
Conversation
Thanks! I like your means of having a builder method to create the In the meantime, could you look into getting a unit test added? |
I'm just hypothesizing at the moment... trying to consider the best and most flexible design for this. Given that, take all of this with a grain of salt.. I'm hesitant about the filtering being strictly index based, because I could see a filter working based on child elements as well. That leads me to think about possibly passing in a callback function... something like this based on your test: let element: XMLElement = xml!["root"]["catalog"]["book"][1].element!
let subIndexer: XMLIndexer = try! element.filteredIndexer { elem in
let subrangeChildren: [XMLContent] = Array(elem.xmlChildren[1...3])
let elem = XMLElement(name: name, index: index, options: options, children: subrangeChildren, allAttributes: allAttributes)
return XMLIndexer.element(elem)
} In this case, public func filteredIndexer(_ filter: (XMLElement) -> XMLIndexer) -> XMLIndexer { } That means that the filtering is customizable from the caller versus either 1) forcing it to be index-based or 2) having multiple overloaded methods to handle filtering. Does that seem at all reasonable to you? Or am I just overcomplicating things? 😅 |
I'm not sure I follow everything you proposed. Generally I'm open to a more 'generic' approach, for now there're only two options for filtering the new indexer:
You're proposing now, to avoid creating specific functions / arguments for these and (!) upcoming options, but instead using a filter specified by a callback. Okay, seem reasonable, even though it's not necessary for our usecase I see the advantage in the long run. In the end it would then makes sense to somehow add this method to the XMLIndexer itself and then rename it to filter. Then we should mimic the behaviour from Swift's own filter's on CollectionTypes, this would keep it consistent (like with the userInfo #171). I'm willing to take a look at that - if I understood everything correctly ;-) |
Yes, you're correct - following something like the filter on CollectionTypes approach sounds like what I was thinking. I'm happy to help throw some code or gists up, too, if that would help! |
Okay, I created now two filter methods. I tried including the filter by indexes in one and the same filter method. Even though this is possible it's not really useful. Imagine our specific usecase: We like to create a (Sub)XMLIndexer based on all XMLElements (filter 1) and a specific range of indexes (filter 2). Doing this in one call is technical possible, but then the user would have to know how the indexes are going to change due to the applying of filter 1. I tried messing / updating the indexes internally, but that makes things quite complicated. |
Sorry for the long delay on responses - I've been traveling the past few days. Let me see if I can play around with the tests in your PR to see if I can come up with any other possible filter implementations that might be more flexible. If not, no big deal. And again, thanks for your suggestions and patience! :) |
@LordNali - I just created a branch to try to experiment with this a little bit... see c3406cf and https://github.com/drmohundro/SWXMLHash/compare/subindexer-suggestions. The main things I did were:
I left your tests mostly as-is aside from moving the call to the indexer. You can also see an additional test I added as I was thinking that the Does it 1) make sense and 2) cover your specific use case, too? |
Yes and yes - your proposal is indeed much more cleaner and powerful, so I would certainly go for it. |
Codecov Report
@@ Coverage Diff @@
## master #174 +/- ##
==========================================
+ Coverage 27.13% 27.23% +0.09%
==========================================
Files 13 13
Lines 2034 2122 +88
==========================================
+ Hits 552 578 +26
- Misses 1482 1544 +62
Continue to review full report at Codecov.
|
I just pushed |
Hmm, I just tried using the new version / filter. The filter seem to work just fine (I printed the new indexers It seems that our custom |
Hmm... that is strange. I'll have to check that and see if I can figure out what is going on. I'll see if I can get some test cases in place to reproduce the behavior. |
See discussion in #174. The issue was related to whether or not filter should return a list or an element... that should depend on if a single element was filtered to or not.
@LordNali I was able to reproduce it. I've got a fix in place with a test... see #177. I've pushed out (as an aside, the issue was related to how the |
I just tried to new version, but still the same issue. We're filtering only by index not by specific elements) and then using |
Do you think you could get a repro set up that I could look at? Or a failing test? If we can only reproduce it given your XML and the data is sensitive, then if you'd be able to shoot me a zip of a repro instead? I think we're close here, but I'm just not getting the exact set up locally I guess. |
Okay, I narrowed it a bit down - this is the method that fails:
And the reason is the provided Maybe I understood the behaviour of the deserializer wrong, but then again, with the previous subIndexer implementation our code did work without any problems. |
In #174, there appears to still be some cases where filter doesn't seem to work as expected. Hopefully these tests will help iron out some other possibilities.
Well, it seems odd to me that there would ever be a case where the I just added 5460d01 to cover a few more cases... it has some Do you see anything, particularly in the deserialization of I feel like we're getting closer. |
In #174, there appears to still be some cases where filter doesn't seem to work as expected. Hopefully these tests will help iron out some other possibilities.
Oh that was my fault - I updated the method that fails. It's indeed But still it fails due to the fact, that it's called by a I integrated SWXMLHash now from source instead as framework, you make debugging much easier (for now). |
It depends on which path it falls into based on what the target deserialization type is. So, for example, if you're deserializing into an array of objects (i.e. When your call to let foo: [Stuff] = xml["stuff"].filter { }.value() Meaning, it should deserialize to a list of |
We're trying to read one (1) single object - so the call should be redirected to the The problem is the fact, that the new indexer from the new filter method doesn't return a single I understand or at least find in somehow logic, that the Is there now way to imitate that behaviour in the new filter method? |
Just a quick note to explain why I'm a little hesitant to add additional functionality to So, the new filter method returns a list if more than one element is found - it checks the count of the elements currently indexed and, if it is 1, it returns This code below works for me: <root>
<arrayOfGoodBasicItems>
<basicItem id="1234a">
<name>item 1</name>
<price>1</price>
</basicItem>
<basicItem id="1234b">
<name>item 2</name>
<price>2</price>
</basicItem>
<basicItem id="1234c">
<name>item 3</name>
<price>3</price>
</basicItem>
</arrayOfGoodBasicItems>
</root> let subParser = parser!["root"]["arrayOfGoodBasicItems"].filter { _, idx in idx == 0 }
let value: BasicItem = try subParser.value() That said, if I change the filter method to look for more than one index (e.g. Here's a question... are you expecting the For example: parser!["root"]["arrayOfGoodBasicItems"].filter { _, idx in idx == 0 } vs. parser!["root"]["arrayOfGoodBasicItems"]["basicItems"].filter { _, idx in idx == 0 } Which one makes more sense to you? |
Okay, that's a detailed explanation and helps at lot. Our issue is certainly the actual style of our XML (I mentioned it in the beginning and unfortunately we can't change the XML input). Our XML look likes this - to keep working with your BasicItem Example:
The problem is the fact, that the The actual question is how to handle this? Is this a task for the
Some might argue this is cleary a problem of the input |
I totally hear you - blaming the XML is valid and can't be changed... if we could blame the XML and have it fixed, we wouldn't still be dealing with SOAP 😄 So, I was thinking about this and I think the solution is to have two filter methods:
Given the above implementations, I think you could solve it like this: // NOTE: using filterAll instead of filterChildren
let subParser = xml["root"].filterAll { _, idx in idx >= 1 && idx <= 3 }
let basicItem: BasicItem = subParser["basicItem"].value()
// and so on... I'll see if I can get this in a branch. if this works, I'll just deprecate the existing |
I just opened #178 that will hopefully resolve this. Please take a look at it... I'll hold off on merging until you get a chance to check it out. In hindsight, my original implementation was definitely off - it wasn't even consistent in whether it filtered against |
I’ll check this out next week. Thanks a lot for the new PR. |
Okay, so I tried #178 - but IMHO this doesn't address our actual issue. I'm using now the new method But then we're trying to call I'm not 100 % sure we're using the provided methods in the right way. We ned a way of filtering the all / children property of an |
I'm going to describe to you what I'm understanding and you tell me if I'm stating it correctly. <root>
<...>
<basicItem id="1234a">
<name>item 1</name>
<price>1</price>
<...>
</root> You want to deserialize the // stay on "root", don't "index" down
let result = xml["root"].filterChildren({ _, idx in idx >= 1 && idx <= 3 }).value()
// result above would only have <root><basicItem/><name/><price/></root>?
// ... versus what it does today which is having <basicItem/><name/></price>? So, given the above, this would STILL remain on the "root" element, though? It wouldn't index down to be siblings with "basicItem", "name" and "price"? Am I following? |
I think it's a bit more complicated then that:
Our XML contains here the actual root element and inside the root element (a list, but for this example only one) additional element, this element belongs like root property 1 - 3 to root, but is not of primitive type (e.g. Int or String), but of a custom type (here Class) for which we wrote an own deserializer. What the filter then should in the end archive is the creation of a indexer that looks like this:
So the same |
Okay, here's how it works now. Instead of Given the latest commit in #178, I put the following in a Swift playground to play with: let xmlToParse = """
<root>
<some-weird-element />
<another-weird-element />
<prop1 name="prop1" />
<prop2 name="prop2" />
<basicItem id="1234a">
<name>item 1</name>
<price>1</price>
</basicItem>
<prop3 name="prop3" />
<last-weird-element />
</root>
"""
let parser = SWXMLHash.parse(xmlToParse)
let subParser = parser["root"].filterChildren { _, index in index >= 2 && index <= 5 }
print("non-filtered")
print(parser)
print("\nfiltered")
print(subParser) It outputs this:
I think this more closely aligns with what you're looking for. The only caveat I can think of is that this excludes any non-XML content, mainly like whitespace. That's why the filtered output isn't lined up the same. |
Could this be an issue - in the future? I don't want you to introduce something only for our usecase, that mess things up. I just tested the new commit, this time it works perfectly for our XML. |
Nah, I'm not too worried about the whitespace issue... the only case that I'm aware of that people are explicitly interested in it is related to displaying XML, usually in terms of attempts to parse HTML. Also, thanks for the feedback and working with me through this - I'll see if I can get this version pushed out soon! |
|
First draft for a subrange XMLIndexer based on an XMLElement - see #170