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

Feature/ox #74

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Feature/ox #74

wants to merge 2 commits into from

Conversation

mwlang
Copy link

@mwlang mwlang commented Mar 24, 2016

Implements a Sax Parser based on the Ox gem: https://github.com/ohler55/ox

There is one test pertaining to scrubbing invalid encodings that I have no idea what to do to resolve to the expectation. I tried passing the given XML into the existing scrub_xml method, but it did not make the test pass and seems such pre-processing would defeat the purpose of SAX parsing anyhow. I noticed the other two Parsers aren't scrubbing XML, either, so how they pass this test is beyond me. Personally, I'd say this doesn't really seem like a valid use-case to be concerned with in the library and should be removed altogether. It should be the user's responsibility to pass correctly encoded strings to the library.

Anyway, The Ox parser is working very well and I've already put it to use in our production environment. Hopefully, you'll find this a good addition to this library.

Pending:
  Hash#normalize_param should have specs
    # Not yet implemented
    # ./spec/nori/core_ext/hash_spec.rb:6

Failures:

  1) Nori using the :ox parser should scrub bad characters
     Failure/Error: expect(parse(xml)["tag"]).to eq("a\uFFFDc")

       expected: "a\uFFFDc"
            got: "a\xEF\xBF\xBDc"

       (compared using ==)
     # ./spec/nori/nori_spec.rb:33:in `block (4 levels) in <top (required)>'

Finished in 0.12789 seconds
240 examples, 1 failure, 1 pending

Failed examples:

rspec ./spec/nori/nori_spec.rb:31 # Nori using the :ox parser should scrub bad characters

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.

1 participant