Multiline comments #126
Multiline comments #126
Conversation
There was a problem hiding this comment.
Thanks, @little-bobby-tables, great job refactoring this nasty verbatim_text transform! I left some comments below, please take a look.
lib/slime/parser/text_block.ex
Outdated
| ^ | ||
| declaration indent | ||
| """ | ||
| def render(lines, decl_indent, trailing_whitespace \\ "") do |
There was a problem hiding this comment.
May be use declaration_indent as variable name? An abbreviation is a bit confusing.
lib/slime/parser/text_block.ex
Outdated
| [{first_line_indent, first_line, is_eex_line} | rest] = lines | ||
|
|
||
| text_indent = if first_line == "" do | ||
| [{indent, _, _} | _] = rest |
There was a problem hiding this comment.
what about single ' used for spacing, example similar to one in my work project:
p
'
span text
' .
I understand, span< could be used here, but it is a valid slim and should be supported (probably should add a test case for it)
src/slime_parser.peg.eex
Outdated
| % NOTE: We have to track leading spaces for verbatim_text, so it handled separately | ||
| tag <- verbatim_text / space? tag_item / blank:(space? &eol); | ||
| % NOTE: Leading whitespace is handled separately for text items | ||
| tag <- text_item / space? tag_item / blank:(space? &eol); |
There was a problem hiding this comment.
I think it is better to keep verbatim_text separated from comment, so here it would be comment / verbatim_text / ...
|
Thanks for your comments, @Rakoth! I've added a test case for However, I've noticed that credo now reports the TextBlock.render function as having excessively high ABC complexity — do you have any ideas on how to refactor it? |
|
@little-bobby-tables Since it is already in a separate module, we could simply extract parts of it into private functions. |
I've noticed that Slim handles HTML comments (
/!) just like verbatim text nodes (|and') — complete with interpolation — so I decided to move the common semantics into atext_blockrule.The change introduces:
/!)/)What do you think?