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

fix(jruby): make less-naive inference of xpath custom function ns #2148

Merged
merged 3 commits into from
Dec 27, 2020

Conversation

flavorjones
Copy link
Member

What problem is this PR intended to solve?

Closes #1890
Related to #2147

JRuby's logic for inferring a namespace for the custom XPath function handler has been naive since it was introduced in 468c757. This PR makes it less naive and less likely to break things by looking for function calls in the query via regex, and looking for matches among the handler's declared Ruby methods.

Ideally we should require namespaces in both JRuby and CRuby implementations at some point to avoid having to do this kind of heavy lifting. See #2147 for a proposal.

Have you included adequate test coverage?

Sure.

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

Only changes the JRuby implementation, to do a better job of matching the CRuby behavior.

@flavorjones flavorjones added this to the v1.11.0 milestone Dec 26, 2020
@flavorjones flavorjones force-pushed the 1890-fix-jruby-xpath-namespace-inference branch from cb001f5 to e1461ce Compare December 26, 2020 21:39
@codeclimate
Copy link

codeclimate bot commented Dec 26, 2020

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

View more on Code Climate.

@flavorjones flavorjones merged commit a24d9d4 into master Dec 27, 2020
@flavorjones flavorjones deleted the 1890-fix-jruby-xpath-namespace-inference branch December 27, 2020 12:26
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

Successfully merging this pull request may close these issues.

1 participant