Crystal: Formatter indentation bug

[Sija]

Extracted from #5146.

[1, 2, 3]
  .each do |n|
    n
  end

Gets formatted to:

[1, 2, 3]
  .each do |n|
  n
end

/cc @MakeNowJust

[asterite]

Not a bug. The expression starts at indent 0 and the block is at indent 2. Look:

# This is OK
[1, 2, 3].each do |n|
  n
end

# Now I just put a newline after `]`, still OK
[1, 2, 3]
  .each do |n|
  n
end

[straight-shoota]

Maybe it's not a bug but what about a feature request?

I think this example looks bad. It would make more sense to increase the indent of the next line after a do and close the block at the same indent as the line with do regardless of where the expression started.

[asterite]

Okay, then we have this:

[1, 2, 3].map { |x| x - 1 }
         .select { |x| x > 1 }
         .each do |x|
  puts 1
end

Do we want it to format like this:

[1, 2, 3].map { |x| x - 1 }
         .select { |x| x > 1 }
         .each do |x|
            puts 1
end

Or maybe it's already wrong that . is indent too far, so maybe it should be like this:

[1, 2, 3].map { |x| x - 1 }
  .select { |x| x > 1 }
  .each do |x|
     puts 1
end

Or maybe like this:

[1, 2, 3].map { |x| x - 1 }
         .select { |x| x > 1 }
         .each do |x|
            puts 1
         end

In the end, it boils down to user preference, something that the formatter doesn't allow.

But yes, we can continue discussing what's the best formatting here.

[Sija]

For my taste end should be on the same line as starting do, in the same vein as in other places. Same goes for { and }.

[RX14]

@Sija uhh, you mean?

foo do |x|
      bar
    end

I think thats a terrible idea

[straight-shoota]

I think he means the line with end should have the same indent as the line including do. And the lines in between should be increased indent by one.

[1, 2, 3]
  .each do |n|
    n
  end

foo do |x|
  bar
end

[1, 2, 3].map { |x| x - 1 }
         .select { |x| x > 1 }
         .each do |x|
            puts 1
         end

I'd say this is the most logical.

[Sija]

@straight-shoota yep, u get my drift.

[MakeNowJust]

All cases work fine!

I'll create a new pull request.

[luislavena]

[1, 2, 3].map { |x| x - 1 }
         .select { |x| x > 1 }
         .each do |x|
           puts 1
         end

The problem with the above is that a small change on position of the first call will cause a ripple effect on all the lines of the block:

[1, 2, 3, 4, 5].map { |x| x - 1 }
               .select { |x| x > 1 }
               .each do |x|
                 puts 1
               end

Adding noise to the diffs. :confused:

[straight-shoota]

You can just ignore whitespace in the diff... And this style with high indents should certainly not be promoted.

You can write it better this way:

numbers = [1, 2, 3, 4, 5]
numbers = .map { |x| x - 1 }
          .select { |x| x > 1 }
          .each do |x|
            puts 1
          end

[faultyserver]

A style that I've been using recently is to double-indent the lines before the block body starts, but keep everything else the same:

[1, 2, 3, 4, 5]
    .map { |x| x - 1 }
    .select { |x| x > 1 }
    .each do |x|
  puts 1
end

I like this format because it keeps the body from being arbitrarily lengthened by leading whitespaced, and (imo) makes it more clear where the method chaining ends and the block body actually starts.

Particularly, I think this looks better for nested blocks:

[1, 2, 3, 4, 5]
    .map { |x| x - 1 }
    .select { |x| x > 1 }
    .each do |x|
  puts 1

  [1, 2, 3, 4, 5]
      .map { |x| x - 1 }
      .select { |x| x > 1 }
      .each do |x|
    puts 1
  end
end

[jhass]

Let's call this closed via #5234, everything else seems too opinionated.