Elixir: [Formatter] Alignment of do in with clauses

[mschae]

I realize formatter-related discussions/suggestions are usually very emotional and subjective. My goal here is to gather feedback and background on a current behavior of the formatter, not to spark discussions around its usefulness or code styles.

Also I am very fond of the formatter, am glad it's there and promote its use. Even when I don't disagree with every way it behaves because I prefer a consistently-styled codebase that in parts diverge from my personal taste to an inconsistently-styled codebase that in parts matches my personal taste. So thanks a lot for it! :heart:

Given the following (very meaningful) code snippet:

defmodule Greetings do
  def hello_world do
    with %{foo: bar} <- %{foo: "bar"},
         {:ok, baz} <- {:ok, "baz"}
    do
      IO.puts "yay!"

      {:ok, {bar, baz}}
    end
  end
end

I would like to keep the indentation of do as is. Having all of the control words on one indentation level helps me read the code more easily. However the code formatter formats it as follows:

defmodule Greetings do
  def hello_world do
    with %{foo: bar} <- %{foo: "bar"},
         {:ok, baz} <- {:ok, "baz"} do
      IO.puts("yay!")

      {:ok, {bar, baz}}
    end
  end
end

Environment

• Elixir & Erlang versions (elixir --version):

$ elixir --version
Erlang/OTP 20 [erts-9.1] [source] [64-bit] [smp:8:8] [ds:8:8:10] [async-threads:10] [hipe] [kernel-poll:false]

Elixir 1.6.0 (compiled with OTP 19)

• Operating system:

$ uname -srv
Darwin 17.4.0 Darwin Kernel Version 17.4.0: Sun Dec 17 09:19:54 PST 2017; root:xnu-4570.41.2~1/RELEASE_X86_64

Or in other words: macOS High Sierra 10.13.3

[josevalim]

Hi @mschae! In master and v1.6 branches you can achieve something similar by adding parens:

defmodule Greetings do
  def hello_world do
    with(
      %{foo: bar} <- %{foo: "bar"},
      {:ok, baz} <- {:ok, "baz"}
    ) do
      IO.puts("yay!")
      {:ok, {bar, baz}}
    end
  end
end

We don't plan to support any other way besides the current one and the one above because if we were to consider all options (with \, yours, etc), it would defeat the point of the formatter in trying to be consistent. :)

[OvermindDL1]

Hi @mschae! In master and v1.6 branches you can achieve something similar by adding parens:

Actually you can't:
https://github.com/OvermindDL1/overdiscord/commit/9849a389779f4758d67dd2ad85910b15b07ec902?diff=unified#diff-196940679e8d4a6c7cc7f484d343fbd8L102

It performed this diff once formatted:

-        with(
-          xkcd_link when xkcd_link != nil <- Meeseeks.text(Meeseeks.one(doc, css("channel item link"))),
-          xkcd_title when xkcd_title != nil <- Meeseeks.text(Meeseeks.one(doc, css("channel item title")))
-        ) do
+
+        with xkcd_link when xkcd_link != nil <-
+               Meeseeks.text(Meeseeks.one(doc, css("channel item link"))),
+             xkcd_title when xkcd_title != nil <-
+               Meeseeks.text(Meeseeks.one(doc, css("channel item title"))) do
           case db_get(state, :kv, :xkcd_link) do
-            ^xkcd_link -> nil
+            ^xkcd_link ->
+              nil
+
             _old_link ->
               db_put(state, :kv, :xkcd_link, xkcd_link)
               db_put(state, :kv, :xkcd_title, xkcd_title)
+
               if true != check_greg_xkcd(state, xkcd_link, xkcd_title) do
-                #send_msg_both("#{xkcd_link} #{xkcd_title}", "#gt-dev", state.client)
+                # send_msg_both("#{xkcd_link} #{xkcd_title}", "#gt-dev", state.client)
               end
           end
         end
