Crystal: Format multiline chained method calls for better git diffs

[paulcsmith]

I checked the issues and couldn't find something similar, so posting here to gather thoughts before proposing a PR

something = SomeClass.new
  .do_something
  .do_something_else

# Is formatted to this when using `crystal tool format`
thing = SomeClass.new
                 .do_something
                 .do_something_else

I think the first one is "better" because even if I change the class or rename the variable, the other lines don't change. This makes diffs a lot cleaner and makes it easier to see what actually changed. I also think it makes it easier to remember the rules, because everything is consistently indented by 2 spaces. It also makes it much easier to keep things under 80 lines. With the current format, if you have a long variable name it makes it very difficult to keep some lines under 80 on the following lines.

task_for_current_user = TaskQuery.new
                                 .where({published: current_user.id}) # Pushes this far to the right, making it hard to fit under 80 lines

Subjectively, I also think it looks nicer since long variable names won't push things super far to the right (which looks strange IMO)

What are thoughts on changing the formatter so that it uses the first version (indented by two spaces instead of right aligned?)

[RX14]

If you want every method call aligned in the format suggested above, you can always use do this

something = SomeClass
  .foo(bar) { |x| ... }
  .do_something
  .do_something_else

[mverzilli]

Another alternative:

something = 
  SomeClass.new
    .do_something
    .do_something_else

[paulcsmith]

Those are cool ideas! Thanks for the suggestions @mverzilli and @RX14.

[asterite]

@paulcsmith That's a good observation

Most of the formatter rules are for code to look nicer, but it's true that lines can get a bit long, and the diff issue is a good point too. In fact, right now we are aligning successive constants so if you add another one the others will "change" and that will affect diffs.

Maybe the formatter should always use 2 spaces for indentation? That would be similar to gofmt (well, gofmt uses tabs, but the idea would be the same).

If we do that, some things will maybe not look very nice. For example:

# Now
var = if cond
        v1
      else
        v2
      end

# With 2 spaces, always
var = if cond
  v1
else
  v2
end

Another example:

# Now
call(x ||
     y ||
     z)

# With 2 spaces, always
call(x ||
  y ||
  z)

And another one (similar to yours):

# Now
[1, 2, 3].map { |x| x + 1 }
         .select(&.odd?)
         .group_by(&.itself)

# With 2 spaces, always
[1, 2, 3].map { |x| x + 1 }
  .select(&.odd?)
  .group_by(&.itself)

I somehow find the current formatting always more readable, even though spacing is not always the same. And there are always "workarounds" to make long lines fit, like the previous two comments.

What do others think?

[Sija]

@asterite looks way better for me, I vote 2 spaces 👍

[paulcsmith]

@asterite Thanks for the thoughts and examples. I think a consistent 2 space indentation would be great, but that's partially based on my subjective preference for how 2 spaces look. I think it's better for diffs, easier to understand, easier to modify and easier to fit under 80 chars + I think it looks better. I personally don't like how things are so aligned to the right with the current formatting. That's more of an aesthetics thing though. I think it is a tad bit easier to read in the current crystal format, but for me it's not enough to prefer it over the 2 space version.

EDIT: I think the second example looks worse with 2 spaces, but I like the other 2 more with 2 space indentation

[Sija]

2nd example looks better with right indentation but that's mostly because every line is the same length, below example makes it IMO closer to the proposed alternative:

call(foo ||
     bar_with_beer ||
     baz_bear)

[RX14]

var = if foo
        "bar"
      else
        "baz"
      end

can be rewritten

if foo
  var = "bar"
else
  var = "baz"
end

which is IMHO better in pretty much every way.

[1, 2, 3].map { |x| x + 1 }
         .select(&.odd?)
         .group_by(&.itself)

can be formatted

[1, 2, 3]
  .map { |x| x + 1 }
  .select(&.odd?)
  .group_by(&.itself)

All of these examples format to themselves with the current formatter, so I guess this change isn't required.

[drosehn]

If the formatter does 2-space indents when a new line is inside a new block, then (IMO) it should do something different when a new line is the continuation of a previous line. A constant 4-space or 6-space indent could be one way to handle wrapped statements.

[paulcsmith]

@RX14 Yes I think I'm starting to agree with you here. I hadn't tried the examples you posted, but now that I have the existing formatter works great. I'd still be happy to have it be the default for the original example I posted, but even if that's not the case I think that would be fine.

[sdogruyol]

This is not labeled. Is this open for discussion or shall we close it?

[asterite]

We already chose how the formatter works. We can't change it everytime someone complaints that they don't like its opinionated nature. So I'm closing this.

[paulcsmith]

Thanks for reviewing. Agreed that not every opinion can be satisfied without greatly devaluing the benefit of the formatter