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

Guidance for MatchLambda as argument #25207

Closed
nojaf opened this issue Jul 18, 2021 · 9 comments · Fixed by #25764
Closed

Guidance for MatchLambda as argument #25207

nojaf opened this issue Jul 18, 2021 · 9 comments · Fixed by #25764

Comments

@nojaf
Copy link
Contributor

nojaf commented Jul 18, 2021

Hello, in the section Formatting function parameter application it is mentioned that

Parameters should generally be indented relative to the function or fun/function keyword, regardless of the context in which the

However, there is no example given for function | Some _ -> x | None -> y expressions.
Consider the following code:

module Foo =
    let bar =
        []
        |> List.choose 
            (function
             | _ -> "")

Would it be formatted like this or follow the rule of Formatting constructors, static members, and member invocations?

Something like

module Foo =
    let bar =
        []
        |> List.choose (
            function
            | _ -> ""
        )

Document Details

Do not edit this section. It is required for docs.microsoft.com ➟ GitHub issue linking.

@nojaf
Copy link
Contributor Author

nojaf commented Jul 18, 2021

Just thinking out loud, but as Philip is leaving MS, is there any chance @dsyme could perhaps pick up these questions instead?

@dsyme
Copy link
Contributor

dsyme commented Jul 18, 2021

If you @dsyme me I'll do my best,

For this code

module Foo =
    let bar =
        []
        |> List.choose 
            (function
             | _ -> "")

I would format like this I think:

module Foo =
    let bar =
        []
        |> List.choose (function
             | _ -> "")

or more usually with constructors

module Foo =
    let bar =
        []
        |> List.choose (function
            | A _ -> "a"
            | B _ -> "")

@dsyme
Copy link
Contributor

dsyme commented Jul 18, 2021

So I basically disagree with the guidance there. The indentation after fun/function used in end-of-line position should be with respect to the first character of the previous line, e.g.

module Foo =
    let bar =
        data
        |> List.map (fun x ->
            match x,y with
            | "a" -> A1
            | _ -> B 2)
        |> List.choose2 (fun x y ->
            match x,y with
            | A _, B_ -> "ab"
            | B _, A _ -> "ba")

Indeed I almost never indent w.r.t. fun/function

@nojaf
Copy link
Contributor Author

nojaf commented Jul 19, 2021

Hey Don, thanks for your input. It is both interesting and scary at the same time 😄.
I think what you suggested makes sense though it does conflict with the existing lore here.
And revisiting previous guidance can be quite impactful for Fantomas.

As you may or may not know, Fantomas uses this document to base its default formatting style.
A change here reflects a change of out-of-the-box experience of Fantomas.
So, what I'm trying to say is that we should not go over this lightly.

Back to the matter at hand, how would you format the following code snippets?
And more importantly, what is the rationale behind it?

// lambda inside parenthesis
let square =
    (fun b ->
        b * b
        prinftn "%i" b * b)

// other parameters besides the lambda in a function application
Target.create
    "Clean"
    (fun _ ->
        [ "bin"
          "src/Fantomas/bin"
          "src/Fantomas/obj"
          "src/Fantomas.CoreGlobalTool/bin"
          "src/Fantomas.CoreGlobalTool/obj" ]
        |> List.iter Shell.cleanDir)

// destructuring of lambda parameters
// note that I'm hinting at what happens if the lambda parameters become long
List.filter
    (fun ({ ContentBefore = contentBefore }) ->
        // some comment
        let a = 8
        let b = List.length contentBefore
        a + b)

// multiple lambda arguments
let init =
    addDateTimeConverter
        (fun dt -> Date(dt.Year, dt.Month, dt.Day))
        (fun (Date (y, m, d)) -> System.DateTime(y, m, d))

// multiline arguments where some are lambdas
SettingControls.toggleButton
    (fun _ ->
        UpdateOption(key, MultilineFormatterTypeOption(o, key, "character_width"))
        |> dispatch)
    (fun _ ->
        UpdateOption(key, MultilineFormatterTypeOption(o, key, "number_of_items"))
        |> dispatch)
    "CharacterWidth"
    "NumberOfItems"
    key
    (v = "character_width")

// lambda argument of DotGet expression
services
    .AddAuthentication(fun options ->
        options.DefaultScheme <- "Cookies"
        options.DefaultChallengeScheme <- "oidc")
    .AddCookie("Cookies")
    .AddOpenIdConnect(fun options ->
        options.Authority <- "http://localhost:7001"
        options.ClientId <- "mvc"
        options.ClientSecret <- "secret"
        options.ResponseType <- "code"
        options.SaveTokens <- true)

@dsyme
Copy link
Contributor

dsyme commented Jul 19, 2021

Yes I was surprised at the guidance here, and understand it's disturbing to change things. We can take our time and make sure we get this right.

Back to the matter at hand, how would you format the following code snippets?
And more importantly, what is the rationale behind it?

For me each of those would be formatted as you have it. The rules would be something like this?

If (fun <pat> -> or (function -> occur as the final argument in an application (after taking into account the need for fewer arguments due to piping with |> or ||> or |||>), then, when deciding whether to break the arguments into mutiple lines or not, the (fun <pat> -> or (function -> text can be considered as one argument.

If -> occurs as final position on a line, then the next token is indented four spaces from the first character of that line.

The one place where this might change things is

Target.create
    "Clean"
    (fun _ ->
        [ "bin"
          "src/Fantomas/bin"
          "src/Fantomas/obj"
          "src/Fantomas.CoreGlobalTool/bin"
          "src/Fantomas.CoreGlobalTool/obj" ]
        |> List.iter Shell.cleanDir)

which might be formatted as follows if the overall decision is to format the arguments to create on a single line? This seems a reasonable and consistent formatting to me?

Target.create "Clean" (fun _ ->
    [ "bin"
      "src/Fantomas/bin"
      "src/Fantomas/obj"
      "src/Fantomas.CoreGlobalTool/bin"
      "src/Fantomas.CoreGlobalTool/obj" ]
    |> List.iter Shell.cleanDir)

Regarding this one:

// note that I'm hinting at what happens if the lambda parameters become long

Certainly if the pattern text doesn't fit on a single line then this rule doesn't apply. I guess my intuition is that in someFunction arg1 (fun <pat> -> the text (fun <pat> -> counts as an overall argument - if the overall line needs breaking then the priority would be to split into multiple arguments.

However even if (fun <pat> -> is split onto a single line, the next token is still indent four spaces w.r.t. the ( and not the fun.

There will be cases where fun doesn't have a leading paranthesis. e.g.

let f1() =
    someFunction <| fun smallPattern -> someLargeExpression

let f2() =
    someFunction <| fun largePattern -> someLargeExpression

I guess these are

let f1() =
    someFunction <| fun smallPattern ->
        someLargeExpression

let f2() =
    someFunction <| 
        fun largePattern ->
            someLargeExpression

The latter is a case where the fun acts as the indent delimiter (again because it is the first token on the line ended by ->). I guess the rule would be extended, e.g. this?

If <| fun <pat> -> or <| function <pat> -> occur as the final argument in an application , then, when deciding whether to break the arguments into mutiple lines or not, that text can be considered as one argument.

@nojaf
Copy link
Contributor Author

nojaf commented Jul 19, 2021

If (fun -> or (function -> occur in final position as a line, then the next token is indented four spaces from the first character of that line.

Is there any impact on this when the lambda is followed by any other arguments.
Example

let foo =
    List.mapi (fun idx element ->
        // some comment
        div [ Key idx] [ element ]) myList 

(fun idx element -> ends the line maybe but it does not end the expression.
Where should the myList and why? Is the rule the same if it is followed by multiple argments?

@dsyme
Copy link
Contributor

dsyme commented Aug 11, 2021

Is there any impact on this when the lambda is followed by any other arguments.

I don't have a strong preferred style for this. I can certainly see the case for this for consistency:

let foo =
    List.mapi 
        (fun idx element ->
            // some comment
            div [ Key idx] [ element ])
        myList 

The problem however is when there are very many curried arguments, e.g.

let foo =
    List.mapi 
        (fun idx element ->
            // some comment
            div [ Key idx] [ element ])
        myList 
        myList 
        myList 
        myList 
        myList 
        myList 

Here I find myself doing this:

let foo =
    List.mapi 
        (fun idx element ->
            // some comment
            div [ Key idx] [ element ])
        myList myList myList myList myList myList 

However I'm aware of the inconsistencies this leads to, and part of the problem is the over-reliance on massive argument lists. I don't have any strong preference for a consistent rule a-priori. I'll now look at the test cases in the PR you linked above and see how they feel.

@nojaf
Copy link
Contributor Author

nojaf commented Aug 12, 2021

Thanks, for simplicity sake I think we could go with following the same behaviour as other long arguments (being on the next lines with one indent). This will not always look great, but for most cases, this would be acceptable.

I believe all my questions have been answered. I will create a PR for the style guide.

@dsyme
Copy link
Contributor

dsyme commented Aug 12, 2021

OK, thsnk you!

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