-      _ -> IO.inspect(response, label: :INVALID_RESPONSE_XKCD)
+
+      _ ->
+        IO.inspect(response, label: :INVALID_RESPONSE_XKCD)
     end

I'd really really really prefer it to keep (...) around the arguments. What it does now is SO messy looking (although I like what it did to the rest of the rest there except the comment, full comments should be # (hash and a space) and commented out code should be just # (no space), but that's something else), in addition to it mixing indentation and alignment is just horrible...

EDIT: And I am using:

╰─➤  iex
Erlang/OTP 20 [erts-9.2] [source] [64-bit] [smp:6:6] [ds:6:6:10] [async-threads:10] [hipe] [kernel-poll:false]

Interactive Elixir (1.6.1) - press Ctrl+C to exit (type h() ENTER for help)

[josevalim]

@OvermindDL1 can you please check master and v1.6 branches? It has not been released yet, otherwise I would have said so. :) Here is the commit: b3e7587bf8170a500a8751dca9ef6312af50c466

If it still fails, please try to provide a failing test to our suite.

[OvermindDL1]

Ah it's recent! I don't run anything but release versions on that server (to match travis) so gimme a bit to clone it to my test/dev server. :-)

[OvermindDL1]

And tested, when parenthesis don't already exists it keeps the existing horrible mixing of indentation and alignment formatting (?!?) but when I explicitly add the parenthesis back like it was before then and only then does it keep them, so I'll have to manually go back through the code to add them back (blehg....), but it seems to work. Can't wait until it's on a version that travis has. :-)

_(still not a fan of with/for using comma separated parts instead of them just being normal expressions in a block... even more so since trailing commas are not allowed thus making re-organization of lines, which happened quite a bit while working out the logic on some of these sections, quite a huge amount of pain...)_

[ericmj]

Can't wait until it's on a version that travis has

You can use pre-release branches on travis, but they are of course not stable: https://docs.travis-ci.com/user/languages/elixir/#Specify-which-Elixir-version-to-build-with

[OvermindDL1]

@ericmj Ah cool, I've not used travis much (hence why I've been setting this project on it to learn it, I usually run my own CI). I'm not seeing on that page how to specify a pre-release version, it only shows released version numbers (which I'm already using to specify 1.6.1)?

[ericmj]

If you specify elixir: '1.6' it will track the v1.6 maintenance branch.

[OvermindDL1]

Ah so just drop the patch number, cool thanks!

[TMSCH]

Hi everyone!

Thanks for all the work on the formatter, it's a really great tool to have :)

We've been trying it out on our codebase and some concerns popped up, a couple of them are expressed here.

What is the rationale behind having 2 different formatted outputs for the same piece of code based on whether parenthesis are present or not? I'd argue it doesn't solve the goal of having 1 uniform codebase (Person 1 might prefer without parenthesis, Person 2 with).

Furthermore, I agree with @mschae that the default formatting is less readable than what he proposes. Let's look at a different example that shows a bit more code, and the corresponding mix format output:

defmodule Greetings do
  def hello_world do
    with %{foo: bar} <- %{foo: "bar"},
         {:ok, my_slightly_longer_name} <-
           unfortunately_this_doesnt_fit_on_the_previous_line(bar) do
      IO.puts("yay!")

      {:ok, {bar, baz}}
    end
  end
end

This is not very readable: where does the inner with block starts? The do is at the end of an indented line. And it is not realistic to require having each with statement fit in one line.

Now with parenthesis:

defmodule Greetings do
  def hello_world do
    with(
      %{foo: bar} <- %{foo: "bar"},
      {:ok, my_slightly_longer_name} <-
        unfortunately_this_doesnt_fit_on_the_previous_line(bar)
    ) do
      IO.puts("yay!")

      {:ok, {bar, baz}}
    end
  end
end

It does look better. @mschae's version, however:

