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

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

Jump to bottom

Open

rv-kip wants to merge 28 commits into dylang:master

from rv-kip:master

Open

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

rv-kip wants to merge 28 commits into dylang:master from rv-kip:master

Conversation

@rv-kip

Copy link

@rv-kip rv-kip commented Dec 20, 2014

Optional disabling of CDATA wrapping by field name
Adding items by concatenation of existing items or by complete replacement of existing items
Associated unit tests
Runnable example code examples/simple.js
Corresponding readme updates.

This PR supercedes #37, which has some relevant discussion.
This PR is similar to existing PR #28, but allows for finer control over what fields can be skipped for CDATA wrapping.

Kip Gebhardt and others added 19 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

@rv-kip


 Update readme.md

ab2ab66


 Everyone loves an example

87e0c77


 Didn't end up using underscore.

6ac9af9


 Changing param name to no_encode_fields in all needed places

3070e49


 ... plus one more place

f0fc81f


 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


 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 tests for new functionality

5993cd5

@rv-kip


 Missing mime dependency and a dupe dep

6478f12

@rv-kip


 Removing old mocha test

65a9d42

@rv-kip


 Documentation update and readme generation

fa4d015

@rv-kip


 Moving mime to dependencies

5450f8d

@rv-kip


 Indenting change on example. Commenting.

c1f6460

[画像:dylang]

dylang reviewed

Dec 20, 2014

View reviewed changes

package.json Outdated

Copy link

Owner

Choose a reason for hiding this comment

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

mime-types should be good enough

Copy link

Author

Choose a reason for hiding this comment

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

When I clone (or rm -rf node_modules), npm i, and npm test, I get an error for missing mime module unless mime is in package.json

Copy link

Owner

Choose a reason for hiding this comment

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

see above :)

@rv-kip


 Removing unecessary mime module.

603b252

@rv-kip

Copy link

Author

rv-kip commented Dec 21, 2014

This is ready for review.

@rv-kip

Copy link

Author

rv-kip commented Jan 20, 2015

@dylang, Is there any more info, documentation, or testing you need from me for this PR?

@dylang

Copy link

Owner

dylang commented Jan 21, 2015

Sorry for taking so long with this. I'll try to get to it later this week.

@rv-kip

Copy link

Author

rv-kip commented Jan 21, 2015

Not a problem. Just wanted to make sure you weren't waiting for something from me.

@dylang

Copy link

Owner

dylang commented Feb 8, 2015

The cdata work looks good. I don't like the generic output function name, but at least it's short.

The replace_items and concat_items feel like they should be in a separate PR. Since you've already done the work here there's no need to create a separate PR.

I think a better API would be something like feed.removeItems() and feed.addItems(<array of items>). What do you think?

@dylang dylang mentioned this pull request

Feb 8, 2015

Option to specify CDATA wrapper for content #28

Closed

@rv-kip

Copy link

Author

rv-kip commented Feb 9, 2015

@dylang Your function rename suggestions are fine. Better to change them now than have a legacy support issue.

@rv-kip

Copy link

Author

rv-kip commented Feb 12, 2015

@dylang, my latest commit changes the names of the extra functions and adds a unit test for removeAllItems() Ready for review.

@rv-kip

Copy link

Author

@dylang, I've released the rss-braider module that depends on the changes in this PR. I'd prefer to be dependent on your repo and not my fork. Let me know if there's anything I can do to help with merging this PR. 😄
https://www.npmjs.com/package/rss-braider

rv-kip and others added 4 commits

November 2, 2015 08:24

@rv-kip


 Fixing merge conflicts

bfe54b7

@rv-kip


 Removing newlines that were breaking unit tests after 87c956d

cd123b6

@rv-kip


 Merge branch 'master' into master

a8cfc9e

@rv-kip


 Update package.json

6b7e4d7

@rv-kip rv-kip closed this

Feb 27, 2018

@rv-kip rv-kip reopened this

Feb 27, 2018

@rv-kip

Copy link

Author

rv-kip commented Feb 27, 2018

It seems like the Travis failure is related to npm3. This env has an npm config of

...
 "before_install": "npm -g i npm@3",
...

@rv-kip rv-kip mentioned this pull request

Feb 27, 2018

no_cdata_fields not working rv-kip/rss-braider#7

Open

@rv-kip

Copy link

Author

rv-kip commented Jun 27, 2018

close/open to trigger Travis

@rv-kip rv-kip closed this

Jun 27, 2018

@rv-kip rv-kip reopened this

Jun 27, 2018

@rajinikumar

Copy link

rajinikumar commented Jul 16, 2018

@dylang, Could you please reivew the @rv-kip changes.

@rajinikumar

Copy link

rajinikumar commented Jul 16, 2018

@dylang Please review @rv-kip changes. because he done the changes few years back.

@dylang dylang requested a review from ErisDS

July 16, 2018 14:37

@mattymess

Copy link

mattymess commented Oct 10, 2019

Any movement on this? Would love to see it get merged

Labels

None yet

5 participants

@rv-kip @dylang @rajinikumar @mattymess @karan-kapoor90

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

Are you sure you want to change the base?

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

Uh oh!

Conversation

@rv-kip rv-kip commented Dec 20, 2014

Uh oh!

@dylang dylang Dec 20, 2014

Choose a reason for hiding this comment

Uh oh!

@rv-kip rv-kip Dec 20, 2014

Choose a reason for hiding this comment

Uh oh!

@dylang dylang Dec 20, 2014

Choose a reason for hiding this comment

Uh oh!

rv-kip commented Dec 21, 2014

Uh oh!

rv-kip commented Jan 12, 2015

Uh oh!

rv-kip commented Jan 20, 2015

Uh oh!

dylang commented Jan 21, 2015

Uh oh!

rv-kip commented Jan 21, 2015

Uh oh!

dylang commented Feb 8, 2015

Uh oh!

rv-kip commented Feb 9, 2015

Uh oh!

rv-kip commented Feb 9, 2015

Uh oh!

rv-kip commented Feb 12, 2015

Uh oh!

rv-kip commented Mar 16, 2015

Uh oh!

rv-kip commented Apr 27, 2015

Uh oh!

rv-kip commented May 29, 2015

Uh oh!

rv-kip commented Feb 27, 2018

Uh oh!

rv-kip commented Jun 27, 2018

Uh oh!

rajinikumar commented Jul 16, 2018

Uh oh!

rajinikumar commented Jul 16, 2018

Uh oh!

mattymess commented Oct 10, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants