Skip to content

Add CDATA support #175

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

Merged
merged 2 commits into from
Jun 21, 2017
Merged

Add CDATA support #175

merged 2 commits into from
Jun 21, 2017

Conversation

jahow
Copy link
Contributor

@jahow jahow commented Jun 8, 2017

Following #173

This PR adds a new data type: CDATA, along with a new method in Output.js called writeCDATA.

The CDATA type derives from the String type and adds a specific parse function for unmarshalling, as well as a marshal function which uses the writeCDATA function.

The following compiled files are included as well:

  • Jsonix-all.js
  • Jsonix-min.js
  • node/scripts/jsonix.js

I've added basic tests but I'm not sure this is enough so please don't hesitate to tell me if I need to add more. In particular, I couldn't find the right place to add a test that actually does marshalling/unmarshalling.

@jahow
Copy link
Contributor Author

jahow commented Jun 8, 2017

Please note that a property declared as:

{
    type: 'element',
    name: 'cdata',
    typeInfo: 'CDATA'
}

will be marshalled as:

<![CDATA[element value]]>

instead of:

<cdata>
    <![CDATA[element value]]>
</cdata>

I hope that makes sense. This is because properties of type value can only be marshalled by writing text in the element, and that would have been difficult/messy to change. Using a property element made more sense in my opinion.

@highsource
Copy link
Owner

highsource commented Jun 8, 2017 via email

@jahow
Copy link
Contributor Author

jahow commented Jun 9, 2017

You're probably right about unmarshalling, to be honest I haven't tested this yet as I needed pretty badly to have the marshalling part functional.

After sleeping on it, I though that maybe the right way to do this would be to add another property type called 'CDATA' ('c' in compact mode) which would work similar to the 'value' property type, except that it would write a CDATA section instead of a simple text node in the element.

The CDATA type I added wouldn't be needed altogether.

Using a value property:

{
    type: 'value',
    name: 'data',
    typeInfo: 'String'
}

would give:

<root>element value</root>

Whereas using a CDATA property:

{
    type: 'CDATA',
    name: 'data',
    typeInfo: 'String'
}

would give:

<root><![CDATA[element value]]></root>

What do you think?

@highsource
Copy link
Owner

highsource commented Jun 9, 2017 via email

@jahow
Copy link
Contributor Author

jahow commented Jun 9, 2017

Something like

{
    type: 'value',
    name: 'data',
    typeInfo: 'String',
    asCDATA: true
}

?

@highsource
Copy link
Owner

highsource commented Jun 9, 2017 via email

@jahow
Copy link
Contributor Author

jahow commented Jun 9, 2017

Perfect, I'll rework the PR in that direction and write some more tests!

@jahow
Copy link
Contributor Author

jahow commented Jun 13, 2017

@highsource better?

@highsource
Copy link
Owner

highsource commented Jun 13, 2017 via email

@jahow
Copy link
Contributor Author

jahow commented Jun 14, 2017

No problem, take your time! I just wanted to have this cleaned up & functional.

@highsource highsource merged commit 649399c into highsource:master Jun 21, 2017
@mercury83 mercury83 mentioned this pull request Mar 1, 2019
@highsource highsource self-requested a review March 18, 2019 20:05
@highsource highsource self-assigned this Mar 18, 2019
@highsource highsource added this to the 2.4.2 milestone Mar 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants