Skip to content
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

Adhere to Cheerio's XML mode when xmlMode option is specified #22

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

inta
Copy link

@inta inta commented May 8, 2014

Just the small addition of xmlMode/$.xml as mentioned in issue 4.

@cgross
Copy link
Owner

cgross commented May 12, 2014

Thanks for the pull request. There's alot more that would be needed here until a release can be made. There's obviously documentation (otherwise nobody but you, I, and anyone who reads this PR would know about it). Also, at least one unit test.

Could you make those changes?

@inta
Copy link
Author

inta commented May 12, 2014

Yea, I can write some doc.

What do you expect the test to cover? Just one test with a XHTML/XML file (to ensure the output is correct XML), or running all existing tests again with xmlMode enabled?

@cgross
Copy link
Owner

cgross commented May 12, 2014

One test with an XHTML/XML file would be fine. Just please make sure it has the tags that cheerio was previously changing (like meta and br). Thanks!

@inta
Copy link
Author

inta commented May 12, 2014

Humph, seems not to be that easy. The xml function will rewrite empty elements to self closing ones. That is valid XML, but not XHTML. E.g. <script src="…"></script> will end up as <script src="…"/> which is useless.

@cgross
Copy link
Owner

cgross commented May 12, 2014

Isn't that still valid XHTML?

@inta
Copy link
Author

inta commented May 13, 2014

That is not valid XHTML 1 and definitely not valid for polyglot documents either. I do not really know about XHTML5, but polyglot problem does apply here as well.

I think that cannot be fixed here. I do not know if Cheerio or htmlparser2 do offer anything to handle XHTML.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants