Separate interpolation from verbatim text #130
Conversation
TODO: * fix the remaining tests * apply the changes to embedded engines as well (??? this is a part of the public API so I'm not sure how to approach it yet) * remove the quoting regex
There was a problem hiding this comment.
😻 Awesome job, @little-bobby-tables!
There is one thing to discuss, I left a comment about interpolations in embedded engines.
src/slime_parser.peg.eex
Outdated
| @@ -91,13 +87,10 @@ text_block <- text_block_line (crlf | |||
| text_block_nested_lines <- text_block_line (crlf ( | |||
| indent lines:text_block_nested_lines dedent / lines:text_block_nested_lines | |||
| ))*; | |||
| text_block_line <- space? (dynamic:text_with_interpolation / simple:text); | |||
| text_block_line <- space? text_item*; | |||
There was a problem hiding this comment.
text_item only matches a non-empty string, which works well for e.g. inline_html where text is mandatory, but needs the optional modifier for empty string consumers like text_block_line.
lib/slime/parser/embedded_engine.ex
Outdated
|
|
||
| def parse(header, lines) do | ||
| case Regex.named_captures(@embedded_engine_regex, header) do | ||
| %{"engine" => engine, "indent" => indent} when engine in @registered_engines -> |
There was a problem hiding this comment.
Better to keep this check, it will be easier to add descriptive error in case of invalid engine name
lib/slime/parser/embedded_engine.ex
Outdated
|
|
||
| def render_with_engine(engine, lines) when is_list(lines) do | ||
| def render_with_engine(engine, line_contents) when is_list(line_contents) do | ||
| lines = Enum.map(line_contents, &compile/1) |
There was a problem hiding this comment.
Since engine developer should be aware of interpolation injections into engine body, I think it will be better to pass original list of raw text and interpolations in form of {:eex, text} instead of compiled version with interpolations presented as <%= text %>. For example in elixir engine interpolations should not be converted into eex injections at all. Also with this approach there is no need in compile here, because the result of embedded engine render will any way go through a compiler later.
There was a problem hiding this comment.
example for elixir engine, which now compiles into invalid eex:
elixir:
a = "test"
b = "b-#{a}"
= b
There was a problem hiding this comment.
Thanks! I didn't think about that; it certainly makes sense to decide on how to render interpolation on a per engine basis, as your example shows. I've added it as a test case.
On a somewhat unrelated note, this should probably be mentioned in the release notes — it seems to me the Markdown example won't work anymore.
| @@ -25,9 +27,6 @@ defmodule Slime.Parser.Transform do | |||
| "#" => %{attr: "id"} | |||
| }) | |||
|
|
|||
| # TODO: separate dynamic elixir blocks by parser | |||
| @quote_outside_interpolation_regex ~r/(^|\G)(?:\\.|[^#]|#(?!\{)|(?<pn>#\{(?:[^"}]++|"(?:\\.|[^"#]|#(?!\{)|(?&pn))*")*\}))*?\K"/u | |||
|
💥 Thank you @little-bobby-tables and @Rakoth — you guys are awesome 💜 @Rakoth I'd like to release a new version, did you see my comments here: #129? |
This is the last PR I've had in works, it fixes some long-outstanding issues like this and #119.
I've had to change the embedded engines code a bit, I've made sure that the tests are passing but they may not be covering all use cases so feedback on that is highly appreciated.