Formatting of large binary pattern matches

[Eptis]

Not sure if this is the preferred formatting by the community, but the formatter in 1.6 currently formats this code:

def dump(<< a1, a2, a3, a4, a5, a6, a7, a8, b1, b2, b3, b4, c1, c2, c3, c4, d1, d2, d3, d4, e1,
            e2, e3, e4, e5, e6, e7, e8, e9, e10, e11, e12 >>) do
    try do
      << d(a1)::4, d(a2)::4, d(a3)::4, d(a4)::4,
         d(a5)::4, d(a6)::4, d(a7)::4, d(a8)::4,
         d(b1)::4, d(b2)::4, d(b3)::4, d(b4)::4,
         d(c1)::4, d(c2)::4, d(c3)::4, d(c4)::4,
         d(d1)::4, d(d2)::4, d(d3)::4, d(d4)::4,
         d(e1)::4, d(e2)::4, d(e3)::4, d(e4)::4,
         d(e5)::4, d(e6)::4, d(e7)::4, d(e8)::4,
         d(e9)::4, d(e10)::4, d(e11)::4, d(e12)::4 >>
    catch
      :error -> :error
    else
      binary ->
        {:ok, binary}
    end
  end

as:

def dump(<<
        a1,
        a2,
        a3,
        a4,
        a5,
        a6,
        a7,
        a8,
        b1,
        b2,
        b3,
        b4,
        c1,
        c2,
        c3,
        c4,
        d1,
        d2,
        d3,
        d4,
        e1,
        e2,
        e3,
        e4,
        e5,
        e6,
        e7,
        e8,
        e9,
        e10,
        e11,
        e12
      >>) do
    try do
      <<
        d(a1)::4,
        d(a2)::4,
        d(a3)::4,
        d(a4)::4,
        d(a5)::4,
        d(a6)::4,
        d(a7)::4,
        d(a8)::4,
        d(b1)::4,
        d(b2)::4,
        d(b3)::4,
        d(b4)::4,
        d(c1)::4,
        d(c2)::4,
        d(c3)::4,
        d(c4)::4,
        d(d1)::4,
        d(d2)::4,
        d(d3)::4,
        d(d4)::4,
        d(e1)::4,
        d(e2)::4,
        d(e3)::4,
        d(e4)::4,
        d(e5)::4,
        d(e6)::4,
        d(e7)::4,
        d(e8)::4,
        d(e9)::4,
        d(e10)::4,
        d(e11)::4,
        d(e12)::4
      >>
    catch
      :error -> :error
    else
      binary ->
        {:ok, binary}
    end
  end

I personally would expect and prefer something more condensed. Is this intended or could this be improved?

Environment

• Elixir 1.6.0-rc.0, Erlang 20.1
• Operating system: OSX

[Eiji7]

I'm also not a fun of such rule. I think that there should be two separate formatting rules.

List is one big mystery. It's not surprise to see any data type in it really often, so it could be applied to both rules for its special cases.

Rule with condensation

Personally I'm not worked with such big binaries, but I really often see similar problem when formatting List, Keyword and when defining aliases.

1. alias A few times I had situation when I defined lots of aliases.
2. binaries Binaries as above are really good example to condensate.
3. List (that does not contain at least one element specified in 2nd rule and String) + Keywords Keywords usually contains simple options. Values of them mostly are short atoms or integers.

Rule without condensation

1. List (that contain at least one element specified in this rule or String)
2. Map + struct When defining maps and structs I'm usually calling functions directly inside map values
3. Tuple Tuples are usually short - mostly from 2 to 3 and sometimes 4 elements.

[stefanchrobot]

From the docs:

  If there are no newlines around the brackets, then the formatter will
  try to fit everything on a single line, such that the snippet below

      [foo,
       bar]

  will be formatted as

      [foo, bar]

The case in question seems to be when the formatter runs out of line length when fitting things into one line - it then falls back to a single item per line.

One strategy would be to try and fit as much as possible in one line, but that would not work in cases where the author wants to impose line breaks because it makes some sense as in the originally reported example.

