Improve selectors, fix complex selector parsing issues #58
Conversation
and improve its formatting
and improve their formatting
test/selectors_test.rb
Outdated
| assert_pattern do | ||
| actual => [ | ||
| Selectors::ComplexSelector[ | ||
| left: Selectors::TypeSelector[value: { name: { value: "section" } }], | ||
| combinator: { value: { value: ">" } }, | ||
| right: Selectors::ComplexSelector[ | ||
| left: Selectors::TypeSelector[value: { name: { value: "table" } }], | ||
| combinator: nil, | ||
| right: Selectors::TypeSelector[value: { name: { value: "tr" } }] | ||
| ] | ||
| ] | ||
| ] | ||
| end |
There was a problem hiding this comment.
After sleeping on it, I don't like this recursive structure. The spec defines a complex-selector as:
<complex-selector> = <compound-selector> [ <combinator>? <compound-selector> ]*
which would imply a preferred AST like:
assert_pattern do
actual => [
Selectors::ComplexSelector[
child_nodes: [
Selectors::TypeSelector[value: { name: { value: "section" } }],
Selectors::Combinator[value: { value: ">" }],
Selectors::TypeSelector[value: { name: { value: "table" } }],
Selectors::TypeSelector[value: { name: { value: "tr" } }]
]
]
]
endI'd like to change this, either in this PR or in the next one. @kddnewton thoughts?
There was a problem hiding this comment.
I've got a commit the converts to this flattened AST ... let me know what you want to do!
There was a problem hiding this comment.
OK, pushed a commit that flattens this AST, LMK what you think.
|
@kddnewton I know there's a lot of code here -- let me know if it needs explanation. The tests drove out all the changes I made, if that's helpful information. |
lib/syntax_tree/css/selectors.rb
Outdated
| end | ||
|
|
||
| # <relative-selector> = <combinator>? <complex-selector> | ||
| def relative_selector | ||
| combinator = maybe { combinator } | ||
| c = maybe { combinator } | ||
|
|
||
| if combinator |
There was a problem hiding this comment.
I think I'm not understanding this line. combinator here is changing from a local variable read to a method call. Is that intentional?
There was a problem hiding this comment.
As originally written, the outer variable combinator clobbers the method combinator and so the variable is always nil.
For example:
#! /usr/bin/env ruby
def foo
42
end
def bar(&block)
yield
end
foo = bar { foo }
foo
# => nilI'm happy to fix it some other way, this was just the simplest I came up with at the time.
There was a problem hiding this comment.
Sorry - I missed a replacement of combinator with c on the next line, will fix it.
There was a problem hiding this comment.
Pushed a complete changeset, let me know if it needs explanation.
test/selectors_test.rb
Outdated
| assert_pattern do | ||
| actual => [ | ||
| Selectors::ComplexSelector[ | ||
| left: Selectors::TypeSelector[value: { name: { value: "section" } }], | ||
| combinator: { value: { value: ">" } }, | ||
| right: Selectors::ComplexSelector[ | ||
| left: Selectors::TypeSelector[value: { name: { value: "table" } }], | ||
| combinator: nil, | ||
| right: Selectors::TypeSelector[value: { name: { value: "tr" } }] | ||
| ] | ||
| ] | ||
| ] | ||
| end |
|
Sorry it took so long to review! |
- avoid clobbering the combinator method with a local variable - return a recursive tree of complex selectors Partial fix for ruby-syntax-tree#25
flavorjones
force-pushed
the
flavorjones-work-on-selectors
branch
from
c78ef8c to
bb5fcc0
Compare
|
@kddnewton I've added two commits to:
Sorry, I know this is a lot of code but I wrote the flatten-ast commit on top of the formatting commit and rebasing to isolate the flatten-ast commit was going to be time consuming. |
Dipping my toes into contributing with this first PR, focused on improving selectors for potential use in Nokogiri.
Combinator,CompoundSelector, andComplexSelector.a + .b {}and.a > .bdon't work #25 describing selector parsing issuesI'd love feedback on whether I'm properly following any conventions you want followed.