Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 2 commits into from
Aug 25, 2020
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
15f2a25
Make multi-line tuples & binaries next-break-fits
awalterschulze Aug 21, 2020
48a598e
simplify is_binary_op_break_fits_exception and fold it into is_next_b…
awalterschulze Aug 25, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 9 additions & 6 deletions src/erlfmt_format.erl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -318,8 +318,9 @@ binary_op_to_algebra(Op, Meta, Left, Right, Indent) ->
combine_comments(Meta, maybe_wrap_in_parens(Meta, Doc)).

binary_op_to_algebra(Op, Left, Right, LeftD, RightD, Indent) ->
DontBreakCalls = is_call(Right) andalso not has_break_between(Left, Right),
case DontBreakCalls orelse is_next_break_fits(Right) of
BreakFitsException = is_binary_op_break_fits_exception(Right) andalso
not has_break_between(Left, Right),
case is_next_break_fits(Right) orelse BreakFitsException of
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

@michalmuskala michalmuskala Aug 24, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

true ->
with_next_break_fits(true, RightD, fun(R) ->
concat([group(LeftD), <<" ">>, Op, <<" ">>, group(R)])
Expand All @@ -328,6 +329,12 @@ binary_op_to_algebra(Op, Left, Right, LeftD, RightD, Indent) ->
breakable_binary_op_to_algebra(Op, Left, Right, LeftD, RightD, Indent)
end.

is_binary_op_break_fits_exception({call, _, _, _}) -> true;
is_binary_op_break_fits_exception({macro_call, _, _, _}) -> true;
is_binary_op_break_fits_exception({tuple, _, _}) -> true;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
    ),

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
}

Copy link
Contributor Author

@awalterschulze awalterschulze Aug 24, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

is_binary_op_break_fits_exception({bin, _, _}) -> true;
is_binary_op_break_fits_exception(_) -> false.

breakable_binary_op_to_algebra(OpD, Left, Right, LeftD, RightD, Indent) ->
HasBreak = has_break_between(Left, Right),
RightOpD = nest(break(concat(<<" ">>, OpD), group(RightD)), Indent, break),
Expand Down Expand Up @@ -713,10 +720,6 @@ has_inner_break(Outer, Inner) ->
is_next_break_fits_op(Op) ->
lists:member(Op, ?NEXT_BREAK_FITS_OPS).

is_call({call, _, _, _}) -> true;
is_call({macro_call, _, _, _}) -> true;
is_call(_) -> false.

is_next_break_fits(Expr) ->
lists:member(element(1, Expr), ?NEXT_BREAK_FITS) andalso has_no_comments_or_parens(Expr).

Expand Down
13 changes: 13 additions & 0 deletions test/erlfmt_format_SUITE.erl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -423,6 +423,19 @@ binary_operator(Config) when is_list(Config) ->
" 1\n"
"]\n"
),
?assertSame(
"Foo = {\n"
" long_element_1,\n"
" long_element_2,\n"
" long_element_3\n"
"}\n"
),
?assertSame(
"Foo = <<\n"
" 1/float,\n"
" <<222, 333>>\n"
">>\n"
),
?assertSame(
"f() ->\n"
" {PreComments, InnerComments, PostComments} = split_comments(\n"
Expand Down