erlfmt_format: preserve empty lines between values in containers

[awalterschulze]

Fixes #72

This preserves empty lines between values in containers, like exports and records.

This required some refactoring, since the simple fold_doc now needed to not only be aware of the break, but also needed to have the original value, to be able to check for empty lines to preserve.

Really struggled to make this refactoring more readable and I guess it is debatable, whether I achieved that, so looking forward to ideas in the review.

Also needed to preserve the end line for better empty line detection.
Should probably do the same for inner end line, so maybe we can think of a better function name.

[michalmuskala]

In the parser we have a ?range_anno macro that does something similar. What do you think about having a similar function in erlfmt_scan? Something like:

range_anno(First, Last) when is_tuple(First), is_tuple(Last) ->
    (element(2, First))#{end_location := get_end_location(Last)}.

We could call it here as:

Meta = erlfmt_scan:range_anno(Head, lists:last(Group))

[awalterschulze]

That sounds great :)

[awalterschulze]

We should probably also set the inner_end_location, right?

[michalmuskala]

Hmm... potentially. Do we really need it? It would need some caseing to only add it if it was there in Last in the first place.

[awalterschulze]

I added range_anno, but I needed to pass in Meta, otherwise, we lose the comments.

[michalmuskala]

Looking at all the complexity trailing comments introduce, I started thinking if maybe we should change how we represent them in the AST. My proposal would be to use the post_comment anno of the last element if the container has elements. If the container is empty with only the comment inside, we would still have the standalone comment node. What do you think? I believe it should make a lot of code simpler, we have to special-case those standalone comments a lot and effectively apply some manual processing that would be done for post_comment of last node anyway.

[awalterschulze]

Looking at all the complexity trailing comments introduce, I started thinking if maybe we should change how we represent them in the AST. My proposal would be to use the post_comment anno of the last element if the container has elements. If the container is empty with only the comment inside, we would still have the standalone comment node. What do you think? I believe it should make a lot of code simpler, we have to special-case those standalone comments a lot and effectively apply some manual processing that would be done for post_comment of last node anyway.

I guess I have always thought the pre_comments were too attached. I would expect to have more standalone comments in the ast. Basically if there is an empty line between this and the next element, then the comment is intuitively not attached to the next element.
So that would be one way of refactoring, is to embrace the detached comments.

But to be pragmatic we could also embrace the aggressive attachment of comments and consistently say that all comments are attached. This would attach these trailing comments as post comments.

Which ever way we go, I think we can do this in a follow up pull request, and possibly pair on it together.

[michalmuskala]

The problem of standalone comments is that they don't behave syntactically like other elements, for example inside lists they don't have , in-between them. Attaching comments to elements (what your code effectively does in here) bypasses this issue.

[awalterschulze]

Yes so I am happy to go with the pragmatic route, in a follow up commit.

[michalmuskala]

What happens if the way the container was initially laid out didn't include the "force breaks" space between [ and first element? In particular, if it was written down in a "comma-first" style like below:

[ 1
, 2

, 3
]

Do we collapse it into a single line [1, 2, 3] or do we expand it into:

[
    1,
    2,

    3
]

Either choice is fine by me, I think we should have a test that explores this, though.

[awalterschulze]

a test that explores this, though.

Great catch :)
This seems to be the current passing test.

?assertFormat(
        "[ 1\n"
        ", 2\n"
        "\n"
        ", 3\n"
        "]\n",
        "[1, 2,\n"
        "\n"
        "3]\n"
    ).

I also agree, that either case would be great, except the one that is happening now.

[awalterschulze]

Have to say it is very tough to get this and other containers like tuples, to place nice together

What happens if the way the container was initially laid out didn't include the "force breaks" space between [ and first element? In particular, if it was written down in a "comma-first" style like below:

[ 1
, 2

, 3
]

Do we collapse it into a single line [1, 2, 3] or do we expand it into:

[
    1,
    2,

    3
]

Either choice is fine by me, I think we should have a test that explores this, though.

This is now fixed, it was quite tricky :)

[michalmuskala]

Hmm.... We should probably re-write the guide because it no longer works as advertised. It's not enough to just remove the newline between [ and first element.

This is so much complexity in here 😢

[awalterschulze]

Good catch.
I fixed the guide.

I think it makes sense to not only take a hint from the first newline, but rather from any newline in the container.

[michalmuskala]

Have to say it is very tough to get this and other containers like tuples, to place nice together

Yeah, there's a lot of complexity in the rules for different containers. We probably need to rethink the general approach of keeping everything together after merging this code.