Primitive value should't be wrapped in an array#326
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adjusts XPath evaluation return values and XPath function outputs for variable resolution and node naming/namespace helpers.
Changes:
- Changes variable expression evaluation to return the variable value directly (instead of wrapping in an Array).
- Updates
namespace_uriandnameXPath functions to early-return values from withinget_namespaceblocks and adds""fallbacks.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| lib/rexml/xpath_parser.rb | Changes the return type of variable expression evaluation. |
| lib/rexml/functions.rb | Reworks namespace_uri/name to return directly from block and adds empty-string fallbacks. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| when :variable | ||
| var_name = path_stack.shift | ||
| return [@variables[var_name]] | ||
| return @variables[var_name] |
| get_namespace( node_set ) do |node| | ||
| return node.namespace | ||
| end | ||
| "" |
tompng
force-pushed
the
primitive_array_bugfix
branch
from
542bd9d to
6189869
Compare
| when :variable | ||
| var_name = path_stack.shift | ||
| return [@variables[var_name]] | ||
| return @variables[var_name] |
| result = expr(expression.dclone, [node], subcontext) | ||
| trace(:predicate_evaluate, expression, node, subcontext, result) if @debug | ||
| result = result[0] if result.kind_of? Array and result.length == 1 | ||
| if result.kind_of? Numeric | ||
| if result == position | ||
| new_nodeset << node | ||
| end | ||
| elsif result.instance_of? Array | ||
| if result.size > 0 and result.inject(false) {|k,s| s or k} | ||
| if result.size > 0 | ||
| new_nodeset << node | ||
| end | ||
| if result.size > 0 | ||
| new_nodeset << node | ||
| end |
| get_namespace( node_set ) do |node| | ||
| return node.namespace | ||
| end | ||
| "" |
| get_namespace( node_set ) do |node| | ||
| node.expanded_name | ||
| return node.expanded_name | ||
| end | ||
| "" |
| when :variable | ||
| var_name = path_stack.shift | ||
| return [@variables[var_name]] | ||
| return @variables[var_name] |
There was a problem hiding this comment.
Returning nil may cause unintentional behavior. However, it was returning [nil] before, and it may also cause problem, so basically nothing has changed.
In XPath 1.0, only string | number | boolean | nodeset are allowed.
In Nokogiri, using undefined variable will raise Undefined variable: $foo (Nokogiri::XML::XPath::SyntaxError).
In JavaScript's document.execute, all variables are bound to empty string.
Ideally, REXML should validate variables somewhere, and need to decide how to handle undefined variables: fallback or raise. Although changing/deciding this may be out of scope of this pull request.
Ref: https://www.w3.org/TR/1999/REC-xpath-19991116/#section-Basics
A VariableReference evaluates to the value to which the variable name is bound in the set of variable bindings in the context. It is an error if the variable name is not bound to any value in the set of variable bindings in the expression context.
Related to #326 Functions must return nodeset or a primitive value. Functions::text() returning an array of string is a bug to be fixed. Although, `text()` in xpath is resolved as `[:child, :text]`, it's not a function, so Functions::text is unused. Also removes `Functions::processing_instruction`. These are node-test, not functions.
Similar to #324
Values should be nodeset(Array) or primitive, not
[primitive]Also removes these workarounds related to primitive wrapped in array
It works even wrapping with an array, but leaving it will be a blocker of implementing delayed nodeset ordering.