Make multi-line tuples & binaries next-break-fits

[awalterschulze]

Fixes #112

[michalmuskala]

I think we should only do this if the tuple will be indeed broken out. I think right now something weird will happen for not broken tuples that don't fit, e.g.

Foo = {1, 2, 3,
    4, 5, 6}

[awalterschulze]

Yes this is what happening, given the code in this pull request, the following test will pass

?assertSame(
        "Foo = {1, 2, 3,\n"
        "    4, 5, 6}\n",
        15
    ),

[michalmuskala]

Yeah, I don't think this should happen. I think for this it should be:

Foo =
    {1, 2, 3,
        4, 5, 6}

We should only inline those when the tuple is broken up, e.g.

Foo = {
    1, 
    2,
    3,
    4
}

[awalterschulze]

This is what is happening on master right now:

?assertSame(
        "{1, 2, 3,\n"
        "    4, 5,\n"
        "    6}\n",
        11
    ),

?assertSame(
        "Foo =\n"
        "    {1, 2, 3,\n"
        "        4, 5,\n"
        "        6}\n",
        15
    ),

I think I like what is happening in this pull request

?assertSame(
        "Foo = {1, 2, 3,\n"
        "    4, 5, 6}\n",
        15
    ),

What would you prefer?

[awalterschulze]

Yeah, I don't think this should happen. I think for this it should be:

Foo =
    {1, 2, 3,
        4, 5, 6}

We should only inline those when the tuple is broken up, e.g.

Foo = {
    1, 
    2,
    3,
    4
}

Aha sorry, didn't see this. I agree :)

[michalmuskala]

Should we special-case it only here for assignment or should we do it everywhere i.e. amending is_next_break_fits?

[awalterschulze]

I tried it out, so basically adding, call, macro_call and bin to the NEXT_BREAK_FITS list, results in:

{function, _, [{clause, _, {call, _, _, [Pat]}, empty, [_]}]} = parse_form(
    "f(" ++ String ++ ") -> ok."
)

instead of before

{function, _, [{clause, _, {call, _, _, [Pat]}, empty, [_]}]} =
    parse_form("f(" ++ String ++ ") -> ok.")

and

foo(
    bar(
        1
    )
)

instead of before

foo(bar(
    1
))

So I don't think we should modify is_next_break_fits and add these cases.

So then there is still a question about whether we should pattern match in binary_op_to_algebra to only have this special case for Op =:= '='.

Or did I misinterpret what you were trying to say?

[michalmuskala]

I was thinking about replicating the conditions that make a tuple be laid out with line,

erlfmt/src/erlfmt_format.erl

Lines 395 to 396 in 15f2a25

	has_opening_line_break(Meta, Values) orelse
	has_trailing_comments(Values) orelse

This means transforming is_next_break_fits into something like

is_next_break_fits({FlexContainer, Meta, Values}) when FlexContainer =:= tuple; FlexContainer =:= bin ->
    has_opening_line_break(Meta, Values) orelse has_trailing_comments(Values);
is_next_break_fits(Other) ->
    %% current implementation

This means:

• we retain current behavior for lists, maps, and other NEXT_BREAK_FITS
• we still special-case call and macro call to break after = and :: operators
• for things that are laid out as flex we print them like today when they are flex, but in cases where they are foced to be multi-line we treat them as any other container

The last point is the only modification

[awalterschulze]

This worked very well. I had no instinct for this yet. Great idea :)

[michalmuskala]

Looks great!