Include an explicit Combinator in the AST for implicit descendant

[flavorjones]

Making the descendant combinator explicit (where previously it was implicit by having two CompoundSelectors next to each other in a ComplexSelector) makes consuming the AST easier for visitors, and simplifies the selectors parser and its handling of whitespace.

This is an alternative to #60.

[flavorjones]

I also think this is preferable to #60 because it allows us to make this additional change:

diff --git a/lib/syntax_tree/css/selectors.rb b/lib/syntax_tree/css/selectors.rb
index f8dbe6c5..fbae220b 100644
--- a/lib/syntax_tree/css/selectors.rb
+++ b/lib/syntax_tree/css/selectors.rb
@@ -295,7 +295,11 @@ def initialize(tokens)
       end

       def parse
-        selector_list
+        selector_list.tap do
+          unless EOFToken === tokens.peek
+            raise ParseError.new "Unexpected token: #{tokens.peek.value.inspect}"
+          end
+        end
       end

and then a malformed selector like div::first-line.xl will result in an exception like:

SyntaxTree::CSS::Selectors::ParseError: Unexpected token: "."

The code on the branch in #60 will happily parse this as two compound selectors, [div::first-line, .xl], which is incorrect behavior IMHO.

[kddnewton]

So I think this is illustrating an issue with the design of ComplexSelector. Previously it was a left, a combinator, and a right. Now it's a flat list. This puts it on the consumer to understand what is implied by the flat list.

Thinking about html > body > main div I think I would prefer that to look like:

(complex-selector
  (complex-selector
    (complex-selector
      (type-selector (wqname (ident-token "html")))
      (combinator (delim-token ">"))
      (type-selector (wqname (ident-token "body")))
    )
    (combinator (delim-token ">"))
    (type-selector (wqname (ident-token "main")))
  )
  (combinator (whitespace-token " "))
  (type-selector (wqname (ident-token "div")))
)

This makes the hierarchy clear, and has the benefit of not requiring any parsing from the consumer. What do you think?

[flavorjones]

I think I would prefer that to look like ...

You said the opposite in #58 (comment) ! 😆

If you prefer the original way, I'm happy to roll that up into this PR, give me a little bit.

[kddnewton]

Shit, I totally did. Damnit. Sorry!

The other thing we could do which might make this nicer is we could define types for each combinator instead of having it all be the same thing.

So, DirectDescendentSelector and DescendentSelector and SiblingSelector and so on. That might be a better experience.

[flavorjones]

define types for each combinator

Yup, I will be sending a PR to do that, but I wanted to sort out this PR first ...

I do slightly prefer the flat list (with proper Combinator types) because there's no operator precedence to worry about that would require proper binary operators.

Do you want me to add a commit introducing Combinator types?

[kddnewton]

Ahh of course, that was what you were saying earlier. Sorry I must be slow today I'm finally catching up.

Yeah, let's look at what combinator types would look like. At the moment I'm mostly reacting to having to look at "WhitespaceToken" to understand how to handle it, which is making it feel bad.

[flavorjones]

@kddnewton OK, pushed a commit introducing the subclasses. LMK if you want a different approach (there are obviously alternatives to using constants the way I did)

[flavorjones]

I'm noticing now that all the combinator classes are dispatched with visit_combinator, LMK if you think it makes more sense to split it up into multiple methods.

[kddnewton]

Yeah this looks good. I think I'm going to change the PP_NAME to be a method call, but other than that I think this is good.