Fix for pull request #794 #797
Conversation
…etween Java and C
…ied. Details below. This patch changes the behavior of node.content to recursively aggregate text from children nodes, unless this is a Text node in which case the content is returned. In order to support recursively appending contents of children elements and to fix the broken append behavior demonstrated by the test in the previous commit, I had to remove the caching (which I'm not sure why it was there in the first place) Also since we recursively append text, we'll have to invalidate the cache when changes deeper in the document tree occur. Also I removed the content() from XML::Text since we don't need it anymore.
This solution is much cleaner, but may not be as efficient. This can be reverted to the previous commit which was pure Java if there are issues with this implementation.
|
@jvshahid I have a time to work on this weekend. So, I'll have a look on Thursday or so. Give me a couple of days. |
|
Tagging myself here for updates. |
|
@jvshahid That content caching is mostly from a historical reason. When I started working, the caching was already there. It confused me a while, also it didn't work perfectly. I fixed those as much as possible. I tried your fix and confirmed the fix worked well. So, I think we don't need caching anymore. I know sometime fix in Ruby side is much simpler and cleaner. But, I've tried to avoid since I want to keep Ruby code compatibility with CRuby version. Honestly, I'm not sure what direction we should go. Also, content method has a problem reported in #521 . This one is hard to fix. Just stripping out all whitespaces causes many failures in other tests. |
|
Thanks @yokolet for the review. I'll let you decide which version to merge on master. I don't really care. Regarding #521, it looks like the newlines are actually inserted in the |
…st case from pull request #794.
|
I merged Java version of fix and pushed to the master. Sometime around Feb or March this year, I've talked with other Nokogiri committers about JRuby specific Ruby code. The conclusion then was that's ok for test cases, but others needed to think about. I did write JRuby specific Ruby code because implementation only in Java would be so hard and weird. I think we should bring this topic on the table again. |
|
I'm going to close this issue. |
Hi @yokolet,
This is a fix for pull request #794. Can you please eyeball the changes. Most importantly I've removed the content caching from XmlNode.java. Initially I implemented the fix in Java (commit 1928899) but later found it much cleaner to do it in Ruby (commit 96f91eb).
Commits 31dd404 and d556f06 include the two test cases that I was trying to fix.
Do you remember why caching was there ? Is it going to break anything ?
Thanks
-JS