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 an options parameter to all the methods that allow creation of an HTML document fragment #1692

Conversation

JackMc
Copy link
Contributor

@JackMc JackMc commented Nov 20, 2017

We are facing an issue where we cannot provide a DocumentFragment with the XML::ParseOptions::HUGE option because it doesn't take the options int like the regular Document class does. This adds an optional parameter for passing that down.

Let me know if there need to be extra tests to ensure this is working :-)

@flavorjones
Copy link
Member

There are failing tests here related to the jruby implementation that will need to be addressed.

I would like test coverage for this, yes, to ensure we're exercising this code path. Not sure which parse option to test with; maybe NOENT or NOERROR would be good candidates?

@JackMc
Copy link
Contributor Author

JackMc commented Nov 20, 2017

Thanks for the prompt response @flavorjones! I ran the tests locally, but I guess it only runs the plain Ruby implementation. Was just writing the Ruby test now. I'll look at the Java tests too :)

@JackMc
Copy link
Contributor Author

JackMc commented Dec 3, 2017

So after looking at this a bit, it turns out that there's a difference in the behaviour of the Java and C implementations. When instantiating a XmlDocumentFragment, the C implementation yields the provided block with an argument of itself (see https://github.com/JackMc/nokogiri/blob/058f5700fbff4b264b4727da484bfc419db1eb32/ext/nokogiri/xml_document_fragment.c#L28), whereas the Java implementation does not. This is causing trouble when using MRI and

This is not documented or tested and is used internally in one place as far as we can tell (d18f1ac#diff-8b98857b6a2cb5117b8b1de836357ecdR59). I'd argue that it would be best to rewrite this one usage so the block parameter can be freed up for this purpose (bringing it in line with XmlDocument's functionality) and so we can bring the Java and C implementations to functional equivalence.

@flavorjones what do you think?

@JackMc
Copy link
Contributor Author

JackMc commented Dec 3, 2017

Actually, it looks like that previous use is only for Node, so it doesn't seem to be used anywhere.

@JackMc
Copy link
Contributor Author

JackMc commented Jan 9, 2018

Bump :) This is mostly done I think. The tests that are failing here match the tests that are failing on master.

@JackMc JackMc force-pushed the add-options-argument-for-document-fragment branch from 4ba8b4a to d2a7947 Compare March 7, 2018 19:42
@JackMc
Copy link
Contributor Author

JackMc commented Mar 7, 2018

Bump :) I've rebased upon master now so this should be easier to review

@JackMc
Copy link
Contributor Author

JackMc commented Jul 23, 2018

Bump :)

@flavorjones flavorjones added this to the next milestone Nov 8, 2018
@flavorjones
Copy link
Member

Sorry for the terrible response time on this. I'd like to get this into v1.9.0, and so I'm going to rebase these commits and submit a separate PR to make sure they pass CI. I'll continue any conversation that needs to happen here, but will link to the other branch for visibility.

@flavorjones
Copy link
Member

Rebase went green at #1843.

I noticed the test added for this, test_passed_options_have_an_effect only seems to test that the call accepts a block. I'm going to increase test coverage to ensure options set either by argument or in block take effect.

@flavorjones
Copy link
Member

Ok - so this changeset doesn't actually work in the JRuby implementation, and so it's not going to make it into v1.9.0.

It's also becoming apparent, as I added test coverage, that the JRuby implementation has some other pre-existing warts that I need to fix. Here's how I'd like to approach:

  1. [net new behavior] have all XML::Document and HTML::Document underlying parse methods (e.g., #read_io, #read_memory) save the ParseOptions as a Document attribute, to make testing a bit easier
  2. [partially implemented by this PR] have all XML::DocumentFragment and HTML::DocumentFragment parse methods take options and an options-configuration block
  3. [fixes] ensure the JRuby impl matches the CRuby behaviors

I'm going to open a new issue, linking to this one, to track all the additional work. I'll make sure your commit appears in master so you get contributor credit.

Changing target milestone to v1.10.0

@flavorjones
Copy link
Member

See #1844 for progress on this work.

@flavorjones flavorjones self-assigned this Dec 29, 2018
@flavorjones flavorjones modified the milestones: v1.10.0, v1.11.0 Jan 4, 2019
@flavorjones
Copy link
Member

Please note that the latest CI failure is a problem with the new PR pipeline and not an issue with this PR, which previously was green. Still targetting for v1.11.0.

Base automatically changed from master to main January 17, 2021 21:52
@flavorjones flavorjones modified the milestones: v1.12.0, v1.13.0 Aug 2, 2021
flavorjones added a commit that referenced this pull request Dec 24, 2021
Similar to Document constructors, we can now pass in parse options as
a parameter or via a config block.

Fixes #1692
Fixes #1844

Co-authored-by: Jack McCracken <jack.mccracken@shopify.com>
@flavorjones
Copy link
Member

See #2399 for an updated version of this branch.

flavorjones added a commit that referenced this pull request Dec 24, 2021
Similar to Document constructors, we can now pass in parse options as
a parameter or via a config block.

Fixes #1692
Fixes #1844

Co-authored-by: Jack McCracken <jack.mccracken@shopify.com>
@flavorjones
Copy link
Member

Will ship in v1.13.0 ... thanks for your patience!

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.

None yet

2 participants