-
Notifications
You must be signed in to change notification settings - Fork 51
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
Implements an option to prevent the automatic encoding #26
base: master
Are you sure you want to change the base?
Conversation
This change looks good 👍 Would you mind dropping a bit more of an explanation, including references, for the use-case here? This will help with documentation and other people trying to do the same thing. |
Sure! Here's a usage example I used in the tests: xml(
[ { x: '<a>test</a>' } ],
{ escape: false }
) This would return <x><a>test</a></x> We'd probably also want a local way to do this. Similar to how [ { x: { _raw: '<a>test</a>' } } ] |
I'm interested to understand fully the reason for doing this in atom - you say "[feed entries] can have a type="html" attribute.", I'm trying to understand the difference between "can" and "should", is there a reference in the atom docs or to something about best practice that you can link here? There is a very related issue on the node-rss repo (which directly depends on node-xml): dylang/node-rss#27 - node-rss is supposed to output atom-compatible feeds, and the discussion there suggests that escaping should happen only if there are chars which need escaping, E.g. an 'auto' mode rather than on/off. Just trying to get an idea of what the best path forward is for both node-xml and node-rss in respect to this issue. |
Sorry, I may have been a bit wage in my explanation. Text elements can have an attribute
Here's the relevant part on their Website The example on that page isn't very clear. But as I interpret it, you could do
That As far as I understand the problem in dylang/node-rss#27 is only remotely related. The auto detection he's talking about isn't something that I would include in node-xml itself (except maybe as an option), since it would give the programmer less control. |
I wanted to check up on the state of this issue, since the Atom feed on my project is invalid ATM. Will this get merged? |
I also have a project that would benefit from this additional functionality. |
I'm sorry, for some reason all the notifications from this repo started going into my spam folder so I had no idea there'd been any followup! This PR currently has a merge conflict. If we could get that fixed up, then I'll look at pushing this out. This PR is based from a master branch, making it slightly tricker than normal to fixup, but these steps should work assuming your repo is called
Hopefully this gets us moving again! |
Great! Thanks a lot. :) I'll probably get to it today. |
Thanks a lot @ErisDS for the instructions :) |
Is there still something that has to be done, in order for this PR to be merged? |
We need this too! |
Bump! Looking forward to this |
I'm implementing an Atom feed generator and I'd like to be able to disable the automatic string encoding.