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

Remove support for future script macros #1871

Merged
merged 1 commit into from Feb 4, 2019

Conversation

ksolo
Copy link
Contributor

@ksolo ksolo commented Feb 1, 2019

libxml2 added support for future script macros based on the below
specification

https://www.w3.org/TR/html401/appendix/notes.html#h-B.7.1

This syntax was reserved, but has yet to be defined. It is still unclear
how these macros will be used and what risks will be created if/when
implemented.

This PR removes support until more information is provided.

@flavorjones
Copy link
Member

flavorjones commented Feb 1, 2019

Hey @ksolo! Thanks so much for submitting this, I agree we should remove this support in Nokogiri.

It looks like this PR failed CI because the test you added is being run even when Nokogiri is built against the "system" libxml2 (in our CI, this is Ubuntu). We should skip this test unless we're running with the vendored libxml2.

An example of how to do this is in https://github.com/sparklemotion/nokogiri/blob/master/test/html/test_attributes.rb

Basically, skip the test if Nokogiri::VersionInfo.instance.libxml2? && Nokogiri::VersionInfo.instance.libxml2_using_system?

Does that make sense?

@ksolo
Copy link
Contributor Author

ksolo commented Feb 1, 2019

@flavorjones That makes perfect sense. Thank you!

libxml2 added support for future script macros based on the below
specification

https://www.w3.org/TR/html401/appendix/notes.html#h-B.7.1

This syntax was reserved, but has yet to be defined. It is still unclear
how these macros will be used and what risks will be created if/when
implemented.

This PR removes support until more information is provided.
@codeclimate
Copy link

codeclimate bot commented Feb 1, 2019

Code Climate has analyzed commit f9d58f6 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.5%.

View more on Code Climate.

@tenderlove tenderlove merged commit 86dded1 into sparklemotion:master Feb 4, 2019
@flavorjones
Copy link
Member

👍

@flavorjones flavorjones added this to the v1.10.x patch releases milestone Apr 1, 2019
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.

None yet

3 participants