Skip to content

Implement XPath abbreviate of parentheses and fix some bugs#328

Open
tompng wants to merge 1 commit into
ruby:masterfrom
tompng:xpath_parentheses_abbreviate
Open

Implement XPath abbreviate of parentheses and fix some bugs#328
tompng wants to merge 1 commit into
ruby:masterfrom
tompng:xpath_parentheses_abbreviate

Conversation

@tompng
Copy link
Copy Markdown
Member

@tompng tompng commented Jun 6, 2026

Implement abbreviate of parenthesized :group added in #317

Also fixes these bugs so that we can test parenthesized xpaths such as (1+2)*3

parser = REXML::Parsers::XPathParser.new
parser.abbreviate parser.parse("count(/a)") #=> NoMethodError
parser.abbreviate parser.parse("-1") #=> NoMethodError
parser.abbreviate parser.parse("1+2") #=> NoMethodError
parser.abbreviate parser.parse("/a[1+2]") #=> "/a[1 plus 2]" (XPath doesn't have `plus` keyword)

Copilot AI review requested due to automatic review settings June 6, 2026 09:14
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Extends the XPath abbreviation logic to cover additional XPath expression node types (arithmetic, grouping, unary negation) and adds tests to validate the new behavior.

Changes:

  • Added parser tests for literals, unary/binary operators, grouping/parentheses, functions, and unknown nodes.
  • Updated abbreviate to delegate more node types to predicate_to_path.
  • Expanded predicate_to_path to format +, -, *, unary negation, and grouping nodes.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File Description
test/parser/test_xpath.rb Adds test coverage for arithmetic/grouping/function formatting and unknown-node handling.
lib/rexml/parsers/xpathparser.rb Updates abbreviation/serialization logic to support more parsed XPath node types.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +207 to +210
when :neg
parsed.shift
path << "-"
path << predicate_to_path( parsed, &block )
Comment thread test/parser/test_xpath.rb Outdated
Comment on lines +60 to +62
def test_unknown_not_infinitly_recursing
assert_instance_of(String, abbreviate([:unknown]))
end
Comment on lines +106 to +110
when :function, :literal, :group, :neg, :and, :or, :mult, :plus, :minus, :neq, :eq, :lt, :gt, :lteq, :gteq, :div, :mod, :union
component = ''
components << component
parsed.unshift(op)
component << predicate_to_path(parsed) {|x| abbreviate(x)}
component << " )"
when :literal
component << quote_literal(parsed.shift)
when :function, :literal, :group, :neg, :and, :or, :mult, :plus, :minus, :neq, :eq, :lt, :gt, :lteq, :gteq, :div, :mod, :union
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not good, but we need to do this to avoid infinite recursion of unknown operator.
predicate_to_path calls abbreviate if predicate_to_path doesn't know the operator.

Some possible workaround for this may break XPathParser#expand which is not tested and the spec is unknown.

@tompng tompng force-pushed the xpath_parentheses_abbreviate branch from b6a2707 to 7487772 Compare June 6, 2026 09:18
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.

2 participants