CDATA filtering options, new item addition functions, unit tests #37

rv-kip · 2014-12-13T02:06:29Z

Allowing selective disabling of CDATA wrapping by field name.
Adding items by concatenation of exiting items of by replacement
Associated unit tests
example code
Readme updates.

This is similar to existing PR #28, but allows for finer control over what fields can be skipped for CDATA wrapping.

…concat_items and replace_items. Associated unit tests

rv-kip · 2014-12-15T21:29:00Z

examples/simple.js

Example of specifying a field for no CDATA wrapping

jasonkarns · 2014-12-15T21:37:43Z

Might I suggest the option be named something other than no_cdata_fields? Perhaps, no_encode_fields or similar? Including 'cdata' in the name implies something special with cdata, when in fact the option is just disabling the encoding of the listed elements. node-rss could just as well search/replace all <, &, etc and not use cdata at all. In which case, this option would disable search/replace encoding instead. (since there isn't any difference between cdata and entity-encoding as far as xml is concerned) And as cdata is a particular mechanism of encoding, I suggest the more generic term be used for the option instead.

rv-kip · 2014-12-16T00:14:39Z

Seems like a good suggestion. The xml module in node-rss already handles escaping of the fields if they are not specified with the CDATA attribute.

So, depending on the no_encode_fields flag,
title: 'item & <div>title</div>'
becomes
<title><![CDATA[item & <div>title</div>]]></title>
or
<title>item & <div>title</div></title>

jasonkarns · 2014-12-16T14:10:25Z

The xml module in node-rss already handles escaping of the fields if they are not specified with the CDATA attribute.

@rv-kip I must have misunderstood, then. If the elements are going to be encoded regardless whether they're in a CDATA block or not, then the no_encode_fields name doesn't fit. But now I'm really confused at the point of this PR. Consumers shouldn't care at all whether a particular field is entity-encoded or wrapped in CDATA. XML parsers treat either case identically. As a proper xml producer, both forms are indistinguishable.

rv-kip · 2014-12-16T19:24:30Z

In my use-case, some consumers of the generated XML/RSS will display the "CDATA" tags as if they were part of the content. This is a due to the heterogeneity of our XML clients. Being able to suppress the CDATA tags and escape the data for particular fields turns them into text fields that will display properly.

jasonkarns · 2014-12-16T19:29:22Z

As long as it's clear that this change (or a portion thereof) is to support
non-compliant XML clients.

Personally, I wouldn't accept any code meant solely to satisfy non
conformant parsers, but I don't own this project. :)

An alternative would be to wrap node-rss in a custom adapter object. This
way the scar tissue of dealing with non-conformant parsers would be cleanly
separated from node-rss. In fact, it would make a decent node module in its
own right (accepting arbitrary xml doms and serializing exclusively with
entity-encoding).

my 2 cents

On Tue, Dec 16, 2014 at 2:24 PM, Kip Gebhardt [email protected]
wrote:

In my use-case, some consumers of the generated XML/RSS will display the
"CDATA" tags as if they were part of the content. This is a due to the
heterogeneity of our XML clients. Being able to suppress the CDATA tags and
escape the data for particular fields turns them into text fields that will
display properly.

—
Reply to this email directly or view it on GitHub
#37 (comment).

rv-kip · 2014-12-16T19:38:27Z

Yep. It clearly isn't ideal that some clients behave incorrectly, but the XML landscape isn't a perfect one. Perhaps no_cdata_fields is a clearer name.

jasonkarns · 2014-12-16T19:50:44Z

Yeah, if the result will be encoded either way, better that the option be
named to convey precisely what it's doing.

On Tue, Dec 16, 2014 at 2:38 PM, Kip Gebhardt [email protected]
wrote:

Yep. It clearly isn't ideal that some clients behave incorrectly, but the
XML landscape isn't a perfect one. Perhaps no_cdata_fields is a clearer
name.

—
Reply to this email directly or view it on GitHub
#37 (comment).

…. Adding test for XML escaping when no_cdata_fields is used

… by item() resulting in inconsistencies like missing category field. Added test for this issue

rv-kip · 2014-12-18T00:02:41Z

I found a couple of typos and a bug while developing against this PR. I'll stop adding to it now ;)

dylang · 2014-12-19T18:53:57Z

Hi @rv-kip. I feel like a jerk because I touched a lot of the code while your PR was in flight and this might be a painful merge.

Do you want to try pull in the latest from master? Or split it into smaller PR's?

dylang · 2014-12-19T18:55:31Z

readme.md

The readme file is generated.

grunt pre-publish to re-generate it.

The files it uses are in /templates/readme.

rv-kip · 2014-12-19T19:12:55Z

No problem. I'll resubmit a PR from latest master. This can be closed.

…ply-for-new-pr Conflicts: lib/index.js test/rss.test.js

Kip Gebhardt and others added 5 commits December 12, 2014 16:14

Merge from upstream master

1fab44d

Allowing selective disabling of CDATA wrapping by field name. Adding …

23aa233

…concat_items and replace_items. Associated unit tests

Update readme.md

ab2ab66

Everyone loves an example

87e0c77

Didn't end up using underscore.

6ac9af9

rv-kip reviewed Dec 15, 2014
View reviewed changes

examples/simple.js

Copy link

Author

rv-kip Dec 15, 2014

Choose a reason for hiding this comment

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

Example of specifying a field for no CDATA wrapping

Kip Gebhardt added 2 commits December 15, 2014 16:34

Changing param name to no_encode_fields in all needed places

3070e49

... plus one more place

f0fc81f

Kip Gebhardt added 4 commits December 16, 2014 14:18

Going back to 'no_cdata_fields' for name. Clarifying use case in docs…

6fba453

…. Adding test for XML escaping when no_cdata_fields is used

Fixing typo in example

38363d1

README cleanup for custom_namespaces and custom_elements

e23f8f6

Fixing bug. Items passed in on RSS obj creation did not get processed…

f528b84

… by item() resulting in inconsistencies like missing category field. Added test for this issue

rv-kip mentioned this pull request Dec 19, 2014

added support for custom elements and namespaces #35

Merged

dylang reviewed Dec 19, 2014
View reviewed changes

readme.md Outdated

Copy link

Owner

dylang Dec 19, 2014

Choose a reason for hiding this comment

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

The readme file is generated.

grunt pre-publish to re-generate it.

The files it uses are in /templates/readme.

Kip Gebhardt added 2 commits December 19, 2014 11:38

Merge branch 'master' of https://github.com/dylang/node-rss into reap…

ff24ee9

…ply-for-new-pr Conflicts: lib/index.js test/rss.test.js

Moving tests from defunct PR into new branch

ce46bbf

rv-kip added 6 commits December 19, 2014 17:11

Added tests for new functionality

5993cd5

Missing mime dependency and a dupe dep

6478f12

Removing old mocha test

65a9d42

Documentation update and readme generation

fa4d015

Moving mime to dependencies

5450f8d

Indenting change on example. Commenting.

c1f6460

rv-kip closed this Dec 20, 2014

rv-kip mentioned this pull request Dec 20, 2014

CDATA filtering options, new item addition functions, unit tests, bug fixes #38

Open

CDATA filtering options, new item addition functions, unit tests #37

CDATA filtering options, new item addition functions, unit tests #37

Uh oh!

Conversation

rv-kip commented Dec 13, 2014

Uh oh!

rv-kip Dec 15, 2014

Choose a reason for hiding this comment

Uh oh!

jasonkarns commented Dec 15, 2014

Uh oh!

rv-kip commented Dec 16, 2014

Uh oh!

jasonkarns commented Dec 16, 2014

Uh oh!

rv-kip commented Dec 16, 2014

Uh oh!

jasonkarns commented Dec 16, 2014

Uh oh!

rv-kip commented Dec 16, 2014

Uh oh!

jasonkarns commented Dec 16, 2014

Uh oh!

rv-kip commented Dec 18, 2014

Uh oh!

dylang commented Dec 19, 2014

Uh oh!

dylang Dec 19, 2014

Choose a reason for hiding this comment

Uh oh!

rv-kip commented Dec 19, 2014

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants