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

Scrub invalid characters from source XML #72

Merged
merged 1 commit into from
Mar 4, 2016

Conversation

alethea
Copy link
Contributor

@alethea alethea commented Mar 4, 2016

Nori::Parser has a new option, :scrub_xml, which defaults to true, when it's true, the parser will clean invalid or undefined characters from the string using String#scrub if it's available (Ruby 2.1 or later) or String#encode otherwise. This should allow documents containing invalid characters to still be parsed.

@alethea alethea force-pushed the scrub_xml branch 2 times, most recently from fa1780e to 37d0191 Compare March 4, 2016 03:32
Nori::Parser has a new option, :scrub_xml, which defaults to true, when
it's true, the parser will clean invalid or undefined characters from
the string using String#scrub if it's available or String#encode
otherwise.
@tjarratt
Copy link
Contributor

tjarratt commented Mar 4, 2016

Thanks for contributing this pull request @alethea!

In particular, I appreciate you changing the old stubbed tests (which were probably in the wrong place) to instead test the behavior that we care about (scrubbing out invalid characters) rather than the implementation (sending a particular message to the string). That's awesome. I care a lot about good testing hygiene, and that's something I've wanted to invest in since I started maintaining, but never really managed to dedicate enough time to. Thank you. A thousand times, thank you.

tjarratt added a commit that referenced this pull request Mar 4, 2016
Scrub invalid characters from source XML
@tjarratt tjarratt merged commit 9370565 into savonrb:master Mar 4, 2016
@alethea
Copy link
Contributor Author

alethea commented Mar 4, 2016

You're welcome! Thanks for the speedy response and review.

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

Successfully merging this pull request may close these issues.

2 participants