davidwessman / syntax_tree-erb

Syntax Tree support for ERB
MIT License
21 stars 3 forks source link

[Formatting] Syntax error for `if` condition that spans an `each do ... end` block #74

Open rdimartino opened 5 months ago

rdimartino commented 5 months ago
ERB-snippet ```html
    <% if @items.each do |i|%>
  • <%= i %>
  • <% end.blank? %>
  • No items
  • <% end %>
```
Expected formatting ```html
    <% if @items.each do |i|%>
  • <%= i %>
  • <% end.blank? %>
  • No items
  • <% end %>
```
Actual formatting ```sh Error: syntax error, unexpected `end' > 1 |
    | ^ 2 | <% if @items.each do |i|%> 3 |
  • <%= i %>
  • 4 | <% end.blank? %> ```

Comment

This may be an anti-pattern but it's valid ERB. Instead of checking the size of @items directly, you can check if the each block rendered anything and if not (i.e. .blank? is true) then you render the fallback.

If this pattern can't be supported, it would be nice if the syntax error pointed to a better spot at least.

Versions

syntax_tree: 6.1.1 syntax_tree-erb: 0.10.5

davidwessman commented 5 months ago

Ruby has so much syntax 🙈 Would be great to figure out a way to let SyntaxTree or Prism handle all the Ruby-code.

davidwessman commented 5 months ago

@rdimartino I did not have time to think about the parsing - but I managed to improve the error reporting to look much nicer. Do you want to try it out? It is on main.

rdimartino commented 5 months ago

@davidwessman Thanks! It's definitely better, but I noticed that the indicated line appears to be the outer most block containing the problematic syntax, which may not always lead to finding where the problem is.

E.g. I have something like this

<%= content_tag :header do %>
  <div class="flex-1 min-w-0">
    <h2>
      <%= yield :page_header %>
    </h2>
    <%= content_tag :div do %>
      <%= yield :page_subheader %>
    <% end if content_for?(:page_subheader) %>
  </div>
  <%= content_tag :div do %>
    <%= yield :page_actions %>
  <% end if content_for?(:page_actions) %>
<% end if content_for?(:page_header) %>

where all of those blocks ending with end if content_for?(...) cause syntax errors.

And the error is:

Error: Could not parse ERB-tag: syntax error, unexpected `end'
  62 |       <%= render "shared/flash" %>
  63 |       <div">
> 64 |         <%= content_tag :header do %>
     | ^
  65 |           <div>
  66 |             <h2>
  67 |               <%= yield :page_header %>

So, given the indicated line, I fix the outer block, removing the if content_for?(:page_header) and wrapping the block inside an if instead, like:

<% if content_for?(:page_header) %>
  <%= content_tag :header do %>
    ...
  <% end %>
<% end %>

but the new error still points to this outer block:

Error: Could not parse ERB-tag: syntax error, unexpected `end'
  62 |       <%= render "shared/flash" %>
  63 |       <div>
> 64 | <% if content_for?(:page_header) %>
     | ^
  65 | <%= content_tag :header do %>
  66 |   <div>
  67 |     <h2>

So, it does help by giving a better scope for the issue, but if I had a block around a lot of content (like if current_user.present? or something) and this pattern was causing an issue, the indicated line might not help me find where the problem was.

davidwessman commented 4 months ago

One step further now 😊 But still need to work on the right column.

rdimartino commented 4 months ago

🙌

Error: Could not parse ERB-tag: syntax error, unexpected `end'
  70 |     <%= content_tag :div do %>
  71 |       <%= yield :page_subheader %>
> 72 |     <% end if content_for?(:page_subheader) %>
     | ^
  73 |   </div>
  74 |   <%= content_tag :div do %>
  75 |     <%= yield :page_actions %>

Now this is helpful!

davidwessman commented 4 months ago

🎉

And now I just merged some column index as well (it might be off in some cases, still need to work out some better algorithm for it). Give it a spin!

davidwessman commented 4 months ago

If I would send all the content:

content_tag :div do
  yield :page_subheader
end if content_for?(:page_subheader)

then SyntaxTree would have no problem parsing it, but I am not sure how to do that and get the formatting to work :( Something with placeholders could work.

rdimartino commented 4 months ago

Column indexing seems to be working great as well! Every example I have point to the d of the block end