Skip to content

move prefix and ns into XPathVisitor#3225

Merged
flavorjones merged 4 commits intomainfrom
flavorjones-move-prefix-and-ns-into-xpath-visitor
Jun 11, 2024
Merged

move prefix and ns into XPathVisitor#3225
flavorjones merged 4 commits intomainfrom
flavorjones-move-prefix-and-ns-into-xpath-visitor

Conversation

@flavorjones
Copy link
Member

What problem is this PR intended to solve?

Refactoring the CSS parser internals a bit to separate concerns between the parser and the xpath visitor.

  • move prefix into the XPathVisitor (from the AST xpath_for method)
  • move namespaces into the XPathVisitor (from the Parser constructor)
  • update the CSS selector caching mechanism
  • CSS.xpath_for now prefers keyword arguments, and warns on options hash

Have you included adequate test coverage?

This is mostly a refactor of internals, existing test coverage is mostly sufficient, and I added some tests for edge cases and new attributes.

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

N/A

Previously we've been handling it as a separate input to the
CSS.xpath_for method. This simplifies the code and the parser cache
key.
Previously this was passed into CSS::Parser.new and used during the
parsing phase. Now it's passed into the visitor and we do namespace
munging there, where it should be.

Note the namespaces has also been moved out of the top-level parser
cache key into XPathVisitor#config.

Finally, note that namespaces is nil by default (instead of being an
empty Hash) which should prevent some unnecessary object creation in
the most common use cases.
@flavorjones flavorjones force-pushed the flavorjones-move-prefix-and-ns-into-xpath-visitor branch from 35ec2cf to 99be08f Compare June 11, 2024 17:00
@flavorjones flavorjones merged commit 396fa3f into main Jun 11, 2024
@flavorjones flavorjones deleted the flavorjones-move-prefix-and-ns-into-xpath-visitor branch June 11, 2024 17:43
@flavorjones flavorjones added this to the v1.17.0 milestone Jun 14, 2024
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