Split all parameters of a function application over multiple lines if they exceed page width #929

Bobface · 2020-06-24T11:36:19Z

Fixes #907 by putting every parameter of a function application on a newline if the function application exceeds PageWidth.

cc @knocte

Bobface · 2020-06-24T11:47:17Z

@nojaf What is your first impression of this implementation? I am not quite sure about some changes I had to make to some tests.

Also, what should we do if the parameters do not exceed page width if they were in one line, but they are split over multiple lines in the original and have comments at EOL? I was thinking to keep them split over multiple lines in that case.

nojaf

Hey @Bobface , thanks for this PR. I personally like the outcome.
Some tests are a bit weird though.
I hope my remarks are useful, let me know if you have any further questions.

src/Fantomas.Tests/ComputationExpressionTests.fs

nojaf · 2020-06-24T18:00:53Z

src/Fantomas.Tests/ElmishTests.fs

-            (span [ ClassName "has-text-weight-semibold" ] [
-                ofInt fullMatchedLength
-             ])
+        tokenDetailRow "FullMatchedLength" (span [ ClassName "has-text-weight-semibold" ] [


This seems a bit weird as the span block is multiline.
So I would expect it to be more like

tokenDetailRow "FullMatchedLength" (span [ ... ])

I believe the problem here is the following:
When the long expression is checked the printer tries to put all parameters in one line. Further down in the recursive calls where the array is printed it is detected that the page width will be exceeded if the array is printed in one line so it is split.
That satisfies the maximum width for the long expression again.

Might need your help on what's the best way to fix that one.

The code now makes

tokenDetailRow "FullMatchedLength" (span [ ClassName "has-text-weight-semibold" ] [ ofInt fullMatchedLength ])

of it. This is correct behaviour in my opinion. The function call doesn't fit on the remainders of the line because of the arguments doesn't fit. This makes sense.
I think the exception here will be lambda's, if you have those, you might want to keep the original behaviour. See next comment.

nojaf · 2020-06-24T18:01:22Z

src/Fantomas.Tests/RecordTests.fs

-                       Opt.valueWith "new value"
-                           [ "fourth"
-                             "ssssssssssssssssssssssssssssssssssssssssssssssssssss" ] ]) ]
+                       Opt.valueWith "new value" [ "fourth"


Same remark, the last part is multiline

nojaf · 2020-06-24T18:03:37Z

src/Fantomas/CodePrinter.fs

-                    +> unindent))
+        fun ctx ->
+            (ifElseCtx 
+                (exceedsWidth ctx.Config.PageWidth


Use expressionFitsOnRestOfLine instead here.
I would also create separate values for the short and long expression path.
Something like:

fun ctx -> let shortExpression = ... let longExpression = ... expressionFitsOnRestOfLine shortExpression longExpression ctx

I think you might want to also check for isCompExpr and use the existing expression in that case.

I have tried to use expressionFitsOnRestOfLine however it directly goes into "short expression mode" and splits even very short apps over multiple lines in some cases. Is that intended?

Did you try it in this PR? What you are describing sounds a bit weird.

Yes, I have just pushed the version with expressionFitsOnRestOfLine so you can see the failing tests where even short expressions are newlined. I assume shortExpressionWithFallback which is used by expressionFitsOnRestOfLine is the culprit (see the comment):

let private shortExpressionWithFallback (shortExpression: Context -> Context) (fallbackExpression) maxWidth startColumn (ctx: Context) = // if the context is already inside a ShortExpression mode and tries to figure out if the expression will go over the page width, // we should try the shortExpression in this case. match ctx.WriterModel.Mode with | ShortExpression infos when (List.exists (fun info -> info.ConfirmedMultiline || info.IsTooLong ctx.Config.PageWidth ctx.Column) infos) -> ctx | _ -> ....

nojaf · 2020-06-26T06:59:16Z

src/Fantomas/CodePrinter.fs

+                    +> unindent))))
+
+            //ifElseCtx (fun ctx -> exceedsWidth ctx.Config.PageWidth longExpression ctx || parametersMustBeNewlinedBecauseOfTriviaContent es ctx) shortExpression longExpression ctx
+            ifElseCtx (fun ctx -> parametersMustBeNewlinedBecauseOfTriviaContent es ctx) shortExpression (expressionFitsOnRestOfLine longExpression shortExpression) ctx


expressionFitsOnRestOfLine longExpression shortExpression that doesn't really make much sense.
The function expressionFitsOnRestOfLine will first try the longExpression and use the short one as fallback.
Those parameters should be swapped.

I also think you don't always want this behaviour, it will depend on the es of | App(e, es).
If one expression in es is a lambda for example I don't think you necessarily want the suggested formatting of putting every argument on one line.

For example:

