Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
sergey-tihon opened this issue Jul 5, 2022 · 7 comments · Fixed by dotnet/docs#33524
Open

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

sergey-tihon opened this issue Jul 5, 2022 · 7 comments · Fixed by dotnet/docs#33524

Comments

@sergey-tihon
Copy link

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
Copy link
Contributor

nojaf commented Jul 6, 2022

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
Copy link
Contributor

nojaf commented Jan 9, 2023

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:

  • A simple identifier (A or A<'a>)
  • An application with parentheses (B(), C(c), D(fun d -> d))
  • An index between [ ] (.[e])

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:

  • The start of the chain will typically be a prefixed namespace. So you want to put that on the same line as it doesn't do anything interesting yet (in comparison to a function application).
  • In order to save some newlines the simple links are combined when possible.

Examples

See fsprojects/fantomas#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
Copy link
Contributor

dsyme commented Jan 9, 2023

This looks so good

@baronfel
Copy link
Contributor

baronfel commented Jan 9, 2023

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
Copy link
Contributor

nojaf commented Jan 12, 2023

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
Copy link
Contributor

nojaf commented Jan 13, 2023

Style guide PR: dotnet/docs#33524

@nojaf
Copy link
Contributor

nojaf commented Jan 20, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants