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 global XPath functions handler #1894

Open
wants to merge 1 commit into
base: master
from

Conversation

@jonathanhefner
Copy link
Contributor

jonathanhefner commented Apr 17, 2019

What problem is this PR intended to solve?

Allow global definition of XPath functions, e.g.

module MyFunctions
  def regex(node_set, pattern)
    node_set.find_all { |node| node['some_attribute'] =~ /#{pattern}/ }
  end
end

Nokogiri::XML::XPathFunctions.include(MyFunctions)

node.search('.//title[regex(., "\w+")]', 'div.employee:regex("[0-9]+")')

This is a revival of #464 with a different approach. Although that PR is shown as merged, I think that's a GitHub bug. The original implementation was mbklein/nokogiri@8fb3da3 which has not been merged.

Have you included adequate test coverage?

I think so. I refrained from adding tests which would overlap with existing tests, but I would be happy to add more, if you have suggestions.

Does this change affect the C or the Java implementations?

No. Apparently, yes. The Java implementation injects XML namespaces into XPath queries based on the Ruby methods the function handler defines. Previously, that list of methods was derived from the function handler class, which omits methods delegated by SimpleDelegator. Now the list of methods is derived directly from the function handler object.

@codeclimate

This comment has been minimized.

Copy link

codeclimate bot commented Apr 17, 2019

Code Climate has analyzed commit c3376dd 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% (0.0% change).

View more on Code Climate.

@jonathanhefner jonathanhefner force-pushed the jonathanhefner:xpath-functions branch 2 times, most recently from 99ba913 to 28ddb33 Apr 17, 2019
@jonathanhefner jonathanhefner force-pushed the jonathanhefner:xpath-functions branch from 28ddb33 to c3376dd Apr 18, 2019
@@ -114,9 +118,15 @@ public IRubyObject evaluate(ThreadContext context, IRubyObject expr, IRubyObject
if (!handler.isNil()) {

This comment has been minimized.

Copy link
@jonathanhefner

jonathanhefner Apr 18, 2019

Author Contributor

This condition should now always be true (i.e. handler will always be an instance of XPathFunctions). Should I remove this if?

@flavorjones

This comment has been minimized.

Copy link
Member

flavorjones commented Apr 18, 2019

Thank you for submitting this PR! I'll take a look as soon as I can, this is a super-busy week for me.

@flavorjones flavorjones self-requested a review Dec 4, 2019
@flavorjones flavorjones self-assigned this Dec 4, 2019
@flavorjones

This comment has been minimized.

Copy link
Member

flavorjones commented Dec 4, 2019

Sorry this has gone so long without review; I'm tagging it for v1.11.0 which I'm hoping to release at the end of 2019, so will review in that timeframe.

@flavorjones flavorjones added this to the v1.11.0 milestone Dec 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.