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

Introduce support for keyword arguments #3323

Open
flavorjones opened this issue Nov 14, 2024 · 3 comments
Open

Introduce support for keyword arguments #3323

flavorjones opened this issue Nov 14, 2024 · 3 comments
Labels
event/hackday2024 RubyConf 2024 Hack Day

Comments

@flavorjones
Copy link
Member

flavorjones commented Nov 14, 2024

The Nokogiri API was largely designed and implemented for Ruby 1.9, which pre-dates keyword argument support and parameter forwarding with .... As a result, many methods are using positional arguments where keyword arguments are probably better.

For example, Nokogiri::XML::Document.parse is declared as:

def parse(string_or_io, url = nil, encoding = nil, options = ParseOptions::DEFAULT_XML)

I would like to update these method signatures to support keyword arguments without dropping support (yet) for positional arguments. In the case of Document.parse, this might look like:

def parse(string_or_io,
          url_ = nil, encoding_ = nil, options_ = XML::ParseOptions::DEFAULT_XML,
          url: url_, encoding: encoding_, options: options_)

I would also like to update the docstrings to describe the keyword arguments, and remove documentation for the positional arguments.

Another example is Nokogiri::XML() which is defined as:

def XML(thing, url = nil, encoding = nil, options = XML::ParseOptions::DEFAULT_XML, &block)
  Nokogiri::XML::Document.parse(thing, url, encoding, options, &block)
end

Methods that are forwarding positional arguments should be updated to more cleanly forward either keyword arguments or positional arguments, and avoid redeclaring parameter default values:

def XML(...)
  Nokogiri::XML::Document.parse(...)
end
@flavorjones
Copy link
Member Author

For an example of the kind of documentation I'd like to see, take a look at #3324 which updates Nokogiri::XML::Document.parse.

flavorjones added a commit that referenced this issue Nov 14, 2024
@flavorjones
Copy link
Member Author

The list of methods that need to be updated is at https://docs.google.com/spreadsheets/d/1pWM7N5D9073J8IRVO0yghIySTYhsJoO73sslAVxEO-A/edit?gid=0#gid=0

We'll work on this for RubyConf 2024 hack day!

flavorjones added a commit that referenced this issue Nov 14, 2024
**What problem is this PR intended to solve?**

Related to #3323, introducing keyword argument support in
`XML::Document.parse`.


**Have you included adequate test coverage?**

Some minor test coverage introduced.


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

n/a
@flavorjones flavorjones added the event/hackday2024 RubyConf 2024 Hack Day label Nov 14, 2024
openbl added a commit to openbl/nokogiri that referenced this issue Nov 14, 2024
openbl added a commit to openbl/nokogiri that referenced this issue Nov 14, 2024
openbl added a commit to openbl/nokogiri that referenced this issue Nov 14, 2024
@flavorjones
Copy link
Member Author

TODO: link back to this issue in doc strings for people to discover some documentation about the positional arguments

flavorjones added a commit that referenced this issue Nov 14, 2024
**What problem is this PR intended to solve?**

#3323

**Have you included adequate test coverage?**

No new behavior has been added, so existing tests are sufficient.

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

No.
openbl added a commit to openbl/nokogiri that referenced this issue Nov 15, 2024
flavorjones added a commit that referenced this issue Dec 6, 2024
**What problem is this PR intended to solve?**
#3323

**Have you included adequate test coverage?**
No test changes

**Does this change affect the behavior of either the C or the Java
implementations?**
No
flavorjones added a commit that referenced this issue Dec 6, 2024
hackday 🙂

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

#3323 

**Have you included adequate test coverage?**

No test changes

**Does this change affect the behavior of either the C or the Java
implementations?**
No
flavorjones added a commit that referenced this issue Dec 6, 2024
**What problem is this PR intended to solve?**

Related to #3323,
introducing keyword argument support in
Nokogiri::XML::Schema.read_memory() and argument forwarding in
Nokogiri::XML::Schema{,.new}().

**Have you included adequate test coverage?**

Some minor test coverage mimicking the existing permutations of options.

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

No
flavorjones pushed a commit that referenced this issue Dec 8, 2024
flavorjones added a commit that referenced this issue Dec 8, 2024
Related to #3323

This commit was merged and expanded from #3336, thank you @MattJones!

Co-authored-by: Matt Jones <[email protected]>
flavorjones added a commit that referenced this issue Dec 8, 2024
Related to #3323

This commit was merged and expanded from #3335, thank you @infews!

Co-authored-by: Davis W. Frank <[email protected]>
flavorjones added a commit that referenced this issue Dec 8, 2024
and improve the documentation

Part of #3323
flavorjones added a commit that referenced this issue Dec 8, 2024
flavorjones pushed a commit that referenced this issue Dec 8, 2024
flavorjones added a commit that referenced this issue Dec 8, 2024
Related to #3323

This commit was merged and expanded from #3336, thank you @MattJones!

Co-authored-by: Matt Jones <[email protected]>
flavorjones added a commit that referenced this issue Dec 8, 2024
Related to #3323

This commit was merged and expanded from #3335, thank you @infews!

Co-authored-by: Davis W. Frank <[email protected]>
flavorjones added a commit that referenced this issue Dec 8, 2024
and improve the documentation

Part of #3323
flavorjones added a commit that referenced this issue Dec 8, 2024
flavorjones added a commit that referenced this issue Dec 8, 2024
flavorjones added a commit that referenced this issue Dec 8, 2024
…ke keyword arguments (#3355)

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

As part of #3323 there were a few RubyConf 2024 Hack Day pull requests
addressing Document and DocumentFragment constructors which were related
to, or blocked on, some changes to the C code.

So this PR is a mega-PR that merges all those PRs and unifies the code
and doc style:

- #3327 
- #3336 
- #3334 
- #3335 

But in addition to those PRs also updates:

- the `XML::DocumentFragment` new/initialize argument handling for CRuby
and JRuby
- `XML::DocumentFragment#initialize` kwargs
- `HTML4::Document.parse` kwargs
- general improvement of documentation

**Have you included adequate test coverage?**

I think so!

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

The XML::DocumentFragment allocator has changed, but both
implementations have been updated.
flavorjones added a commit that referenced this issue Dec 8, 2024
…#3332)

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

Related to #3323,
introducing keyword argument support in Nokogiri::XML::Document.parse.

**Have you included adequate test coverage?**

Some minor test coverage mimicking the existing permutations of options.

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

No
flavorjones added a commit that referenced this issue Dec 8, 2024
**What problem is this PR intended to solve?**

part of #3323

**Have you included adequate test coverage?**

Existing coverage should be fine.

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

No.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
event/hackday2024 RubyConf 2024 Hack Day
Projects
None yet
Development

No branches or pull requests

1 participant