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

Removed characters stack from handler #1567

Closed
wants to merge 6 commits into from
Closed

Removed characters stack from handler #1567

wants to merge 6 commits into from

Conversation

andrew-aladev
Copy link
Contributor

Closes #1561.

libxml2 reports all characters between any two tokens, so it looks like we don't need a characterStack in handler.

@kares
Copy link
Contributor

kares commented Dec 29, 2016

Hey Andrew! which JRuby are you guys using? (personally was hoping for a JRuby 1.7.x compat release)

@andrew-aladev
Copy link
Contributor Author

@kares, Hello. I am using jruby-9.1.6.0 but this pull request works fine with jruby-1.7.26.

@kares
Copy link
Contributor

kares commented Jan 5, 2017

@andrew-aladev yy ... its that the official release 1.7 (this eventually gets into) dropped JRuby 1.7 support

@kares
Copy link
Contributor

kares commented Jan 13, 2017

disregard my comments, MRI < 2.1 support was dropped but JRuby 1.7 support is kept, for now. 🆒

@flavorjones
Copy link
Member

Hi! Thanks for submitting this PR.

There's a lot going on in this pull request that's not completely explained by either the PR description or the commit log.

For example, in the first commit you introduce a new file, and a later commit renames that file, and then an even later commit adds that file to the gem manifest. These should all be done in a single commit along with the new feature being tested.

Other commits make changes to the test helper's Document object that are not related to the code being changed.

I'm not necessarily opposed to any of these changes, but I'd like to ask you to clean this up so that each commit makes (and explains) one change, and the series of patches tells a story.

@andrew-aladev
Copy link
Contributor Author

Ok. I will provide a separate commit for each change.

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

Successfully merging this pull request may close these issues.

3 participants