clone & singleton classes #316
Comments
|
Fair enough, will fix. |
|
Well, here's a question for you: Nokogiri documents already follow "nontraditional" clone/dup semantics by copying the underlying document tree. So: can you explain your use case for extending an object and then cloning it? I'm just trying to wrap my head around how this idiom is being used with Nokogiri objects. |
|
I coded up some parsing methods that apply for any web site; the methods are in a module which I include in Nokogiri document. Later on I needed some custom parsing for some specific domains. The easy way out was to extend the document with the module specific for the domain. Different parsing need to be done and tend to be destructive so I cloned the document before calling the different parsing methods. There are other solutions; I can subclass Document, have the methods be module functions and pass the document, create a sort of delegate class and sub class that, extend all clones, etc... But since cloning should copy the singleton class, I thought you might want to fix it :-) |
|
Thanks for going into detail. I'm always interested in what people are doing with nokogiri (well, almost always ;)). LIke I said, this is a fair enough request, I don't have a problem with it. We'll schedule work, and probably target 1.5. |
|
Description of differences between |
|
Picked up this work yesterday, see progress in #3117 "Better late than never" as they say 😅 |
**What problem is this PR intended to solve?** Fixes #316 Classes this PR updates: - [x] `XML::Node` - [x] `XML::Document` - [x] `XML::NodeSet` **Have you included adequate test coverage?** Yes. **Does this change affect the behavior of either the C or the Java implementations?** The fix applies to both implementations.
Back in b92660e (#1834 fixing #1063) I omitted support in JRuby for the "new_parent_document" argument to `Node#dup` because there was no performance reason to implement it. So the test was skipped. However, in 1e7d38a and other commits in #3117 (fixing #316), I introduced a call to `initialize_copy_with_args` that passes the new parent document as an argument on both CRuby and JRuby implementations. Because the test was skipped, I didn't catch that this broke on JRuby. In particular this was a problem for Loofah which relies on decorators, and even more particularly this broke the `Loofah::TextBehavior` formatting concern for `Loofah::*::DocumentFragment` objects. Maybe we should be running downstream tests with JRuby, too? But that feels like a big investment right now so I'll avoid scarring on the first cut, and wait to see if it happens again.
Back in b92660e (#1834 fixing #1063) I omitted support in JRuby for the "new_parent_document" argument to `Node#dup` because there was no performance reason to implement it. So the test was skipped. However, in 1e7d38a and other commits in #3117 (fixing #316), I introduced a call to `initialize_copy_with_args` that passes the new parent document as an argument on both CRuby and JRuby implementations. Because the test was skipped, I didn't catch that this broke on JRuby. In particular this was a problem for Loofah which relies on decorators, and even more particularly this broke the `Loofah::TextBehavior` formatting concern for `Loofah::*::DocumentFragment` objects. Maybe we should be running downstream tests with JRuby, too? But that feels like a big investment right now so I'll avoid scarring on the first cut, and wait to see if it happens again. (cherry picked from commit dda0be2)
**What problem is this PR intended to solve?** Back in b92660e (#1834 fixing #1063) I omitted support in JRuby for the "new_parent_document" argument to `Node#dup` because there was no performance reason to implement it. So the test was skipped. However, in 1e7d38a and other commits in #3117 (fixing #316), I introduced a call to `initialize_copy_with_args` that passes the new parent document as an argument on both CRuby and JRuby implementations. Because the test was skipped, I didn't catch that this broke on JRuby. In particular this was a problem for Loofah which relies on decorators, and even more particularly this broke the `Loofah::TextBehavior` formatting concern for `Loofah::*::DocumentFragment` objects. **Have you included adequate test coverage?** The skipped test is no longer skipped! Maybe we should be running downstream tests with JRuby, too? But that feels like a big investment right now so I'll avoid scarring on the first cut, and wait to see if it happens again. **Does this change affect the behavior of either the C or the Java implementations?** Brings the Java impl into agreement with the C impl.
If a specific Nokogiri::HTML::Document extends a module M, a clone of that document won't.
I expected otherwise, since in Ruby all builtin classes, as well as user created classes will copy the singleton_class when cloning.
Thakns!
require 'nokogiri'
require 'open-uri'
module M; def foo; end end
doc = Nokogiri::HTML(open('http://www.google.com/search?q=tenderlove'))
doc.extend M
doc.clone.respond_to? :foo # => false, should be true
The text was updated successfully, but these errors were encountered: