Keep line breaks in UFCS chains

[belka-ew]

Fixes #503.

I looked at the code and dfmt makes best effort to recognize what is a UFCS chain. I'm not sure what is the correct behaviour. But --keep_line_breaks adding an additional newline is definitely a bug, so I fixed only that part.

[Hackerpilot]

But --keep_line_breaks adding an additional newline is definitely a bug, so I fixed only that part.

The documentation states that the option means "Keep existing line breaks if these don't violate other formatting rules." Which I think, so far, has meant that existing line breaks will be maintained, but that new ones can be added if there are lines that are too long. If the formatter is breaking lines that aren't actually too long, then that's a different problem. And in that case, checking the value of the keep_line_breaks config option is not the right way to do things.

[belka-ew]

@Hackerpilot, is it good to go now?

[belka-ew]

Anybody?

[belka-ew]

Three green bottles standing on the wall Three green bottles standing very tall And if one green bottle should accidentally fall There'll be two green bottles standing on the wall

[belka-ew]

Two green bottles standing on the wall Two green bottles standing very tall And if one green bottle should accidentally fall There'll be one green bottle standing on the wall

[Hackerpilot]

I've been putting off looking at this for two reasons.

1. The first is that nobody seems to have actually read my original comment on this pull request. The keep_line_breaks option is used to preserve, not prevent, line breaking. Adding a check of this configuration option here is not the right thing to do.
2. The second is that the real solution to this problem is to define a correct way to decide if the code in question is a function call pipeline or just a member function call. I don't know how to do this. The best that we have is the code in the ast_info module that generates the UFCS hints, and it's pretty clear that this is not good enough.

[belka-ew]

1. But when turning off keep_line_breaks, the h() function is formatted with each .to!string on a separate line, which is imo much more what we want to have.

   It is not different from how this is handled currently. Please note that the pull request doesn't change the behaviour without --keep_line_breaks. Everything it does is disabling the automatically generated line breaks with --keep_line_breaks.

2. This change doesn't prevent all line breaks, it only prevents smart line breaks inserted based on the AST information to format the UFCS chains differently. It doesn't prevent the line break if the line is too long. I specifically added a test with h() which proves my claim and breaks the line with --keep_line_breaks if the line gets too long. And it is exactly what --keep_line_breaks is supposed to do, it says: Let me decide manually where to break the line if it's not too long.

3. I was smart enough to remove my original repository (and I apologize). I recreated it and rebased the branch on master, but it doesn't link to this pull request automatically, so I probably need to recreate the pull request.

[WebFreak001]

It is not different from how this is handled currently. Please note that the pull request doesn't change the behaviour without --keep_line_breaks. Everything it does is disabling the automatically generated line breaks with --keep_line_breaks.

Yes the problem here is that these automatically generated line breaks that help in readability are missing with --keep_line_breaks here.

As Hackerpilot said:

The keep_line_breaks option is used to preserve, not prevent, line breaking. Adding a check of this configuration option here is not the right thing to do.

and I agree with that.

Users use the auto-formatter to not need to manually insert new lines and indentation, but instead let dfmt automatically do so.

This is what dfmt does right now:

string f()
{
    return duration.total!"seconds"
        .to!string;
}

string g()
{
    return duration.total!"seconds"().to!string;
}

string h()
{
    return duration.total!"seconds"().to!string
        .to!string
        .to!string
        .to!string
        .to!string
        .to!string
        .to!string
        .to!string
        .to!string;
}

And I think that's much better than changing that behavior to the here suggested

string f()
{
    return duration.total!"seconds".to!string;
}

string g()
{
    return duration.total!"seconds"().to!string;
}

string h()
{
    return duration.total!"seconds"().to!string.to!string.to!string.to!string.to!string.to!string
        .to!string.to!string.to!string;
}

Additionally it looks like the linked bug is not fixed with this with the default dub flags. (But only when --keep_line_breaks is used, in which case the formatting regresses as mentioned above)

[belka-ew]

Users use the auto-formatter to not need to manually insert new lines and indentation, but instead let dfmt automatically do so.

And that users can just remove the --keep_line_breaks flag and line breaks will be inserted automatically. In my opinion --keep_line_breaks was meant as a simple solution to force certain line breaks where dfmt can't determine the correct line breaking automatically; because it is sometimes difficult to implement it correctly, like in this case.

[FeepingCreature]

Disclaimer: I work for Funkwerk, same as @belka-ew .

I understand what you're saying about --keep-line-breaks. But I think it's underappreciated that the line breaks that dfmt creates at the moment are aesthetically terrible. Would it be acceptable to introduce another flag, such as --dont_reflow_property_chains? Or --dfmt_reflow_property_chains_respect_column_limit, I mean, I do like the automatic property line breaks and indentation, I just don't like that dfmt apparently decides to do it at 80 [edit: 60! even worse] columns when it's configured for a 120 limit. Part of the motivation here is to not just to disable a formatting rule that we don't like, but to disable a formatting rule that breaks compatibility with the rest of dfmt for no clear reason.

I mean, I'm a dfmt user as well, and I think this formatting rule makes the appearance of the formatted code gratuitously worse. I don't want to argue aesthetics here, but dfmt shouldn't unilaterally enforce one particular aesthetic view about property chain line breaks without an option to disable it, particularly when it's divisive.

[WebFreak001]

I think if we make this a separate flags it would be fine.

But usually this sort of thing should be done with //dfmt off and fixing the things that are simple to fix.