From e87e0746073671e2a9ca11b6dfc8bc9fc8416057 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomasz=20Szcz=C4=99=C5=9Bniak-Szlagowski?= Date: Sat, 14 Sep 2024 19:56:13 +0900 Subject: [PATCH] Support for more syntax and better multiline ERB formatting (#92) * Several improvements - Fix alignment in multiline Ruby, at the cost of making the code less concise - Don't break the line between "<% if" and the condition - Support <% when 1, 2 %> The most controversial change is <%= long_command( ... ) %> instead of <%= long_command( ... ) %> * Support
> --- lib/syntax_tree/erb/format.rb | 227 +++++++++++++----- lib/syntax_tree/erb/nodes.rb | 9 +- lib/syntax_tree/erb/parser.rb | 26 +- lib/syntax_tree/erb/pretty_print.rb | 3 +- test/erb_test.rb | 83 ++++++- test/fixture/block_formatted.html.erb | 10 +- test/fixture/case_formatted.html.erb | 2 + test/fixture/case_unformatted.html.erb | 2 + .../erb_inside_html_tag_formatted.html.erb | 4 + .../erb_inside_html_tag_unformatted.html.erb | 10 + test/fixture/erb_syntax_formatted.html.erb | 24 +- test/fixture/if_statements_formatted.html.erb | 14 +- .../if_statements_unformatted.html.erb | 2 + test/fixture/layout_formatted.html.erb | 24 +- test/fixture/nested_html_formatted.html.erb | 4 +- .../fixture/without_parens_formatted.html.erb | 23 ++ .../without_parens_unformatted.html.erb | 5 + test/formatting_test.rb | 8 + 18 files changed, 369 insertions(+), 111 deletions(-) create mode 100644 test/fixture/erb_inside_html_tag_formatted.html.erb create mode 100644 test/fixture/erb_inside_html_tag_unformatted.html.erb create mode 100644 test/fixture/without_parens_formatted.html.erb create mode 100644 test/fixture/without_parens_unformatted.html.erb diff --git a/lib/syntax_tree/erb/format.rb b/lib/syntax_tree/erb/format.rb index 66dd397..d5e8b18 100644 --- a/lib/syntax_tree/erb/format.rb +++ b/lib/syntax_tree/erb/format.rb @@ -7,6 +7,7 @@ class Format < Visitor def initialize(q) @q = q + @inside_html_attributes = false end # Visit a Token node. @@ -28,20 +29,33 @@ def visit_document(node) q.breakable(force: true) end - def visit_block(node) - visit(node.opening) + # Dependent block is one that follows after a "main one", e.g. <% else %> + def visit_block(node, dependent: false) + process = + proc do + visit(node.opening) - breakable = breakable_inside(node) - if node.elements.any? - q.indent do - q.breakable if breakable - handle_child_nodes(node.elements) + breakable = breakable_inside(node) + if node.elements.any? + q.indent do + q.breakable("") if breakable + handle_child_nodes(node.elements) + end + end + + if node.closing + q.breakable("") if breakable + visit(node.closing) + end end - end - if node.closing - q.breakable("") if breakable - visit(node.closing) + if dependent + process.call + else + q.group do + q.break_parent unless @inside_html_attributes + process.call + end end end @@ -92,11 +106,11 @@ def visit_erb_if(node) end def visit_erb_elsif(node) - visit_block(node) + visit_block(node, dependent: true) end def visit_erb_else(node) - visit_block(node) + visit_block(node, dependent: true) end def visit_erb_case(node) @@ -104,27 +118,42 @@ def visit_erb_case(node) end def visit_erb_case_when(node) - visit_block(node) + visit_block(node, dependent: true) end # Visit an ErbNode node. def visit_erb(node) visit(node.opening_tag) - if node.keyword - q.text(" ") - visit(node.keyword) + q.group do + if !node.keyword && node.content.blank? + q.text(" ") + elsif node.keyword && node.content.blank? + q.text(" ") + visit(node.keyword) + q.text(" ") + else + visit_erb_content(node.content, keyword: node.keyword) + q.breakable unless node.closing_tag.is_a?(ErbDoClose) + end end - node.content.blank? ? q.text(" ") : visit(node.content) - visit(node.closing_tag) end def visit_erb_do_close(node) closing = node.closing.value.end_with?("-%>") ? "-%>" : "%>" - q.text(node.closing.value.gsub(closing, "").rstrip) - q.text(" ") + # Append the "do" at the end of Ruby code (within the same group) + last_erb_content_group = q.current_group.contents.last + last_erb_content_indent = last_erb_content_group.contents.last + q.with_target(last_erb_content_indent.contents) do + q.text(" ") + q.text(node.closing.value.gsub(closing, "").rstrip) + end + + # Add a breakable space after the indent, but within the same group + q.with_target(last_erb_content_group.contents) { q.breakable } + q.text(closing) end @@ -145,55 +174,26 @@ def visit_erb_end(node) visit(node.closing_tag) end - def visit_erb_content(node) + def visit_erb_content(node, keyword: nil) # Reject all VoidStmt to avoid empty lines - nodes = - (node.value&.statements&.child_nodes || []).reject do |node| - node.is_a?(SyntaxTree::VoidStmt) - end - - if nodes.size == 1 - q.text(" ") - format_statement(nodes.first) - q.text(" ") - elsif nodes.size > 1 - q.indent do - q.breakable("") - q.seplist(nodes, -> { q.breakable("") }) do |child_node| - format_statement(child_node) - end - end + nodes = child_nodes_without_void_statements(node) + return if nodes.empty? + q.indent do q.breakable - end - end - - def format_statement(statement) - formatter = - SyntaxTree::Formatter.new("", [], SyntaxTree::ERB::MAX_WIDTH) - - formatter.format(statement) - formatter.flush - - formatted = - formatter.output.join.gsub( - SyntaxTree::ERB::ErbYield::PLACEHOLDER, - "yield" - ) - - output_rows(formatted.split("\n")) - end - - def output_rows(rows) - if rows.size > 1 - q.seplist(rows, -> { q.breakable("") }) { |row| q.text(row) } - elsif rows.size == 1 - q.text(rows.first) + q.seplist(nodes, -> { q.breakable(force: true) }) do |child_node| + code = + format_statement_with_keyword_prefix(child_node, keyword: keyword) + output_rows(code.split("\n")) + # Pass the keyword only to the first child node + keyword = nil + end end end # Visit an HtmlNode::OpeningTag node. def visit_opening_tag(node) + @inside_html_attributes = true q.group do visit(node.opening) visit(node.name) @@ -219,6 +219,7 @@ def visit_opening_tag(node) visit(node.closing) end + @inside_html_attributes = false end # Visit an HtmlNode::ClosingTag node. @@ -383,6 +384,108 @@ def node_should_group(node) node.is_a?(SyntaxTree::ERB::CharData) || node.is_a?(SyntaxTree::ERB::ErbNode) end + + def child_nodes_without_void_statements(node) + (node.value&.statements&.child_nodes || []).reject do |node| + node.is_a?(SyntaxTree::VoidStmt) + end + end + + def format_statement_with_keyword_prefix(statement, keyword: nil) + case keyword&.value + when nil + format_statement(statement) + when "if" + statement = + SyntaxTree::IfNode.new( + predicate: statement, + statements: void_body, + consequent: nil, + location: keyword.location + ) + format_statement(statement).delete_suffix("\nend") + when "unless" + statement = + SyntaxTree::UnlessNode.new( + predicate: statement, + statements: void_body, + consequent: nil, + location: keyword.location + ) + format_statement(statement).delete_suffix("\nend") + when "elsif" + statement = + SyntaxTree::Elsif.new( + predicate: statement, + statements: void_body, + consequent: nil, + location: keyword.location + ) + format_statement(statement).delete_suffix("\nend") + when "case" + statement = + SyntaxTree::Case.new( + keyword: + SyntaxTree::Kw.new(value: "case", location: keyword.location), + value: statement, + consequent: void_body, + location: keyword.location + ) + format_statement(statement).delete_suffix("\nend") + when "when" + statement = + SyntaxTree::When.new( + arguments: statement.contents, + statements: void_body, + consequent: nil, + location: keyword.location + ) + format_statement(statement).delete_suffix("\nend") + else + q.text(keyword.value) + q.breakable + format_statement(statement) + end + end + + def format_statement(statement) + formatter = + SyntaxTree::Formatter.new("", [], SyntaxTree::ERB::MAX_WIDTH) + + formatter.format(statement) + formatter.flush + + formatter.output.join.gsub( + SyntaxTree::ERB::ErbYield::PLACEHOLDER, + "yield" + ) + end + + def output_rows(rows) + if rows.size > 1 + q.seplist(rows, -> { q.breakable(force: true) }) { |row| q.text(row) } + elsif rows.size == 1 + q.text(rows.first) + end + end + + def fake_location + Location.new( + start_line: 0, + start_char: 0, + start_column: 0, + end_line: 0, + end_char: 0, + end_column: 0 + ) + end + + def void_body + SyntaxTree::Statements.new( + body: [SyntaxTree::VoidStmt.new(location: fake_location)], + location: fake_location + ) + end end end end diff --git a/lib/syntax_tree/erb/nodes.rb b/lib/syntax_tree/erb/nodes.rb index 570f0ec..da3158f 100644 --- a/lib/syntax_tree/erb/nodes.rb +++ b/lib/syntax_tree/erb/nodes.rb @@ -322,10 +322,15 @@ def prepare_content(content) if content.is_a?(ErbContent) content else - # Set content to nil if it is empty content ||= [] - ErbContent.new(value: content) + if !content.empty? && keyword&.value == "when" + # "when" accepts multiple comma-separated arguments, so let's try + # to make them parsable. + ErbContent.new(value: ["[", *content, "]"]) + else + ErbContent.new(value: content) + end end rescue SyntaxTree::Parser::ParseError # Try to add the keyword to see if it parses diff --git a/lib/syntax_tree/erb/parser.rb b/lib/syntax_tree/erb/parser.rb index eca23bf..0226dfd 100644 --- a/lib/syntax_tree/erb/parser.rb +++ b/lib/syntax_tree/erb/parser.rb @@ -19,6 +19,7 @@ def initialize(source) @source = source @tokens = make_tokens @found_doctype = false + @erb_context = :outside end def parse @@ -300,7 +301,7 @@ def make_tokens # strong, vue-component-kebab, VueComponentPascal # abc, #abc, @abc, :abc enum.yield :name, $&, index, line, column_index - when /\A<%/ + when /\A<%={1,2}/, /\A<%-/, /\A<%/ # the beginning of an ERB tag # <% enum.yield :erb_open, $&, index, line, column_index @@ -419,7 +420,20 @@ def parse_until_erb(classes:) items = [] loop do - result = parse_any_tag + result = + case @erb_context + when :string + atleast do + maybe { consume(:text) } || maybe { consume(:whitespace) } || + maybe { parse_erb_tag } + end + when :inside + atleast do + maybe { parse_erb_tag } || maybe { parse_html_attribute } + end + when :outside + parse_any_tag + end items << result break if classes.any? { |cls| result.is_a?(cls) } end @@ -441,6 +455,8 @@ def parse_html_opening_tag ) end + @erb_context = :inside + attributes = many do atleast do @@ -448,6 +464,8 @@ def parse_html_opening_tag end end + @erb_context = :outside + closing = atleast do maybe { consume(:close) } || maybe { consume(:slash_close) } @@ -812,6 +830,8 @@ def parse_html_string ) end + @erb_context = :string + contents = many do atleast do @@ -820,6 +840,8 @@ def parse_html_string end end + @erb_context = :inside + closing = if opening.type == :string_open_double_quote consume(:string_close_double_quote) diff --git a/lib/syntax_tree/erb/pretty_print.rb b/lib/syntax_tree/erb/pretty_print.rb index 0a67a91..529ddf2 100644 --- a/lib/syntax_tree/erb/pretty_print.rb +++ b/lib/syntax_tree/erb/pretty_print.rb @@ -51,7 +51,7 @@ def visit_erb(node) end if node.content q.breakable - q.text("content") + visit(node.content) end q.breakable @@ -67,6 +67,7 @@ def visit_erb_block(node) q.text("(erb_block") q.nest(2) do q.breakable + visit(node.opening) q.seplist(node.elements) { |child_node| visit(child_node) } end q.breakable diff --git a/test/erb_test.rb b/test/erb_test.rb index 9b11021..b671bfe 100644 --- a/test/erb_test.rb +++ b/test/erb_test.rb @@ -111,15 +111,49 @@ def test_if_and_end_in_same_output_tag_short def test_if_and_end_in_same_tag source = "Hello\n<% if true then this elsif false then that else maybe end %>\n

Hey

" - expected = - "Hello\n<% if true\n this\nelsif false\n that\nelse\n maybe\nend %>\n

Hey

\n" + expected = <<~EXPECTED + Hello + <% + if true + this + elsif false + that + else + maybe + end + %> +

Hey

+ EXPECTED + + assert_formatting(source, expected) + end + + def test_erb_output_inside_html_tag + source = "
>
" + expected = "
>
\n" assert_formatting(source, expected) end - def test_erb_inside_html_tag - source = "
>
" - expected = "
>
\n" + def test_erb_if_inside_html_tag + source = "
b<% else %><%= c %><% end %>>
" + expected = "
b<% else %><%= c %><% end %>>
\n" + + assert_formatting(source, expected) + end + + def test_erb_output_inside_html_attribute_value + source = "
" + expected = "
\">
\n" + + assert_formatting(source, expected) + end + + def test_erb_if_inside_html_attribute_value + source = + "
" + expected = + "
b<% else %><%= c %><% end %>\">
\n" assert_formatting(source, expected) end @@ -127,8 +161,13 @@ def test_erb_inside_html_tag def test_long_if_statement source = "<%=number_to_percentage(@reports&.first&.stability*100,precision: 1) if @reports&.first&.other&.stronger&.longer %>" - expected = - "<%= if @reports&.first&.other&.stronger&.longer\n number_to_percentage(@reports&.first&.stability * 100, precision: 1)\nend %>\n" + expected = <<~EXPECTED + <%= + if @reports&.first&.other&.stronger&.longer + number_to_percentage(@reports&.first&.stability * 100, precision: 1) + end + %> + EXPECTED assert_formatting(source, expected) end @@ -136,8 +175,15 @@ def test_long_if_statement def test_erb_else_if_statement source = "<%if this%>\n

A

\n<%elsif that%>\n

B

\n<%else%>\n

C

\n<%end%>" - expected = - "<% if this %>\n

A

\n<% elsif that %>\n

B

\n<% else %>\n

C

\n<% end %>\n" + expected = <<~EXPECTED + <% if this %> +

A

+ <% elsif that %> +

B

+ <% else %> +

C

+ <% end %> + EXPECTED assert_formatting(source, expected) end @@ -145,8 +191,14 @@ def test_erb_else_if_statement def test_long_ternary source = "<%= number_to_percentage(@reports&.first&.stability * 100, precision: @reports&.first&.stability ? 'Stable' : 'Unstable') %>" - expected = - "<%= number_to_percentage(\n @reports&.first&.stability * 100,\n precision: @reports&.first&.stability ? \"Stable\" : \"Unstable\"\n) %>\n" + expected = <<~EXPECTED + <%= + number_to_percentage( + @reports&.first&.stability * 100, + precision: @reports&.first&.stability ? "Stable" : "Unstable" + ) + %> + EXPECTED assert_formatting(source, expected) end @@ -190,8 +242,13 @@ def test_erb_multiline_comment def test_erb_ternary_as_argument_without_parentheses source = "<%= f.submit( f.object.id.present? ? t('buttons.titles.save'):t('buttons.titles.create')) %>" - expected = - "<%= f.submit(\n f.object.id.present? ? t(\"buttons.titles.save\") : t(\"buttons.titles.create\")\n) %>\n" + expected = <<~EXPECTED + <%= + f.submit( + f.object.id.present? ? t("buttons.titles.save") : t("buttons.titles.create") + ) + %> + EXPECTED assert_formatting(source, expected) end diff --git a/test/fixture/block_formatted.html.erb b/test/fixture/block_formatted.html.erb index bed1d7f..0454346 100644 --- a/test/fixture/block_formatted.html.erb +++ b/test/fixture/block_formatted.html.erb @@ -4,10 +4,12 @@ <%= form.text_field(:name, class: "form-control") %>
- <%= form.submit( - "Very very very very very long text", - class: "btn btn-primary btn btn-primary btn btn-primary btn btn-primary" - ) %> + <%= + form.submit( + "Very very very very very long text", + class: "btn btn-primary btn btn-primary btn btn-primary btn btn-primary" + ) + %> <% end %> <%= link_to(dont_replace, what_to_do, class: "do |what,bad|") do |hello| %> diff --git a/test/fixture/case_formatted.html.erb b/test/fixture/case_formatted.html.erb index 3db1640..6346584 100644 --- a/test/fixture/case_formatted.html.erb +++ b/test/fixture/case_formatted.html.erb @@ -5,6 +5,8 @@ This is the second case. <% when 3 %> This is the third case. +<% when 4, 5 %> + This is the fourth and fifth case. <% else %> This is the default case. <% end %> diff --git a/test/fixture/case_unformatted.html.erb b/test/fixture/case_unformatted.html.erb index cdb95c0..3bc8ac8 100644 --- a/test/fixture/case_unformatted.html.erb +++ b/test/fixture/case_unformatted.html.erb @@ -2,6 +2,8 @@ This is the second case. <% when 3%> This is the third case. + <% when 4, 5 %> + This is the fourth and fifth case. <% else %> This is the default case. <% end%> diff --git a/test/fixture/erb_inside_html_tag_formatted.html.erb b/test/fixture/erb_inside_html_tag_formatted.html.erb new file mode 100644 index 0000000..abf8cf9 --- /dev/null +++ b/test/fixture/erb_inside_html_tag_formatted.html.erb @@ -0,0 +1,4 @@ +
+ /> + /> +disabled<% end %> /> diff --git a/test/fixture/erb_inside_html_tag_unformatted.html.erb b/test/fixture/erb_inside_html_tag_unformatted.html.erb new file mode 100644 index 0000000..8a80093 --- /dev/null +++ b/test/fixture/erb_inside_html_tag_unformatted.html.erb @@ -0,0 +1,10 @@ +
+> + +/> +disabled<% end %> +> diff --git a/test/fixture/erb_syntax_formatted.html.erb b/test/fixture/erb_syntax_formatted.html.erb index 835750a..293c36d 100644 --- a/test/fixture/erb_syntax_formatted.html.erb +++ b/test/fixture/erb_syntax_formatted.html.erb @@ -29,17 +29,19 @@ Treat it as a comment

Cool

<%- end %> -<%= t( - ".verified_at", - at: - ( - if @repository.github_status_at - l(@repository.github_status_at, format: :long) - else - "?" - end - ) -) %> +<%= + t( + ".verified_at", + at: + ( + if @repository.github_status_at + l(@repository.github_status_at, format: :long) + else + "?" + end + ) + ) +%> <% assign_b ||= "b" diff --git a/test/fixture/if_statements_formatted.html.erb b/test/fixture/if_statements_formatted.html.erb index 2e7a9d3..93ddbbf 100644 --- a/test/fixture/if_statements_formatted.html.erb +++ b/test/fixture/if_statements_formatted.html.erb @@ -19,8 +19,12 @@ <% end %> -<%= if @model - @model.name -else - t("views.more_than_80_characters_long_row.categories.shared.version.default") -end %> +<%= + if @model + @model.name + else + t("views.more_than_80_characters_long_row.categories.shared.version.default") + end +%> + +
diff --git a/test/fixture/if_statements_unformatted.html.erb b/test/fixture/if_statements_unformatted.html.erb index e0d28b4..335947a 100644 --- a/test/fixture/if_statements_unformatted.html.erb +++ b/test/fixture/if_statements_unformatted.html.erb @@ -6,3 +6,5 @@ <% end %> <%= @model ? @model.name : t("views.more_than_80_characters_long_row.categories.shared.version.default") %> + +
diff --git a/test/fixture/layout_formatted.html.erb b/test/fixture/layout_formatted.html.erb index 1607c7c..d50a984 100644 --- a/test/fixture/layout_formatted.html.erb +++ b/test/fixture/layout_formatted.html.erb @@ -11,16 +11,20 @@ rel="stylesheet" /> <%= full_title(t("general.title"), yield(:title)) %> - <%= stylesheet_link_tag( - "application", - media: "all", - "data-turbolinks-track": "reload" - ) %> - <%= javascript_include_tag( - "application", - "data-turbolinks-track": "reload", - defer: true - ) %> + <%= + stylesheet_link_tag( + "application", + media: "all", + "data-turbolinks-track": "reload" + ) + %> + <%= + javascript_include_tag( + "application", + "data-turbolinks-track": "reload", + defer: true + ) + %> <%= csrf_meta_tags %> <%= csp_meta_tag %> <%= render("application/favicon") %> diff --git a/test/fixture/nested_html_formatted.html.erb b/test/fixture/nested_html_formatted.html.erb index 1322953..56342ba 100644 --- a/test/fixture/nested_html_formatted.html.erb +++ b/test/fixture/nested_html_formatted.html.erb @@ -1,5 +1,7 @@
- <%= t(".title") + " " + t(".description") + " " + t(".pretty_long") %>'This is a single quote' + <%= + t(".title") + " " + t(".description") + " " + t(".pretty_long") + %>'This is a single quote' "This is a double quote"
diff --git a/test/fixture/without_parens_formatted.html.erb b/test/fixture/without_parens_formatted.html.erb new file mode 100644 index 0000000..0dced6a --- /dev/null +++ b/test/fixture/without_parens_formatted.html.erb @@ -0,0 +1,23 @@ +<%= this_is "short", "enough" %> +<% + if condition? this_is_short + x + else + y + end +%> +<% + if condition? this_is_pretty_long, + so_we_probably, + cant_fit_all_arguments, + in_one_line +%> + <%= + render "some_partial", + param_one: 1, + param_two: 2, + param_three: 3, + param_four: 4, + param_five: 5 + %> +<% end %> diff --git a/test/fixture/without_parens_unformatted.html.erb b/test/fixture/without_parens_unformatted.html.erb new file mode 100644 index 0000000..6d84403 --- /dev/null +++ b/test/fixture/without_parens_unformatted.html.erb @@ -0,0 +1,5 @@ +<%= this_is 'short', 'enough' %> +<% if condition? this_is_short then x else y end %> +<% if condition? this_is_pretty_long, so_we_probably, cant_fit_all_arguments, in_one_line %> + <%= render 'some_partial', param_one: 1, param_two: 2, param_three: 3, param_four: 4, param_five: 5 %> +<% end %> diff --git a/test/formatting_test.rb b/test/formatting_test.rb index e0b69b5..1ef9fba 100644 --- a/test/formatting_test.rb +++ b/test/formatting_test.rb @@ -32,6 +32,14 @@ def test_layout assert_formatting("layout") end + def test_method_calls_without_parens + assert_formatting("without_parens") + end + + def test_erb_inside_html_tag + assert_formatting("erb_inside_html_tag") + end + private def assert_formatting(name)