MaxValueBindingWidth and MaxFunctionBindingWidth settings

[Bobface]

This PR will implement the change from a single MaxLetBindingWidth setting to two separate settings MaxValueBindingWidth and MaxFunctionBindingWidth in two steps:

1. Rename MaxLetBindingWidth setting to MaxBindingWidth
2. Split MaxBindingWidth into two separate settings MaxValueBindingWidth and MaxFunctionBindingWidth

The new settings while handle these situations:

• MaxValueBindingWidth: Handles values like let someValue = 1
• MaxFunctionBindingWidth: Handles function definitions like let someFunction someParam = someParam + 1

[Bobface]

@nojaf The first phase is done. Please have a look. This has been done so far:

• Rename MaxLetBindingWidth to MaxBindingWidth
• Make members respect MaxBindingWidth instead of only PageWidth
• Update exisiting test cases which have changed because members now respect MaxBindingWidth
• Add new tests for MaxBindingWidth

When we get this stage to an acceptable level I will continue by splitting MaxBindingWidth into two separate settings MaxValueBindingWidth and MaxFunctionBindingWidth

[knocte]

@nojaf is this good so far? if yes, @Bobface will implement the rest

[nojaf]

@knocte I still need to check on @Bobface last remark.
I would like to debug this to see if we really need the ignoreShort change.

[nojaf]

So I took a look and tried to confirm my initial suspicions: this PR has more code changes than needed.
As an experiment, I copied the new unit tests and renamed the setting to see how the same result could have been achieved with fewer changes.

Two things to note:

• There still might be some room for refactoring the right abstractions to Context.
• I haven't updated the Typed MemberBinding (deliberately) and all tests still pass so extra tests are needed there. Try to do this in a TDD style.

@Bobface could you revisit this PR based on my findings?
As for the ShortExpression mode, this can be confusing from time to time when debugging but you shouldn't disable it. It will eventually pass through and try the fallback expressions. The thing is you can have nested expressions that both rely on ShortExpression mode, so that might have been something you encountered.

[Bobface]

@nojaf I've updated the changes to use your implementation for the CodePrinter. The changes to the Context have been removed.

[nojaf]

@Bobface could you add a test for when the member binding is typed, please.

[Bobface]

@nojaf Done.

[nojaf]

Hey @Bobface thanks for adding the test.
I think we can move forward to splitting the setting into MaxValue and MaxBinding if that is still the plan for this PR.

[Bobface]

@nojaf I've split the setting into two.

There is currently one failing test:
https://github.com/fsprojects/fantomas/pull/866/files#diff-22a481f935212a16b8e8933b52bdd9c4L32-L61

[<Test>]
let ``active patterns``() =
    formatSourceString false """
let (|Even|Odd|) input = if input % 2 = 0 then Even else Odd
let (|Integer|_|) (str: string) =
   let mutable intvalue = 0
   if System.Int32.TryParse(str, &intvalue) then Some(intvalue)
   else None
let (|ParseRegex|_|) regex str =
   let m = Regex(regex).Match(str)
   if m.Success
   then Some (List.tail [ for x in m.Groups -> x.Value ])
   else None""" ({ config with MaxLetBindingWidth = 30 })
   else None""" ({ config with MaxValueBindingWidth = 30; MaxFunctionBindingWidth = 120 })
    |> prepend newline
    |> should equal """
let (|Even|Odd|) input =
    if input % 2 = 0 then Even else Odd
let (|Integer|_|) (str: string) =
    let mutable intvalue = 0
    if System.Int32.TryParse(str, &intvalue) then Some(intvalue) else None
let (|ParseRegex|_|) regex str =
    let m = Regex(regex).Match(str)
    if m.Success
    then Some(List.tail [ for x in m.Groups -> x.Value ])
    else None
"""

The reason is that the active pattern is not identified as function by the helper:
https://github.com/fsprojects/fantomas/pull/866/files#diff-23c516460c0edb3213f811dc4261fc48R1408-R1411

let isFunctionBinding (p: SynPat) =
    match p with
    | PatLongIdent(_, _, ps, _) when (List.isNotEmpty ps) -> true
    | _ -> false 

Maybe you could give me a hint how I can make that helper match active patterns as functions?

[nojaf]

@Bobface on top my head I think you can get arround it with some regex magic.
Take this example.
If the SynPat looks like

Named
                    (Wild tmp.fsx (2,5--2,14) IsSynthetic=false,|Other|_|,false,
                     None,tmp.fsx (2,5--2,14) IsSynthetic=false),

Maybe do a regex to matches |something|_|?

Or maybe try and deduce the information from SynBinding -> SynValData -> SynValInfo -> SynArgInfo. See docs for more info.

[Bobface]

@nojaf Nevermind. isFunctionBinding actually detected the active pattern as function. The problem was that in the failing test MaxFunctionBindingWidth was set to 120. I've updated it to 30 and the test passes now.

[nojaf]

Nice work @Bobface!

[Bobface]

@nojaf Thanks! I have removed the unused helper function, it did not have anything to do with this PR.

[nojaf]

@Bobface I will most likely release a new alpha by the end of the week.