Fix stopNodes to work with removeNSPrefix (#607) #608
Conversation
candrews
force-pushed
the
stopNodes-removeNSPrefix
branch
from
89130cd to
5574156
Compare
|
Thanks for the PR. I'll check it ASAP |
src/xmlparser/OrderedObjParser.js
Outdated
| @@ -280,6 +280,7 @@ const parseXml = function(xmlData) { | |||
| }else {//Opening tag | |||
| let result = readTagExp(xmlData,i, this.options.removeNSPrefix); | |||
| let tagName= result.tagName; | |||
| const tagNameWithNamespace = result.tagNamespace ? result.tagNamespace + ":" + result.tagName : result.tagName; | |||
There was a problem hiding this comment.
This variable name gives impression that it always has namespace in it. I believe, it should be se to tagName. The original tagName variable can be renamed accordingly. Moreover, it would be good if this code is present in readTagExp so reduce an extra operation of comparison and concatenation.
There was a problem hiding this comment.
The original tagName variable can be renamed accordingly.
Pehaps the new name for that variable should be tagNameWithoutNamespace
Moreover, it would be good if this code is present in readTagExp so reduce an extra operation of comparison and concatenation.
I'll see what I can do
There was a problem hiding this comment.
I've updated this PR, and I think the approach make sense and hopefully addresses your concerns.
tagName is the tag name. If removeNSPrefix, then tagName never has a namespace; otherwise, it will have a namespace if there is a namespace.
tagNameWithNamespace is the tag name with namespace if there is a namespace. It's value doesn't depend on removeNSPrefix.
The work is done in readTagExp and it avoids the additional comparison / concatenation.
There was a problem hiding this comment.
So you mean tagNameWithNamespace is actually originalTagName or rawTagName which may or may not have name space?
There was a problem hiding this comment.
Yes, tagNameWithNamespace is the tag with a namespace (if it has a namespace).
I could use other names for that variable, such as originalTagName or rawTagName - whatever you'd prefer.
As they say, naming things is one the hard problems of computer science
There was a problem hiding this comment.
I completely agree and I faced it. But ...WithNameSpace is misleading that's why I'm highlighting it. I think anyone from originalTagName or rawTagName would be better. Both define the purpose.
There was a problem hiding this comment.
I've updated this PR to use rawTagName as the name for this variable :)
Is there anything else I can do to help get this PR merged? I'm quite eager to use this fix in my project
candrews
force-pushed
the
stopNodes-removeNSPrefix
branch
2 times, most recently
from
4cb109c to
922b51f
Compare
Signed-off-by: Craig Andrews <candrews@integralblue.com>
candrews
force-pushed
the
stopNodes-removeNSPrefix
branch
from
922b51f to
37dfbe2
Compare
|
Thanks for your PR. However, this may take little time to publish as I would like to provide a single publish that includes some open bugs. |
|
I'm really eager to be able to use this bug fix. What open bugs are blocking the next release? Maybe I could help address them to move things along. |
Purpose / Goal
The
stopNodesoption currently doesn't work with theremoteNSPrefixoption. This pull request fixes that issue and adds tests to demonstrate the fix.Type
Please mention the type of PR