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

Deprecate positional hash arguments in the API #3200

Open
flavorjones opened this issue May 21, 2024 · 0 comments
Open

Deprecate positional hash arguments in the API #3200

flavorjones opened this issue May 21, 2024 · 0 comments
Milestone

Comments

@flavorjones
Copy link
Member

flavorjones commented May 21, 2024

Currently we've still got a few methods that use the pre-Ruby-3.0 convention of using a positional hash argument to pass options.

For example:

# lib/nokogiri/html5/document_fragment.rb
def initialize(doc, tags = nil, ctx = nil, options = {})
  ...
end

...

def self.parse(tags, encoding = nil, options = {})
  ...
end

I'd like to start planning to deprecate these positional hashes in favor of true keyword arguments.

Note that we can't simply move to keyword arguments, since calls that use explicit hashes would stop working:

Nokogiri::HTML5::DocumentFragment.parse(frag, nil, {max_errors: 1})
# ArgumentError: wrong number of arguments (given 3, expected 1..2)
#     lib/nokogiri/html5/document_fragment.rb:59:in `parse'

So we'll need to do an intermediate step that looks something like:

diff --git a/lib/nokogiri/html5/document_fragment.rb b/lib/nokogiri/html5/document_fragment.rb
index 1cd4b28d..6d3f1fe1 100644
--- a/lib/nokogiri/html5/document_fragment.rb
+++ b/lib/nokogiri/html5/document_fragment.rb
@@ -56,7 +56,12 @@ def serialize(options = {}, &block) # :nodoc:
       end
 
       # Parse a document fragment from +tags+, returning a Nodeset.
-      def self.parse(tags, encoding = nil, **options)
+      def self.parse(tags, encoding = nil, positional_options_hash = nil, **options)
+        unless positional_options_hash.nil?
+          warn("ClassName.methodname: Passing options as an explicit hash is deprecated. Use keyword arguments instead. This will become an error in a future release.", uplevel: 1, category: :deprecated)
+          options.merge!(positional_options_hash)
+        end
+
         doc = HTML5::Document.new
         tags = HTML5.read_and_encode(tags, encoding)
         doc.encoding = "UTF-8"
flavorjones added a commit that referenced this issue Jun 6, 2024
flavorjones added a commit that referenced this issue Jun 11, 2024
flavorjones added a commit that referenced this issue Jun 11, 2024
flavorjones added a commit that referenced this issue Jun 22, 2024
…ext (#3246)

**What problem is this PR intended to solve?**

Coming from the discussion at #3203, I wanted to improve the fragment
parsing API

- as discussed in #2646, parse in no-quirks mode if a context element
name is provided (not a context `Node`, just the name)
- allow passing `:context` kwarg to `DocumentFragment.new` and `.parse`
- deprecate the positional options hash to `.parse` per notes at #3200


**Have you included adequate test coverage?**

Included additional coverage for the API changes


**Does this change affect the behavior of either the C or the Java
implementations?**

HTML5 is only in CRuby.
@flavorjones flavorjones added this to the v1.18.0 milestone Jul 3, 2024
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

No branches or pull requests

1 participant