Delegate tree building to the parser #128
Conversation
|
Thank you @little-bobby-tables! @Rakoth and/or myself will take a peek at this and let you know if we have any feedback 😁 |
There was a problem hiding this comment.
This is a great work done, @little-bobby-tables ! I left some comments, there are two about parser grammar, which we could open as separate issues, if you think it is not in scope of this changes.
lib/slime/parser/transform.ex
Outdated
| "/" -> {[], true} | ||
| "" -> {[], false} | ||
| [] -> {[], false} | ||
| (%EExNode{} = eex) -> {[eex], false} |
There was a problem hiding this comment.
We could use content variable here and in next match:
%EExNode{} -> {[content], false}
lib/slime/compiler.ex
Outdated
| body = cond do | ||
| tag.closed -> "<" <> tag_head <> "/>" | ||
| name in @void_elements -> "<" <> tag_head <> ">" | ||
| :otherwise -> "<" <> tag_head <> ">" |
There was a problem hiding this comment.
matter of taste, but I would prefer line-break after -> in this case
lib/slime/compiler.ex
Outdated
| |> Enum.map(fn branch -> render_branch(branch) end) | ||
| |> Enum.join(lines_sep) | ||
| def compile([]), do: "" | ||
| def compile([h | tree]) do |
There was a problem hiding this comment.
guard will do better here compile(tags) when is_list(tags)
src/slime_parser.peg.eex
Outdated
|
|
||
| % NOTE: Leading whitespace is handled separately for comments and verbatim text | ||
| tag <- comment / verbatim_text / space? tag_item / blank:(space? &eol); | ||
| tags <- (tag crlf*)+; |
There was a problem hiding this comment.
Should we order rules as they appear in grammar, from top to bottom for easier reading?
|
|
||
| tag_item <- (embedded_engine / inline_html / code / html_tag); | ||
| inline_html <- &'<' text_content nested_tags?; |
There was a problem hiding this comment.
Am I missing something, or html with nested children should always have a closing inline html part, so the following examples should raise a parsing error.
<img />
| test
<div>
| test
</div>
| test 1
There was a problem hiding this comment.
Slim does not check for the closing tag. It's an interesting idea — it could be handled similar to if-else — but I think it is out of scope of this PR, yes.
src/slime_parser.peg.eex
Outdated
| code <- inline:('=' '='? tag_spaces? / '-') space? code:code_lines; | ||
| code_lines <- code_line / code_line_with_brake crlf (indent code_lines dedent / code_lines); | ||
| simple_tag <- tag:tag_shortcut spaces:tag_spaces space? | ||
| content:attributes_with_content children:nested_tags?; |
There was a problem hiding this comment.
There are some invalid test cases, currently supported by accident. In ruby-slim tag with inline content could not have nested children tags, (if there are nested lines, they treated as inline content continuation). In some cases it as already not handled by slime, so I think it is time to fix this behaviour, or at least have a parsing error for this case.
For example:
div test
test
in slim renders to
<div>test
test</div>
|
There is also some problem with code continuation lines: raises a parser error about unexpected dedent on line 3 |
Inline tags in Slim can have attributes, both normal and wrapped.
|
@Rakoth, I think I've addressed the issues you've brought up. I couldn't pinpoint exactly why the code continuation rule was failing; I believe it was due to some other rule that used to consume dedents past it, but I'm not sure. Either way, since the preprocessor already skips indentation for lines broken by a backslash, I've just added commas to the regular expression there and that seemed to solve it. I also haven't replied to your comment about grammar reordering. I don't feel strongly about it, it's probably better to do it outside of this changeset. @doomspork: the PR now includes a breaking change concerning the behavior of nodes with inline content. What used to be rendered as nested tags: is now rendered as text, so as to fit this example from the Slim documentation: WDYT? |
|
@doomspork I think we good to merge this PR, any objections? |
|
No sir @Rakoth! This looks good to me. Thank you again @little-bobby-tables, this is great! |
Removing the intermediate representation reduces the complexity of the parser and makes it easier to perform line-independent transforms (handling
if-else, for instance).I had to add trailing whitespace removal to the preprocessor because I couldn't handle certain edge cases like "blank lines after if-else" (test/rendering_test.ex) without severely complicating the grammar.