G-Research / fsharp-formatting-conventions

G-Research F# code formatting guidelines
Apache License 2.0
19 stars 6 forks source link

fsharp_newline_between_type_definition_and_members inconsistency #42

Open nojaf opened 11 months ago

nojaf commented 11 months ago

Hi @Smaug123,

@auduchinok and I were talking about fsharp_newline_between_type_definition_and_members yesterday and we may have found a questionable case.

The setting is on by default in Fantomas:

type Foo with
    member x.Bar () = ()

type Foo2=
    member x.Bar2 () = ()

type Foo3 =
    abstract Bar3: unit -> unit

leads to

type Foo with

    member x.Bar() = ()

type Foo2 =
    member x.Bar2() = ()

type Foo3 =
    abstract Bar3: unit -> unit

Does the new line in type Foo with make sense? Is it desired by the teams? My gut feeling is that we never discussed this case in great detail. And the result we have today just happens to be how the implementation works.

Eugene advocated for having it only when there's a representation or non-member bindings/fields in the default code style.

type A =
    let a = 1

    member this.P = 1

type A =
    val field: int // not sure whether it's correct syntax

    member this.P = 1

type A =
    do ()

    member this.P = 1

type A =
    inherit T()

    member this.P = 1

type A =
    | Case

    member this.P = 1

type B =
    member this.P = 1

type B() =
    member this.P = 1

type B with
    member this.P = 1

I think this sounds reasonable, would be interested to know what the GR folks think about it.

pleaseletmesearch commented 11 months ago

(This is @Smaug123 commenting from my GR-internal GitHub account.)

I think the final snippet looks reasonable; the type Foo with\n\nmember x.Bar() = () does look a bit ugly. Omitting the first newline looks right to me. I'll just send this round in case anyone disagrees.