node-rss icon indicating copy to clipboard operation
node-rss copied to clipboard

Published 20 hours ago • verified publisher icon dylang

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

Open rv-kip opened this issue 10 years ago • 17 comments

  • 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.

rv-kip avatar Dec 20 '14 01:12 rv-kip

This is ready for review.

rv-kip avatar Dec 21 '14 18:12 rv-kip

(bump)

rv-kip avatar Jan 12 '15 19:01 rv-kip

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

rv-kip avatar Jan 20 '15 19:01 rv-kip

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

dylang avatar Jan 21 '15 05:01 dylang

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

rv-kip avatar Jan 21 '15 19:01 rv-kip

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 avatar Feb 08 '15 21:02 dylang

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

rv-kip avatar Feb 09 '15 23:02 rv-kip

To clarify, should feed.removeItems() remove all existing items?

rv-kip avatar Feb 09 '15 23:02 rv-kip

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

rv-kip avatar Feb 12 '15 23:02 rv-kip

@dylang, How are we going on this?

rv-kip avatar Mar 16 '15 21:03 rv-kip

@dylang... Any progress on this?

rv-kip avatar Apr 27 '15 21:04 rv-kip

@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. :smile: https://www.npmjs.com/package/rss-braider

rv-kip avatar May 29 '15 18:05 rv-kip

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 avatar Feb 27 '18 20:02 rv-kip

close/open to trigger Travis

rv-kip avatar Jun 27 '18 17:06 rv-kip

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

rajinikumar avatar Jul 16 '18 11:07 rajinikumar

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

rajinikumar avatar Jul 16 '18 11:07 rajinikumar

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

mattymess avatar Oct 10 '19 22:10 mattymess