fsprojects / fantomas

FSharp source code formatter
https://fsprojects.github.io/fantomas
Other
772 stars 194 forks source link

Newline inconsistency in object expression #2990

Open MangelMaxime opened 11 months ago

MangelMaxime commented 11 months ago

Issue created from fantomas-online

Code

let helpers =
    { new IDecoderHelpers<JToken> with
        member _.isString jsonValue =
            not (isNull jsonValue) && jsonValue.Type = JTokenType.String
        member _.asFloat jsonValue = jsonValue.Value<float>()
        member _.asInt jsonValue = jsonValue.Value<int>()
        member _.getObjectKeys jsonValue =
            jsonValue.Value<JObject>().Properties()
            |> Seq.map (fun prop -> prop.Name)
        member _.getField(fieldName: string, jsonValue: JToken) = jsonValue[fieldName]

    }

let helpers2 =
    { new IDecoderHelpers<JToken> with
        member _.anyToString jsonValue =
            if isNull jsonValue then
                "null"
            else
                use stream = new StringWriter(NewLine = "\n")

                stream.ToString()
    }

Result

let helpers =
    { new IDecoderHelpers<JToken> with
        member _.isString jsonValue =
            not (isNull jsonValue) && jsonValue.Type = JTokenType.String

        member _.asFloat jsonValue = jsonValue.Value<float>()
        member _.asInt jsonValue = jsonValue.Value<int>()

        member _.getObjectKeys jsonValue =
            jsonValue.Value<JObject>().Properties() |> Seq.map (fun prop -> prop.Name)

        member _.getField(fieldName: string, jsonValue: JToken) = jsonValue[fieldName]

    }

let helpers2 =
    { new IDecoderHelpers<JToken> with
        member _.anyToString jsonValue =
            if isNull jsonValue then
                "null"
            else
                use stream = new StringWriter(NewLine = "\n")

                stream.ToString() }

Problem description

I believe it would be better if there was a new line between each member, this would make the code more consistent. I think that if all the member are single line then not having a new line is fine.

Also, the placement of the trailing } is not always placed on a new line. I don't know what triggers that because in my project, I have a similar code as the member _.anyToString jsonValue = method and the } is placed on a new line and indented under the leading {.

Extra information

Options

Fantomas main branch at 1/1/1990

    { config with
                AlternativeLongMemberDefinitions = true
                MultiLineLambdaClosingNewline = true }

Did you know that you can ignore files when formatting by using a .fantomasignore file? PS: It's unlikely that someone else will solve your specific issue, as it's something that you have a personal stake in.

nojaf commented 11 months ago

Hi there,

I believe it would be better if there was a new line between each member, this would make the code more consistent. I think that if all the member are single line then not having a new line is fine.

This is a topic for the style guide.

Also, the placement of the trailing } is not always placed on a new line.

In helpers, } is placed on a new line because it had a blank line before it. That was preserved from the original code. This works as expected.

MangelMaxime commented 11 months ago

In helpers, } is placed on a new line because it had a blank line before it. That was preserved from the original code. This works as expected.

You say that } is placed on a new line because there is blank line before however the following snippet results in the same formatting:

let helpers =
    { new IEncoderHelpers<JToken> with
        member _.encodeUInt32 value = JValue(box value) }

let helpers2 =
    { new IEncoderHelpers<JToken> with
        member _.encodeUInt32 value = JValue(box value) 
    }

Result

let helpers =
    { new IEncoderHelpers<JToken> with
        member _.encodeUInt32 value = JValue (box value)
    }

let helpers2 =
    { new IEncoderHelpers<JToken> with
        member _.encodeUInt32 value = JValue (box value)
    }

In both cases the } is pushed to a new line under the leading {.

Options

[*.{fs,fsx}]
max_line_length = 80
fsharp_space_before_uppercase_invocation = true
fsharp_alternative_long_member_definitions = true
fsharp_multi_line_lambda_closing_newline = true
fsharp_bar_before_discriminated_union_declaration = true
fsharp_multiline_bracket_style = stroustrup
fsharp_keep_max_number_of_blank_lines = 2
nojaf commented 11 months ago

Oh, I see. Your original report did not mention fsharp_multiline_bracket_style = stroustrup.

Stroustrup cannot be applied in absolutely every situation. I was convinced that

let x = { new System.IDisposable with
  member _.Dispose () = ()
}

was invalid code. (Producing an offset error). In certain situations it can lead to invalid code.

This does not seem the case. I am pretty sure we decided against this for object expressions. @josh-degraw does this ring a bell?

So, we could double-check whether is indeed possible for object expressions. If so, someone adds support for it. If not, we document that stroustrup cannot be used absolutely everywhere.

josh-degraw commented 11 months ago

Yeah that's sounds correct to me as far as what we did. However, wouldn't something like this be valid if we wanted to more strictly apply Stroustrup here?

let x = { 
    new System.IDisposable with
        member _.Dispose () = ()
}

This would be how I would expect Stroustrup style to be applied here.

nojaf commented 11 months ago

If it always leads to valid code, I'd be ok with this. It really rings a bell that this wasn't the case. Can't immediately find any prior art though.

nojaf commented 11 months ago

It is mentioned in the style guide. Guess this is possible.

MangelMaxime commented 11 months ago

Oh, I see. Your original report did not mention fsharp_multiline_bracket_style = stroustrup.

Ah yes sorry, I have a hard time finding which settings I have setup locally and not in the online tool.