Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork1
Improve selectors, fix complex selector parsing issues#58
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Improve selectors, fix complex selector parsing issues#58
Uh oh!
There was an error while loading.Please reload this page.
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 |
flavorjonesMay 26, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
After sleeping on it, I don't like this recursive structure. Thespec defines acomplex-selector as:
<complex-selector> = <compound-selector> [ <combinator>? <compound-selector> ]*which would imply a preferred AST like:
assert_patterndoactual=>[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"}}]]]]end
I'd like to change this, either in this PR or in the next one.@kddnewton thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Flatter is better for sure
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
As originally written, the outer variablecombinator clobbers the methodcombinator and so the variable is alwaysnil.
For example:
#! /usr/bin/env rubydeffoo42enddefbar(&block)yieldendfoo=bar{foo}foo# => nil
I'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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Sorry - I missed a replacement ofcombinator withc on the next line, will fix it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Flatter is better for sure
Sorry it took so long to review! |
- avoid clobbering the combinator method with a local variable- return a recursive tree of complex selectorsPartial fix forruby-syntax-tree#25
c78ef8c tobb5fcc0Compareflavorjones commentedJun 2, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@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. |
eb3e2a7 intoruby-syntax-tree:mainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
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.