A solution that I've seen to this problem is to use an extra comma to signify the intention, i.e.:

      << d(a1)::4, d(a2)::4, d(a3)::4, d(a4)::4,
         # truncated
         d(e9)::4, d(e10)::4, d(e11)::4, d(e12)::4,  # leave as-is
      >>

      << d(a1)::4, d(a2)::4, d(a3)::4, d(a4)::4,
         # truncated
         d(e9)::4, d(e10)::4, d(e11)::4, d(e12)::4   # fit as many as possible in one line
      >>

[NobbZ]

A magic comment that would switch off the formatter for some region of code has been requested multiple times and I will do it again here...

[josevalim]

And support for code comments will be denied once more because it will not only open up space for style discussions, which the formatter aims to eliminate, but it will cause developers to pollute the codebase witn notes regarding formatting.

I would rather support project configs for those than code comments.

Some features will never be added, regardless of how many times people ask for them. :)

[mjadczak]

@josevalim so admittedly I was not part of earlier discussions about this, but while I understand your reasons for not wanting to support generic code-level configuration comments, I think a binary "don't touch this" comment or even a function-level @verbatim attribute might be warranted. There are specific scenarios, especially when binary matching, where any configuration, no matter how complex, does not suffice, and one wants to enforce a manual formatting—for example, implementing a binary protocol by laying out binaries in the same way as some arbitrary hardware specification document.

I agree that it would be stupid to try to consolidate code style with a code formatter and then un-consolidate it on a line-by-line basis, sparking more arguments. For some of the issues raised here, e.g. whether a long list should be line-by-line or multi-line but filling up as much of each line as possible, a project-level configuration setting would be best, if this were addressed at all.

However, I don't think that a binary "don't format this block" comment or attribute is a configuration code comment—it simply shifts the responsibility for the format of that block of code to the maintainer. If people start using that to try and enforce their own preferences on code style, rather than to achieve formatting which is not rule-based but rather comes from some external constraints, then that's an issue for their team to resolve IMO.

[wojtekmach]

I personally don't have this problem, but if there needs to be a way to exlclude, perhaps it would be confined to just .formatter.exs, e.g.:

# .formatter.exs
inputs: ["lib/*.{ex,exs}"],
exclude: ["lib/binary_protocol.ex"]

the tradeoff is you'd lose all auto formatting in that file.

[josevalim]

I have a fix on PR #7183.

We are applying the "condensation" rules mentioned by @Eiji7, referred in the source code as "flex rendering" to both tuples and binaries. Flex rules try to fit the maximum number of entries per line.

We apply them to binaries because of the original issue reported here. And we are applying it for tuples because we already had some special rules for tuples but those rules can now be removed in favor of condensation/flex rules.

We can change lists and keyword lists to use condensation under some circumstances but we want to avoid doing conditional condensation, as that makes the output of the formatter harder to reason about.

This should also fix the issue with large tuples mentioned by @OvermindDL1 in the forum in the past. Please try the PR out and let us know how it goes.

[stefanchrobot]

@josevalim This looks kind of random - the delimiter for a tuple is kept on the same line:

 +  def translate(_min_level, :info, :report, {
 +        :std_info,
 +        [application: app, exited: reason, type: _type]
 +      }) do

but it's pushed to a new line for lists:

 +      assert_received {:mix_shell, :error,
 +                       ["You have configured application :app_unknown_sample" <> _]}

Can we have the same behavior for both?

[josevalim]

@stefanchrobot it is the same as with any other data structure. If you add newline after { and before } then it will keep one entry per line. You can fix it by removing the newlines around the curly brackets.

[josevalim]

It is also worth noting that Elixir code has already been formatted a couple times while the formatter was still in development.

The original code was this:

def translate(_min_level, :info, :report, {:std_info, [application: app, exited: reason, type: _type]}) do

And today it would be formatted as:

def translate(
      _min_level,
      :info,
      :report,
      {:std_info, [application: app, exited: reason, type: _type]}
    ) do

Which is more consistent with the assert_receive outcome. But since we have expanded it into a multiline tuple in some past iteration, it is keeping the multiline formatting. :)

[stefanchrobot]

@josevalim Thanks, makes sense!

[josevalim]

Btw, closing in favor of #7183. :)