let foo = Array.map (fun x -> // some multiline code x + x) [| 8;9;6;5 |]

I don't think you want to make

let foo = Array.map (fun x -> // some multiline code x + x) [| 8;9;6;5 |]

of it.

nojaf · 2020-06-26T07:01:06Z

src/Fantomas/CodePrinter.fs

+        //         c
+        //
+        // This has to be split over multiple lines to preserve the line comment
+        let parametersMustBeNewlinedBecauseOfTriviaContent es ctx =


For what unit test did you introduce this? I would assume that a comment in the expression will force expressionFitsOnRestOfLine to take the fallback route so I'm not quite sure this is necessary here.

nojaf · 2020-06-26T07:44:41Z

Hey @Bobface, what do you think of nojaf@6a28f6f.
I limited the scope of where exactly the parameters are split into multiple lines.
Fantomas does not always consider multiline strings as multiline constructs (example, there in theory Fantomas should add the whole thing on a new line after the = but this is a known exception).

The same for lambda's, we want the (fun x -> part still on the same line, that makes more sense mostly.

… they exceed page width

Bobface · 2020-06-29T08:24:52Z

@nojaf This PR is now using your implementation for the CodePrinter. It works fine, but there is one test in AppTests where I am not sure if it is correct:

[<Test>]
let ``require to ident at least +1 after function name #545 (long expression and short line settings)``() =
    formatSourceString false @"
let a s =
    if s <> """" then
        printfn """"""fooo
%s
%s
%s
%s""""""                (llloooooooooooooooooooooooooo s)
                            s
                               (llloooooooooooooooooooooooooo s)
                                     (llloooooooooooooooooooooooooo s)"
        { config  with PageWidth = 50 } 
    |> prepend newline
    |> should equal @"
let a s =
    if s <> """" then
        printfn """"""fooo
%s
%s
%s
%s""""""    (llloooooooooooooooooooooooooo s) s
            (llloooooooooooooooooooooooooo s)
            (llloooooooooooooooooooooooooo s)
"

Some of the parameters for printfn are newlined while others aren't. Is that intended for multiline strings?

nojaf · 2020-06-29T20:43:34Z

Hey @Bobface , yes that test looks a bit weird but it still makes sense I think:

So when there is a multiline string in place, the old formatting will be used.
And there is the newline function is determined by

    let appNlnFun e =
        match e with
        | MultilineString _
        | Lambda _
        | Paren (Lambda _)
        | Paren (MatchLambda _) -> id
        | _ -> autoNlnIfExpressionExceedsPageWidth

so for the MultilineString we don't add a newline, yet for all the other parameter we add a newline when they reach the page width threshold.
Fantomas just doesn't always consider multiline strings as multiline expressions, otherwise you would have a newline for each multiline string and you might not want that.

@jindraivanek would you mind reviewing this as well please?

nojaf requested changes Jun 24, 2020

View reviewed changes

Bobface force-pushed the parameter-newline branch from 3c81b03 to be541d6 Compare June 25, 2020 09:13

nojaf reviewed Jun 26, 2020

View reviewed changes

Bobface force-pushed the parameter-newline branch from b4444b8 to d28d6e9 Compare June 29, 2020 08:12

Split all parameters of a function application over multiple lines if…

e5e4b7d

… they exceed page width

Bobface force-pushed the parameter-newline branch from d28d6e9 to e5e4b7d Compare June 29, 2020 08:13

Bobface marked this pull request as ready for review June 29, 2020 08:24

Bobface changed the title ~~WIP: Split all parameters of a function application over multiple lines if they exceed page width~~ Split all parameters of a function application over multiple lines if they exceed page width Jun 29, 2020

nojaf self-requested a review June 29, 2020 20:43

nojaf approved these changes Jun 29, 2020

View reviewed changes

nojaf requested a review from jindraivanek June 29, 2020 20:44

jindraivanek approved these changes Jun 30, 2020

View reviewed changes

Merge branch 'master' into parameter-newline

079b932

nojaf merged commit dcc70c3 into fsprojects:master Jun 30, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Split all parameters of a function application over multiple lines if they exceed page width #929

Split all parameters of a function application over multiple lines if they exceed page width #929

Bobface commented Jun 24, 2020

Bobface commented Jun 24, 2020

nojaf left a comment

nojaf Jun 24, 2020

Bobface Jun 25, 2020

Bobface Jun 25, 2020

nojaf Jun 26, 2020

nojaf Jun 24, 2020

nojaf Jun 24, 2020

nojaf Jun 24, 2020

Bobface Jun 25, 2020

nojaf Jun 25, 2020

Bobface Jun 25, 2020

nojaf Jun 26, 2020

nojaf Jun 26, 2020

nojaf commented Jun 26, 2020

Bobface commented Jun 29, 2020

nojaf commented Jun 29, 2020 •

edited

Loading

Split all parameters of a function application over multiple lines if they exceed page width #929

Split all parameters of a function application over multiple lines if they exceed page width #929

Conversation

Bobface commented Jun 24, 2020

Bobface commented Jun 24, 2020

nojaf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nojaf commented Jun 26, 2020

Bobface commented Jun 29, 2020

nojaf commented Jun 29, 2020 • edited Loading

nojaf commented Jun 29, 2020 •

edited

Loading