Extract setting for preference of newline placement for computation expressions

[josh-degraw]

I'm very open to bike-shedding the name of the setting here, I just went with what made the most sense in my head at the time but I'm open to alternatives if something makes more sense.

Closes #2276

[nojaf]

Hey @josh-degraw, sorry for the slow response. I've been travelling last week.

From afar this PR looks good. I would explore the option to port some of the Oak code to Context.

let isStroustrup (config: FormatConfig) (e: Expr) =
    let isMultiRecord = config.MultilineBracketStyle = ExperimentalStroustrup

    match e with
    | Expr.Record node when isMultiRecord ->
        match node.Extra with
        | RecordNodeExtra.Inherit _
        | RecordNodeExtra.With _ -> false
        | RecordNodeExtra.None -> true
    | Expr.AnonRecord node when isMultiRecord ->
        match node.CopyInfo with
        | Some _ -> false
        | None -> true
    | Expr.ArrayOrList _ when isMultiRecord -> true
    | Expr.NamedComputation _ when config.PreferComputationExpressionNameOnSameLine -> true
    | _ -> false

I would however like to have another name for this.
The Prefer in PreferComputationExpressionNameOnSameLine really rubs me the wrong way. The word prefer is a bit excessive, given that it is a setting, it already implies a preference. On the top of my head, I would go in the direction of StroustrupComputationExpression. I'm not sure that exact name is what we want though, but I think Stroustrup is just so well known on the streets that I would somehow incorporate it.

If I understand the code right now, you only need to enable the setting to do something.
Having MultilineBracketStyle = Stroustrup is not required to enable the code. If that is the case, I do like that.

Lastly, this would need documentation. Perhaps we need to update the configuration.fsx script to use a local version of Fantomas.Core instead of a version on NuGet. Otherwise, we always need to play catch-up.

[nojaf]

If there is TriviaBefore in the NamedComputation that should be captured as well.
Example:

[<Test>]
let ``prefer computation expression name on same line, with trivia`` () =
    formatSourceString
        false
        """
let t =
    //
    task {
        let! thing = otherThing ()
        return 5
    }
"""
        config
    |> prepend newline
    |> should
        equal
        """
let t =
    //
    task {
        let! thing = otherThing ()
        return 5
    }
"""

[nojaf]

Just a thought but I think this member should be extracted into a function in Context.
If you have that, you can pass the FormatConfig into it and just add an extra when expression for the named computation expression.

I think this makes sense as it further removes the coupling between the Oak model and any coding style.

By refactoring this, I think the current flow could be preserved slightly easier.

[josh-degraw]

If I understand the code right now, you only need to enable the setting to do something. Having MultilineBracketStyle = Stroustrup is not required to enable the code. If that is the case, I do like that.

Yep, that was the goal, this setting should be able to be enabled whether or not the user has enabled Stroustrup globally.

I would however like to have another name for this. The Prefer in PreferComputationExpressionNameOnSameLine really rubs me the wrong way. The word prefer is a bit excessive, given that it is a setting, it already implies a preference.

I agree the name is pretty chunky, I think removing Prefer makes total sense.

On the top of my head, I would go in the direction of StroustrupComputationExpression. I'm not sure that exact name is what we want though, but I think Stroustrup is just so well known on the streets that I would somehow incorporate it.

I see what you mean, the only hesitation I have to that is that like you mentioned this is intended to be fully orthogonal to the Stroustrup setting, and I worry that it could cause confusion to have the name on this setting. I think ComputationExpressionNameSameLine or SameLineComputationExpressionName or ShortComputationExpressions or CompressComputationExpressions might work, but I'm not in love with either of them either ¯\(ツ)/¯.

[nojaf]

I have to that is that like you mentioned this is intended to be fully orthogonal to the Stroustrup setting, and I worry that it could cause confusion to have the name on this setting.

Yeah, I can see why you don't want to attach it to the Stroustrup brand. It is less obvious to think about computation expressions in that way.

I'm not overly crazy about the SameLine wording. That feels a bit clunky still. However, no clue how to improve it. ComputationExpressionStartOnCurrentLine, I think Start is more general instead of Name (technically it could be an expression and not only a name). I'm not sure CurrentLine is any better than SameLine ¯(ツ)/¯.

[josh-degraw]

An alternative could be to do something like NewlineBeforeComputationExpression that defaults to true? I like the name better but would it be confusing at all to have a setting that defaults to true? I originally thought so but now I'm not sure.

[nojaf]

Oh, I like that one. Maybe NewlineBeforeMultilineComputationExpression to remove all ambiguity.
(Although it becomes long again that way).

[josh-degraw]

Yeah that's longer but I think I'm fine with that. It's long but it still doesn't feel quite as clunky as my original suggested name, IMO. I'm fine with that if you are.

[nojaf]

This looks good! The only thing I'm really missing is a documentation entry.
I've changed the script to use a local Fantomas.Core instead of grabbing it from NuGet.
Could you add a section for the new setting, please?

[nojaf]

Great work! I'm very happy you pulled this off with minimal changes.