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

Newline inconsistency in object expression #2990

Open
4 tasks
MangelMaxime opened this issue Nov 17, 2023 · 7 comments
Open
4 tasks

Newline inconsistency in object expression #2990

MangelMaxime opened this issue Nov 17, 2023 · 7 comments

Comments

@MangelMaxime
Copy link

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

  • The formatted result breaks my code.
  • The formatted result gives compiler warnings.
  • I or my company would be willing to help fix this.
  • I would like a release if this problem is solved.

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

nojaf commented Nov 18, 2023

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.

@nojaf nojaf closed this as not planned Won't fix, can't repro, duplicate, stale Nov 18, 2023
@MangelMaxime
Copy link
Author

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

nojaf commented Nov 18, 2023

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

josh-degraw commented Nov 18, 2023

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

nojaf commented Nov 18, 2023

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

nojaf commented Nov 18, 2023

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

@MangelMaxime
Copy link
Author

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.

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

No branches or pull requests

3 participants