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

XML Prolog support #34

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

XML Prolog support #34

wants to merge 4 commits into from

Conversation

laubstein
Copy link
Contributor

This commit fixes #33 .

The code supports only version, encoding and standalone xml prolog attributes (here).
The new special attribute (xmlProlog) is using the default attrPrefix, is this ok?

I have added two new configs:

forceXmlProlog: false,
defaultXmlProlog: '<?xml version="1.0" encoding="UTF-8"?>'

Unit test

  describe('xml prolog', function() {
    it('should be consistent', function() {
      var originalString = '<?xml version="1.0"?><a>1</a>';
      var xmlObject = JXON.stringToXml(originalString);
      var jsonFromXml = JXON.xmlToJs(xmlObject);
      var jsonFromString = JXON.stringToJs(originalString);

      assert.equal(originalString, JXON.jsToString(jsonFromXml));
      assert.equal(originalString, JXON.jsToString(jsonFromString));

      // PhantomJS do not serialize the prolog, so we are forcing a default
      JXON.config({
        forceXmlProlog: true,
        defaultXmlProlog: '<?xml version="1.0"?>'
      });
      assert.equal(originalString, JXON.xmlToString(xmlObject));
    });
  });

The new JSON representation from

<?xml version="1.0"?>
<a>foo</a>

will be...

{
    "a": "foo",
    "$xmlProlog": {
        "version":"1.0"
    }
}

};
var aCache = [];
var rIsNull = /^\s*$/;
var rIsBool = /^(?:true|false)$/i;
var rXmlProlog = /^(<\?xml.*?\?>[\n]?)/;
var rXmlPrologAttributes = /\b(version|encoding|standalone)="([^"]+?)"/g;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these attributes could also be quoted in single quotes ' according to the specs

Copy link
Contributor Author

@laubstein laubstein Aug 17, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My mistake... that's easy to fix.

@tyrasd
Copy link
Owner

tyrasd commented Aug 17, 2016

supports only version, encoding and standalone

that's ok, because these are the only three allowed pseudo-attributes of the xml declaration (see https://www.w3.org/TR/2004/REC-xml-20040204/#sec-prolog-dtd)

using the default attrPrefix

Yeah, that's fine, because the top-most level in the JXON representation cannot currently have any attributes, as it would reference the "parent" of the xml root element. The analogy that this should apply to the pseudo-attributes of the xml document suggests itself.

But I think your current implementation is a bit over-complicated, if you ask me. What do you think about the following proposals:

  • I think the more commonly used name for the xml prolog is xml declaration, let's use a nomenclature that's a bit closer to that.
  • You're adding a couple of helpers only to parse something that's than immediately stringified again. What if we screw all that and just save everything that matches the xml declaration regexp into a string (i.e. as if it were a full xml node)? That would save us a lot of code and for also most (or all?) of the problems I've highlighted in the PR.
  • make forceXmlProlog default to true.

@laubstein
Copy link
Contributor Author

I will try to fix all points and them submit a new commit. 👍

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.

XML Prolog inconsistence
2 participants