defmodule Greetings do
  def hello_world do
    with %{foo: bar} <- %{foo: "bar"},
         {:ok, my_slightly_longer_name} <-
           unfortunately_this_doesnt_fit_on_the_previous_line(bar)
    do
      IO.puts("yay!")

      {:ok, {bar, baz}}
    end
  end
end

is the most readable in my opinion. The do position clearly delimitates the with conditions and its body. Putting parenthesis on with statements just seems weird, for such a commonly used statement.

The with statement is an extremely common and powerful statement of Elixir, and having an unreadable default formatting is one of our current biggest block to using mix format on our codebase.

I'm curious to hear the opinion of @josevalim on why we couldn't enforce 1 single formatted output (e.g. stripping parentheses if present) that clearly separates the blocks as @mschae suggested.

I would only keep the do on the same line if the with statement has only one condition and everything fits in one line.

Last thing: I'm happy to contribute to any changes that could arise from this discussion :)

Thanks!

[ericmj]

This is all subjective, so take this only as my opinion.

The formatter does not make ugly code look good. To me both of these alternatives look bad:

with {:ok, my_slightly_longer_name} <-
           unfortunately_this_doesnt_fit_on_the_previous_line(bar) do
  IO.puts("yay!")
end

with {:ok, my_slightly_longer_name} <-
           unfortunately_this_doesnt_fit_on_the_previous_line(bar)
do
  IO.puts("yay!")
end

The way to make it look good is to rewrite your code to avoid the line break:

with {:ok, my_slightly_longer_name} <- short_fun(bar) do
  IO.puts("yay!")
end

Which I think look better than moving the do to its own line:

with {:ok, my_slightly_longer_name} <- short_fun(bar)
do
  IO.puts("yay!")
end

[TMSCH]

I see your point, it is definitely subjective (though blocks delimitation should be pretty objective) but I also mentioned that I don't think it's realistic to ask to have every with condition fit in one line (e.g. basically ~70 characters once you remove the indents).

The example I gave use a long function name, but another one could use map pattern matching, which is more difficult/impossible to break in one line:

with {:ok, %{name: value, description: description, key: key} <-
       short_fun(bar),
     :ok <- I_need_the_key_here(key, name) do
  IO.puts("yay!")
end

Also, for the one liner case, I proposed to keep the do on the same line.

I'd also argue that even though no code formatter can make ugly code look good, a code formatter decision can make a higher % of code looks more readable than another.

What do you think?

[ericmj]

The objective way of looking at this is to keep the formatter consistent. The way block delimiters are formatted today is more consistent than adding a special case only for multi-line with.

The formatter does not know about Elixir semantics, it only knows the AST. Since it does not know about with or any other macros, functions, or operators you would have to specify a rule for do blocks that is not special cased to with.

[TMSCH]

I agree on the formatter being consistent :)

I'd also rather use the same rule for multi-line everything!

Functions:

def my_function(%{
      key1: value1,
      key2: value2
    })
do
  // Do some work
end

# Reads better than
def my_function(%{
      key1: value1,
      key2: value2
    }) do
  // Do some work
end

Case statements

case i_know_we_could_refactor_this_to_fit_in_one_line(%{
       key: "value"
     })
do
  {:ok, result} -> IO.inspect(result)
  {:error, _reason} -> IO.puts("Error...")
end

# Reads better than
case i_know_we_could_refactor_this_to_fit_in_one_line(%{
       key: "value"
     }) do
  {:ok, result} -> IO.inspect(result)
  {:error, _reason} -> IO.puts("Error...")
end

[josevalim]

The issue is that "reads better" boils down completely to personal opinion. I personally really dislike the do on its own line with parensless calls. :D The formatter already takes a lot of vertical space and this would cause it to take even more.

Those who prefer the style above can add explicit parens and get a very similar behaviour. At the end of the day the formatter will not please anyone a 100%. The reason why many adopt the formatter anyway is because even if you are happy with only 80%, the formatter still gets a lot right and it unifies the style between teams.