this isit #1604
this isit #1604
Conversation
... now contains ~ same code as DTMManagerDefault
we can not override the class since the state we're into is private we have to move it into the apache pkg due pkg-private constructor : DOM2DTMdefaultNamespaceDeclarationNode
`RuntimeException: Could not resolve the node to a handle`
... not needed for Nokogiri but its not clear what else it might affect
resolves sparklemotion#1319 (test borrowed from @jkraemer) adjusted test for sparklemotion#1320 to be a little more complex
|
Wow! Thanks so much for submitting this. Looking at everything now ... |
| @@ -293,7 +293,11 @@ def == other | |||
| # Returns a new NodeSet containing all the children of all the nodes in | |||
| # the NodeSet | |||
| def children | |||
| inject(NodeSet.new(document)) { |set, node| set += node.children } | |||
There was a problem hiding this comment.
I'm not sure I understand why this change is being made. Is it for performance? If so, can you share your benchmarks?
I'm asking only because it seems out of place in this patchset.
There was a problem hiding this comment.
shouldn't be necessary.
wanted to make sure only a single NodeSet is created (as I was debugging how the whole piece behaves with decorators), than I left it in as it seemed to make sense but I do not have benchmarks.
it "must" be faster :) but you're right this should have been 2 PRs I just 💅 as I went with the fixes.
on my defense - its uglier but similar to the def reverse code bellow 😉
I am also fine with moving it out ... if you prefer to have it separately?
There was a problem hiding this comment.
Nah, it's fine, I was just curious.
|
Other than the one question I asked above, this looks great. |
|
Tagging flavorjones/loofah#88 as that is definitely addressed by this patchset. |
@flavorjones #1319 and #1320 at your service!
these are really the 'simplest' thing that works ...
if there's more of these hacks needed maybe switching to another xpath engine should be considered.