nebulab / erb-formatter

Format ERB files with speed and precision
MIT License
144 stars 24 forks source link

Improve multiline formatting #7

Open i-tsvetkov opened 1 year ago

i-tsvetkov commented 1 year ago
elia commented 1 year ago

@i-tsvetkov sorry for the delay, this fell completely off the radar 📡

First of all thanks for taking the time to dig into the code, that was brave! 😄

I'm a bit conflicted about the change, my initial drive for ruby code was for having it indented relatively to the beginning of the ERB tags (like it is currently) and for parentheses-less method calls the workaround was to add parens.

In my view the ideal situation would align to the method call for parens-less calls and as explained above when parens are present.

Thoughts?

i-tsvetkov commented 1 year ago

Hi @elia, I'm glad you found the time to look into this PR.

I saw that you have extracted the upgrade of syntax_tree into https://github.com/nebulab/erb-formatter/pull/10, you should know that there was an issue with newer versions regarding the transformation of short blocks like this one

[1, 2, 3].each do |i|
  puts i
end

into single-line form

[1, 2, 3].each { |i| puts i }

That issue was resolved by this commit.

Regarding the question about the indentation - I think that multi-line code should be indented relative to the start of the ERB tag content (right after the <%) instead of the start of the ERB tag itself (just before the <%) that way we responsibility and complexity of formatting ruby code will be handled by syntax_tree.

Here is an example with an if-else statement:

# relative to the start of the ERB tag content
# the if-else-end is vertically aligned
<%= if rand < 0.5
      puts 1
    else
      puts 2
    end %>

# relative to the start of the ERB tag itself
# the if-else-end is not vertically aligned
<%= if rand < 0.5
  puts 1
else
  puts 2
end %>

What do you think?

i-tsvetkov commented 1 year ago

@elia Any update on this PR?

mtomov commented 1 year ago

Hello,

Thanks for erb-formatter!

I was wondering where you'd like to take this PR?

I find the proposed style to be nicer than current. In fact, when trying out erb-formatter, the lack of indentation with multi line arguments surprised me. Wondered what the reasoning for that choice was as my preference would be the same as this PR proposes.

Thank you!

mtomov commented 1 year ago

Hey @elia

We've manually updated the gem locally with this PR changes, and has been working great so far. Any chance you could revisit this?

elia commented 1 year ago

@mtomov on it, thanks for the ping, I'm pondering options 🙏