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

[Feature request] Configuration to disable blank lines around multiline code constructs #1370

Closed
3 of 6 tasks
isral opened this issue Jan 17, 2021 · 10 comments
Closed
3 of 6 tasks

Comments

@isral
Copy link

isral commented Jan 17, 2021

I request the ability to disable blank lines around multiline code constructs
(https://github.com/fsprojects/fantomas/blob/master/docs/FormattingConventions.md#2020-revision)

I can not find it in https://github.com/fsprojects/fantomas/blob/master/docs/Documentation.md#configuration

Related issue: #1119

Affidavit (please submit!)

Please tick this by placing a cross in the box:

Please tick all that apply:

  • This is not a breaking change to Fantomas
  • I or my company would be willing to help implement and/or test this
  • This suggestion is part of the Microsoft style guide (please add a link to section if so)
  • This suggestion is part of the G-Research style guide (please add a link to section if so)
@nojaf
Copy link
Contributor

nojaf commented Jan 19, 2021

Hello @isral, thank you for your suggestion.
I feel like this topic is broader than the limited information you provided.
For example:

module MyMod

open System
open System.IO

type Person = 
    { Age: int
      Name: string }

let someFunction () =
    // some implementation
    ()

In this case, the open statements, the type, and the let binding are multiline constructs.
I believe you do want blank lines here.
So, I'm guessing you mostly have an issue with the newlines inside inner let bindings.

Next to that, I have also been playing around with the idea to have more control in Fantomas where blank lines occur.
If people can configure it through settings, the existing newlines (found via trivia) could be left out.

On top of my head, you want control over the top-level constructs, inner constructs, and perhaps the number of blank lines after a module.
I need to give this some more thoughts but it could be worth exploring for the next major version.

@isral
Copy link
Author

isral commented Jan 19, 2021

So, I'm guessing you mostly have an issue with the newlines inside inner let bindings.

Yes, that's why I put link to 2020-revision and issue #1119.
I love the compactness of F#, so don't I want blank lines.

let emit nodes =
    let an = AssemblyName("a")
// I don't want this blank line
    let ab =
        AppDomain.CurrentDomain.DefineDynamicAssembly(
            an,
            AssemblyBuilderAccess.RunAndCollect
        )
// I don't want this blank line
    let mb = ab.DefineDynamicModule("a")
    let tb = mb.DefineType("Program")
let emit (il: ILGenerator) method opcodes =
    let labels: Label array = Array.zeroCreate !method.labels
    let locals = Dictionary<string, LocalBuilder>()
// I don't want this blank line
    for i in 0 .. !method.labels - 1 do
        labels.[i] <- il.DefineLabel()
// I don't want this blank line
    for i in method.locals do
        locals.[i.Key] <- il.DeclareLocal(i.Value)

@nojaf
Copy link
Contributor

nojaf commented Jan 20, 2021

Oh, I see the 2020 revision text doesn't really go into detail. The point being, that it applies to both top-level and inner constructs.
Out of curiosity, do you sometimes break this rule? Are there situations where you do put a blank line?

@isral
Copy link
Author

isral commented Jan 20, 2021

Out of curiosity, do you sometimes break this rule? Are there situations where you do put a blank line?

I still new in F# so I don't know.

But in C#:

  • I maybe leave blank lines as is if the code is copied from other source
  • For my own code, sometimes I put // in the blank line so it's not really blank.
	DefaultConstraint = 9064,

	PrimaryConstructorBaseType = 9065,

	FunctionPointerUnmanagedCallingConventionList = 9066,
	FunctionPointerUnmanagedCallingConvention = 9067,
	//
	Compilation, BinaryExpression, PrefixExpression

2 last lines are mine, notice empty comment.
Other lines are copied from Roslyn (I leave the blank lines as is).

@zyzhu
Copy link

zyzhu commented Jan 25, 2021

I just started to try Fantomas. I find blank lines around multiline code is very disruptive to compact F# coding style. An option to disable it would be ideal.

Take the following snippet from DiffSharp to make a case.
https://github.com/DiffSharp/DiffSharp/blob/dev/src/DiffSharp.Core/Data.fs#L53

The default Fantomas settings will format it with too many empty lines, which breaks the flow of the logics.

Before formatting

type MNIST(path:string, ?urls:seq<string>, ?train:bool, ?transform:Tensor->Tensor, ?targetTransform:Tensor->Tensor) =
    inherit Dataset()
    let path = Path.Combine(path, "mnist") |> Path.GetFullPath
    let train = defaultArg train true
    let transform = defaultArg transform (fun t -> (t - 0.1307) / 0.3081)
    let targetTransform = defaultArg targetTransform id
    let urls = List.ofSeq <| defaultArg urls (Seq.ofList
                   ["http://yann.lecun.com/exdb/mnist/train-images-idx3-ubyte.gz";
                    "http://yann.lecun.com/exdb/mnist/train-labels-idx1-ubyte.gz";
                    "http://yann.lecun.com/exdb/mnist/t10k-images-idx3-ubyte.gz";
                    "http://yann.lecun.com/exdb/mnist/t10k-labels-idx1-ubyte.gz"])
    let files = [for url in urls do Path.Combine(path, Path.GetFileName(url))]
    let filesProcessed = [for file in files do Path.ChangeExtension(file, ".tensor")]
    let data, target = 
        Directory.CreateDirectory(path) |> ignore
        let mutable data = dsharp.zero()
        let mutable target = dsharp.zero()
        if train then
            if not (File.Exists(files.[0])) then download urls.[0] files.[0]
            if not (File.Exists(files.[1])) then download urls.[1] files.[1]
            if File.Exists(filesProcessed.[0]) then data <-    dsharp.load(filesProcessed.[0]) else data <-    MNIST.LoadMNISTImages(files.[0]); dsharp.save(data, filesProcessed.[0])
            if File.Exists(filesProcessed.[1]) then target <- dsharp.load(filesProcessed.[1]) else target <- MNIST.LoadMNISTLabels(files.[1]); dsharp.save(target, filesProcessed.[1])
        else
            if not (File.Exists(files.[2])) then download urls.[2] files.[2]
            if not (File.Exists(files.[3])) then download urls.[3] files.[3]
            if File.Exists(filesProcessed.[2]) then data <-    dsharp.load(filesProcessed.[2]) else data <-    MNIST.LoadMNISTImages(files.[2]); dsharp.save(data, filesProcessed.[2])
            if File.Exists(filesProcessed.[3]) then target <- dsharp.load(filesProcessed.[3]) else target <- MNIST.LoadMNISTLabels(files.[3]); dsharp.save(target, filesProcessed.[3])
        data, target

After formatting

type MNIST
  (
    path: string,
    ?urls: seq<string>,
    ?train: bool,
    ?transform: Tensor -> Tensor,
    ?targetTransform: Tensor -> Tensor
  ) =
  inherit Dataset()

  let path =
    Path.Combine(path, "mnist") |> Path.GetFullPath

  let train = defaultArg train true

  let transform =
    defaultArg transform (fun t -> (t - 0.1307) / 0.3081)

  let targetTransform = defaultArg targetTransform id

  let urls =
    List.ofSeq
    <| defaultArg
         urls
         (Seq.ofList [ "http://yann.lecun.com/exdb/mnist/train-images-idx3-ubyte.gz"
                       "http://yann.lecun.com/exdb/mnist/train-labels-idx1-ubyte.gz"
                       "http://yann.lecun.com/exdb/mnist/t10k-images-idx3-ubyte.gz"
                       "http://yann.lecun.com/exdb/mnist/t10k-labels-idx1-ubyte.gz" ])

  let files =
    [ for url in urls do
        Path.Combine(path, Path.GetFileName(url)) ]

  let filesProcessed =
    [ for file in files do
        Path.ChangeExtension(file, ".tensor") ]

  let data, target =
    Directory.CreateDirectory(path) |> ignore
    let mutable data = dsharp.zero ()
    let mutable target = dsharp.zero ()

    if train then
      if not (File.Exists(files.[0])) then
        download urls.[0] files.[0]

      if not (File.Exists(files.[1])) then
        download urls.[1] files.[1]

      if File.Exists(filesProcessed.[0]) then
        data <- dsharp.load (filesProcessed.[0])
      else
        data <- MNIST.LoadMNISTImages(files.[0])
        dsharp.save (data, filesProcessed.[0])

      if File.Exists(filesProcessed.[1]) then
        target <- dsharp.load (filesProcessed.[1])
      else
        target <- MNIST.LoadMNISTLabels(files.[1])
        dsharp.save (target, filesProcessed.[1])
    else
      if not (File.Exists(files.[2])) then
        download urls.[2] files.[2]

      if not (File.Exists(files.[3])) then
        download urls.[3] files.[3]

      if File.Exists(filesProcessed.[2]) then
        data <- dsharp.load (filesProcessed.[2])
      else
        data <- MNIST.LoadMNISTImages(files.[2])
        dsharp.save (data, filesProcessed.[2])

      if File.Exists(filesProcessed.[3]) then
        target <- dsharp.load (filesProcessed.[3])
      else
        target <- MNIST.LoadMNISTLabels(files.[3])
        dsharp.save (target, filesProcessed.[3])

    data, target

@zyzhu
Copy link

zyzhu commented Jan 25, 2021

Oh, I see the 2020 revision text doesn't really go into detail. The point being, that it applies to both top-level and inner constructs.
Out of curiosity, do you sometimes break this rule? Are there situations where you do put a blank line?

I think the blank lines at the top level makes perfect sense. But it is disruptive for inner constructs, at least to me.

@nojaf
Copy link
Contributor

nojaf commented Apr 5, 2021

Hello @zyzhu, @isral and @cxa.
I've created a new setting called fsharp_blank_lines_around_nested_multiline_expressions.
Please give try this out using 4.5.0-alpha-006.

As stated in the docs, this only works for nested expressions and existing newlines are still being preserved.

@nojaf
Copy link
Contributor

nojaf commented Apr 5, 2021

Sample link.

The setting is true by default so you want to put

[*.fs]
fsharp_blank_lines_around_nested_multiline_expressions=false

in your .editorconfig.

@zyzhu
Copy link

zyzhu commented Apr 5, 2021

It's perfect! Finally I applied consistent formatting using Fantomas. Thank you so much.

@cxa
Copy link
Contributor

cxa commented Apr 6, 2021

Nice! Works like a charm, much appreciated.

@nojaf nojaf closed this as completed Apr 30, 2021
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

4 participants