[Feature request] GR style tweaks

[JackMatusiewiczGR]

We ran the latest version of Fantomas on our code and have a list of minor issues we have with the formatting style. Some of these may well be due to our incorrect config and we are going to be double checking to see if that is the case of some of these.

To begin with, here are our config settings:

fsharp_space_before_uppercase_invocation=true
fsharp_space_before_class_constructor=true
fsharp_space_before_member=true
fsharp_space_before_colon=true
fsharp_space_before_semicolon=true
fsharp_indent_on_try_with=true
fsharp_multiline_block_brackets_on_same_column=true
fsharp_newline_between_type_definition_and_members=true
fsharp_align_function_signature_to_indentation=true
fsharp_alternative_long_member_definitions=true

1. Casting operator not being treated like other pipe operators

What we had:

Reflection.invokeStaticGenericMethod
    <@ someFunctionHere @>
    [someParameterHere]
    [someMoreParemetersHere]
    :?>

What fantomas did:

Reflection.invokeStaticGenericMethod
    <@ someFunctionHere @>
    [ someParameterHere ]
    [ someMoreParemetersHere ] :?>

What we wanted:

Reflection.invokeStaticGenericMethod
    <@ someFunctionHere @>
    [ someParameterHere ]
    [ someMoreParemetersHere ]
:?>

2. List function parameter formatting

What we had:

someFunctionHere [parameters] [yetMoreParameters]
:?> _

What fantomas did:

someFunctionHere [ parameters ] [
    yetMoreParameters
] :?> _

What we wanted:

someFunctionHere
    [ parameters ]
    [ yetMoreParameters ]
:?> _

3. Formatting a list of tuples that didn't hit out line limit

What we had:

[MyType.ofPartialThing path, leafObject]

What fantomas did:

[
    MyType.ofPartialThing path, leafObject
]

What we wanted:

[ MyType.ofPartialThing path, leafObject ]

4. Ending parenthesis for lambda functions passed into HOFs

What we had:

List.collect (fun (a, element) ->
    let path' = path |> someFunctionToCalculateThing

    innerFunc<'a, 'b> path' element (foo >> bar value >> List.item a) shape)

What fantomas did:

List.collect (fun (a, element) ->
    let path' =
        path
        |> someFunctionToCalculateThing

    innerFunc<'a, 'b>
        path'
        element
        (foo >> bar value >> List.item a)
        shape)

What we wanted:

List.collect (fun (a, element) ->
    let path' =
        path
        |> someFunctionToCalculateThing

    innerFunc<'a, 'b>
        path'
        element
        (foo >> bar value >> List.item a)
        shape
)

5. Tuple formatting when not near line limit

What we had:

| Leaf.Func getFunc -> iterator.Iterate (path, getFunc >> fun f -> f.Invoke)

What fantomas did:

| Leaf.Func getFunc ->
    iterator.Iterate
        (path,
         getFunc
         >> fun f -> f.Invoke)

What we wanted:

| Leaf.Func getFunc ->
    iterator.IterProv
        (path, getFunc >> fun f -> f.Invoke)

or

| Leaf.Func getFunc ->
    iterator.IterProv
        (path,
         getFunc >> fun f -> f.Invoke)

6. More Tuple formatting

What we had

|> Seq.map (Tuple.rmap (MyLongLeafPath.toInnerPath >> InnerPathType.toHumanReadableString))
|> Seq.map (fun (added, str) -> sprintf "%s - %s" (if added then addedString else removedString) str)

What fantomas did

|> Seq.map
    (Tuple.rmap
        (MyLongLeafPath.toInnerPath
        >> InnerPathType.toHumanReadableString))
|> Seq.map (fun (added, str) -> sprintf "%s - %s" (if added then addedString else removedString) str)

What we wanted:

|> Seq.map
    (Tuple.rmap
        (MyLongLeafPath.toInnerPath >> InnerPathType.toHumanReadableString)
    )
|> Seq.map (fun (added, str) -> sprintf "%s - %s" (if added then addedString else removedString) str)

7. More parameter spacing

What we had:

InstanceObject.make [a; b; c]

What fantomas did:

InstanceObject.make [ a
                                     b
                                     c ]

What we wanted:

InstanceObject.make
    [a ; b ; c]

8. Constructor formatting

What we had:

new ParquetWriter(
    file.FullName,
    Column.Thing
    WriteProperties.make columns,
    dict metadata |> Dictionary
)

What fantomas did:

new ParquetWriter( file.FullName,
                                Column.Thing
                                WriteProperties.make columns,
                                dict metadata |> Dictionary)

What we wanted:

new ParquetWriter(
    file.FullName,
    Column.Thing
    WriteProperties.make columns,
    dict metadata |> Dictionary
)

Will update this as I check to make sure these aren't already solved by config we aren't using.

Thanks,
Jack

[nojaf]

Hey @JackMatusiewiczGR,

Some initial feedback:

1. could you complete all the samples so they at the very least produce something in our online tool.
   Example 1 doesn't parse for example.

2. is related to the Elmish formatting. someFunctionHere [parameters] [yetMoreParameters] matches the formatting explained in Elmish guide.
   I don't want to be "that guy", but I did raise this during a call in the Summer 😋. In short, I'll add something to disable this behaviour.

3. [ MyType.ofPartialThing path, leafObject ], is crossing the 40 character limit of fsharp_max_array_or_list_width.
   Having this on 42 for example does what you want.

4. This is an interesting case, I do agree that it one of those things I don't like about Fantomas myself personally. I think the scope is bigger than the example you posted. I'll create a separate feature request to flesh this one out.

5. Can I get a more complete sample of this? I'm guessing that it is nested code and near the end of the max_line_length.
   At the beginning of a line, it seems ok.

6. Seems to contradict 4. a little. Why is it not

[]
|> Seq.map (
    Tuple.rmap (
        MyLongLeafPath.toInnerPath
        >> InnerPathType.toHumanReadableString
    )
   )
|> Seq.map (fun (added, str) -> sprintf "%s - %s" (if added then addedString else removedString) str)

in this case? Is it tuple versus lambda?

7. Full example, please. It might be related to 2.

8. I should look into that one to see if there is any current reasoning behind this.

As we tackle some of these things, we should also update the style guide accordingly.

Thanks for writing all of this down, much appreciated!

[Smaug123]

I think what we need to resolve 4/6 is some more detailed style guidelines. I've documented our style in G-Research/fsharp-formatting-conventions#10 . I think your version is more consistent; let me know if you think the guidelines still don't pin down the right answer here.

[Smaug123]

Number 7 was fixed by disabling the Elmish formatting.

[Smaug123]

Re 8: I've raised a discussion over at G-Research/fsharp-formatting-conventions#11 about what the right thing to do here is.

[knocte]

Re 4: I'd be interested in this too.

[nojaf]

Hey @JackMatusiewiczGR and @Smaug123, in regards to 4.

    innerFunc<'a, 'b>
        path'
        element
        (foo >> bar value >> List.item a)
        shape

a function call is only multiline now if it cannot respect the max length boundary.
Was the example intended that way? Or are we missing another setting to control the max length of function calls?
Such a thing would not be that difficult to set up I think.

[Smaug123]

That looks fine to me. There are cases where we sometimes split function calls across lines for clarity even if they would fit on one line, but nothing principled.

[nojaf]

I'm going to close this as most of the mentioned items have been resolved by now.
Feel free to open new issues, when there is still anything missing from the G-Research style guide.