fsharp / fslang-design

RFCs and docs related to the F# language design process, see https://github.com/fsharp/fslang-suggestions to submit ideas
518 stars 144 forks source link

[style-guide] Chain of (fluent) calls #688

Open sergey-tihon opened 2 years ago

sergey-tihon commented 2 years ago

We faced an issue that current version of Fantomas format one liner

Cache.providedTypes.GetOrAdd(cacheKey, addCache).Value

like this

Cache
    .providedTypes
    .GetOrAdd(
        cacheKey,
        addCache
    )
    .Value

In my opinion such formation is very verbose and @nojaf mentioned that there is no guidance in style guide how to format such code.

In my opinion formatting should be similar to pipeline expressions and look at least like this

Cache
    .providedTypes
    .GetOrAdd(cacheKey, addCache)
    .Value
nojaf commented 2 years ago

Hello Sergey,

Thanks for bringing this up, this topic can definitely be improved. This is however a bit more complex. Some more examples for the community to chew on:

What should happen with long prefixes?

Microsoft.FSharp.Reflection.FSharpType.GetUnionCases(typeof<option<option<unit>>>.GetGenericTypeDefinition().MakeGenericType(t)).Assembly

System
    .Diagnostics
    .FileVersionInfo
    .GetVersionInfo(
        System
            .Reflection
            .Assembly
            .GetExecutingAssembly()
            .Location
    )
    .FileVersion

There are some offset rules when there is a lambda involved:

let getColl3 =
    GetCollection(fun _ parser ->
        let x = 3
        x)
        .Foo
            configuration
                .MinimumLevel.Debug()
                .WriteTo.Logger(fun loggerConfiguration ->
                    loggerConfiguration
                        .Enrich.WithProperty("host", Environment.MachineName)
                        .Enrich.WithProperty("user", Environment.UserName)
                        .Enrich.WithProperty("application", context.HostingEnvironment.ApplicationName)
                    |> ignore
                )
nojaf commented 1 year ago

Hello,

I would like to take a stab at this. It would be great if this area was improved in the style guide and Fantomas.

Definitions

First of I'd like to define a chain as a sequence of links separated by dots. A link could be:

And a simple link is considered to be either an identifier or an index expression.

Ruleset

If a chain fits on the maximum threshold, it should be on a single line:

A.B().A

When a chain no longer fits, it becomes multiline. The first line of the chain would be a collection of simple links. The rest of the chain would be indented on the next line.

First-line examples:

A.A.A
    .rest...

A<'a>.A.A
    .rest...

A.[0].A
    .rest...

The rest of the chain would be indented:

A.A.A
   .B()
   .C(c)

Every link that is not a simple link would introduce a new line. A simple link can prepend a non-simple link.

A.A.A
    .B()
    .A.C(c)
    .A.A.D(fun d -> d)
    .A

When an application does not fit on the remainder of the line it goes multiline similar as if it were an application outside of a chain:

A.A.A
    .C(
        c1,
        c2,
        c3,
        c4
    )
    .D(fun d ->
        // comment
        d
    )

The inner expression inside the parentheses is indented further. When that expression is a lambda only the body of the lambda is indented further.

The application is considered multiline purely on evaluating its own content. So you could end up with a mix of short and long applications inside the chain.

// OK ✅
A
    .C(short)
    .C(
        long1,
        long2,
        long3
    )

// Not OK ❌
A
    .C(
        short
     )
    .C(
        long1,
        long2,
        long3
    )

Rationale

The reasoning behind this suggestion is the following:

Examples

See https://github.com/fsprojects/fantomas/pull/2696, you can look at the test files to see what the effect is.

Equinox.MemoryStore
    .Resolver(store, FsCodec.Box.Codec.Create(), fold, initial)

configuration.MinimumLevel
    .Debug()
    .WriteTo.Logger (fun loggerConfiguration ->
        loggerConfiguration.Enrich
            .WithProperty("host", Environment.MachineName)
            .Enrich.WithProperty("user", Environment.UserName)
            .Enrich.WithProperty (
                "application",
                context.HostingEnvironment.ApplicationName
            )

System.Diagnostics.FileVersionInfo
    .GetVersionInfo(
        System.Reflection.Assembly
            .GetExecutingAssembly()
            .Location
    )

Limitations

Not everything with a dot in it would be considered a chain. For example List.map (fun e -> e) would still follow the existing rules of an application.

dsyme commented 1 year ago

This looks so good

baronfel commented 1 year ago

I very much like the principled groupings/naming of the different kinds of applications/fluent calls. That makes it much easier to determine and talk about a set of rules for the different scenarios. I agree with Don - this looks amazing!

nojaf commented 1 year ago

Hello @dsyme, I tried this out on some more code bases and the results were favourable. I believe we can go ahead with this.

nojaf commented 1 year ago

Style guide PR: https://github.com/dotnet/docs/pull/33524

nojaf commented 1 year ago

This is available in Fantomas v5.2, the issue can be closed.