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

Add Nokogiri::XML::Node#line= #1918

Merged
merged 1 commit into from
Nov 26, 2019
Merged

Add Nokogiri::XML::Node#line= #1918

merged 1 commit into from
Nov 26, 2019

Conversation

stevecheckoway
Copy link
Contributor

Add support for setting the line number on elements from Ruby. This only
works when Nokogiri uses libxml2.

Line numbers less than 65535 may be set. Larger line numbers require
libxml2 to be compiled with a particular option and even then, it is
buggy. See https://bugzilla.gnome.org/show_bug.cgi?id=767138.


Thank you for contributing to Nokogiri! To help us prioritize, please take care to answer the questions below when you submit this pull request.

What problem is this PR intended to solve?

This PR is intended to solve the issues for downstream projects. In particular, gjtorikian/html-proofer#362 in html-proofer which is preventing that from using Nokogumbo for HTML5 parsing.

This implementation is similar to #1658 (which I think was broken because the option, XML_PARSE_BIG_LINES, mentioned in #1764 is not used). This version simply limits the number of lines to a maximum of 65534. This limitation could be removed if XML_PARSE_BIG_LINES is used, but that could be done later.

Note that the implementation of set_line() is slightly different from #1658 which returns self rather than the argument. set_native_content() and set_name return the argument so I followed that approach.

Have you included adequate test coverage?

It is tested in the same manner as Nokogiri::XML::Node#line. (I think that should be better tested, but JRuby's line number heuristic seems like it would be easy to break and I'm not sure how to use JRuby to test.)

Does this change affect the C or the Java implementations?

This effects the C version only. The Java version uses a heuristic to get line numbers but that heuristic doesn't quite work.

Add support for setting the line number on elements from Ruby. This only
works when Nokogiri uses `libxml2`.

Line numbers less than 65535 may be set. Larger line numbers require
`libxml2` to be compiled with a particular option and even then, it is
buggy. See https://bugzilla.gnome.org/show_bug.cgi?id=767138.
@codeclimate
Copy link

codeclimate bot commented Aug 10, 2019

Code Climate has analyzed commit a730f85 and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 100.0% (80% is the threshold).

This pull request will bring the total coverage in the repository to 93.8% (0.0% change).

View more on Code Climate.

@flavorjones
Copy link
Member

Thanks for submitting this, I'll do my best to take a look in the next day or so.

@gjtorikian
Copy link
Contributor

Any updates on this?

@flavorjones flavorjones added this to the v1.11.0 milestone Nov 8, 2019
@flavorjones
Copy link
Member

@gjtorikian thanks for poking. my personal life is nuts right now and have had limited time to devote to new-feature and PR velocity (have only managed to get security-related updates out for some packages). I'm aligned with this feature but haven't had time to review details and merge it. That said, I expect this will be in v1.11.0 and have tagged it for that milestone. ETA is in the next week or two (optimistically).

@gjtorikian
Copy link
Contributor

Ah, no worries. Self care is important and I hope things settle down for you. 💜

@flavorjones
Copy link
Member

Thanks for your patience. Merging. Will be in v1.11.0, due out in late December (coincident with the Ruby 2.7 release).

@flavorjones flavorjones merged commit d3f18d2 into sparklemotion:master Nov 26, 2019
@stevecheckoway stevecheckoway deleted the linenum branch November 26, 2019 18:11
@flavorjones
Copy link
Member

Please note that this was included in v1.11.0.rc1, so if you want to test, you can use that version.

@stevecheckoway
Copy link
Contributor Author

Great, thanks!

This was referenced Mar 15, 2021
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.

4 participants