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

Remove empty lines in if and for similar to functions #284

Open
flan6 opened this issue Aug 22, 2023 · 13 comments · May be fixed by #311
Open

Remove empty lines in if and for similar to functions #284

flan6 opened this issue Aug 22, 2023 · 13 comments · May be fixed by #311

Comments

@flan6
Copy link

flan6 commented Aug 22, 2023

// code before formating
func main() {

  for i := 0; i < 5; i++ {

    err := doSomething()

    if  err != nil {

      fmt.Println(err)

    } 
    
    doAnotherThingVoid()
    
  }
  
}

would become

// code after formating
func main() {
  for i := 0; i < 5; i++ {

    err := doSomething()
    if  err != nil {

      fmt.Println(err)

    }
    
    doAnotherThingVoid()

  }
}

my proposal is to remove same empty lines in if and for statement as well

// my proposal
func main() {
  for i := 0; i < 5; i++ {
    err := doSomething()
    if  err != nil {
      fmt.Println(err)
    }
    
    doAnotherThingVoid()
  }
}

note that the empty lines between different instructions in same level doesn't get removed. Same way as empty lines between function declarations.
this behavior is more consistent and better to read.

@mvdan
Copy link
Owner

mvdan commented Aug 23, 2023

We already have the following rules:

No empty lines around function bodies
No empty lines around a lone statement (or comment) in a block
No empty lines before a simple error check

What extra rules would you add, exactly? I personally think that anything more than this would make gofumpt too aggressive. Empty lines can be good to logically separate bits of code.

@flan6
Copy link
Author

flan6 commented Aug 24, 2023

First, thank you for this swift response!

I'm proposing to add if and for statements to the function rule. It makes code more regular about empty lines. I do agree empty lines are good to separate bits of code. My point is exactly about lines after opening curly bracers { and before closing ones }.

No empty lines around function, for and if bodies

This addition would produce the following effects (comments where to remove empty lines):

func main() {
  somethingSomething()

  for i := 0; i < 5; i++ {
// REMOVE: No empty lines around *for* bodies
    err := doSomething()
    if  err != nil {
// REMOVE: No empty lines around *if* bodies. 
      go reportError()
      
      fmt.Println(err)
// REMOVE: No empty lines around *if* bodies. 
    }

    go codeKeepsGoingOn()

    doAnotherThingVoid()
// REMOVE: No empty lines around *for* bodies
  }
  
  somethingSomething()
}

this behavior would produce the following final code:

// my proposal
func main() {
  somethingSomething()

  for i := 0; i < 5; i++ {
    err := doSomething()
    if  err != nil {
      go reportError()
    
      fmt.Println(err)
    }

    go codeKeepsGoingOn()

    doAnotherThingVoid()
  }
  
  somethingSomething()
}

@flan6
Copy link
Author

flan6 commented Aug 24, 2023

Currently gofmt already removes empty lines around struct and interface declarations. So I guess we can read it as:

No empty lines around struct declarations
No empty lines around interface declarations

type A interface {
// This line is removed
  Something() bool
  
  Sometime() time.Time
// This line is removed
}

type b struct {
// This line is removed
  id int

  name string
// This line is removed
}

And gofumpt currently removes around functions. So following this pattern we would assume any empty line after an opening curly bracer { and before a closing } would be removed. But right now if statements and for statements are left untouched.

@LuigiAzevedo
Copy link

I support the proposed change to remove blank spaces after open brackets and before closing brackets. In my opinion, this change would not degrade the code readability and would make it more concise with fewer inconsistencies. I appreciate the effort put into improving the code formatter behavior and believe that this change would be a positive step forward.

@seanhagen
Copy link

If you're looking for another reason to support this change, the whitespace linter complains about these kinds of newlines; having gofumpt remove them would help auto-fix those complaints.

@davidmdm
Copy link

I also support this proposal.

I don't mind defining it as two new rules:

  • No empty lines around for bodies
  • No empty lines around if bodies

However, I think if we want to be succinct, the rule that we actually want is no leading or trailing empty lines for code scopes

This would apply to scopes created by if, for, func, and just to inline scopes in general.

func Something() {


  {

    fmt.Println("hello")

  }



  {
    
    fmt.Println("world")
  
  }

}

becomes:

func Something() {
  {
     fmt.Println("hello")
  }

  {
    fmt.Println("world")
  }
}

@flan6
Copy link
Author

flan6 commented Sep 29, 2023

I totally agree @davidmdm

@cristaloleg
Copy link

Isn't this a regression? or I'm just wrong?

@davidmdm
Copy link

davidmdm commented Oct 3, 2023

@cristaloleg please expand?

It can be viewed as two new rules or an expansion on an existing rule.

It would not perform invalid formatting as per gofmt.

@cristaloleg
Copy link

please expand?

I thought that gofumpt was removing those lines already.

    if  err != nil {

        fmt.Println(err)

    } 

was formatted to

    if  err != nil {
        fmt.Println(err)
    } 

I might be wrong or something else changed in my local setup.

@flan6
Copy link
Author

flan6 commented Oct 10, 2023

@cristaloleg I used a very simple example just to show what I intended. I tested if and yes, if you have only one instruction inside the if statement it indeed removes. But if you have a more complex block of code it does not.

flan on ArchLinux in ~/tmp
$ nvim main.go
flan on ArchLinux in ~/tmp
$ cat main.go
package main

import "fmt"

func main() {
        print := true

        if print {

                fmt.Println("remove lines")

                fmt.Println("please format my code")

                fmt.Println("some instructions")

        }

        {

                someSlice := []int{1, 2, 3}

                for i := range someSlice {
                        fmt.Println("this slice is fun times", i)
                }

        }
}
flan on ArchLinux in ~/tmp
$ gofumpt --version
v0.5.0 (go1.21.0)
flan on ArchLinux in ~/tmp
$ gofumpt -w main.go
flan on ArchLinux in ~/tmp
$ cat main.go
package main

import "fmt"

func main() {
        print := true

        if print {

                fmt.Println("remove lines")

                fmt.Println("please format my code")

                fmt.Println("some instructions")

        }

        {

                someSlice := []int{1, 2, 3}

                for i := range someSlice {
                        fmt.Println("this slice is fun times", i)
                }

        }
}

@davidmdm
Copy link

@mvdan any input? I created a PR that seems to accomplish the goal.

@mvdan
Copy link
Owner

mvdan commented Jan 28, 2024

I took a brief look at this. In some cases I agree this removes empty lines which aren't necessary, but in other cases it removes empty lines which I find useful to logically separate different chunks of code. For example, in a project of mine:

diff --git a/syntax/printer.go b/syntax/printer.go
index 7c3ff9e6..ba1e6c29 100644
--- a/syntax/printer.go
+++ b/syntax/printer.go
@@ -62,7 +62,6 @@ func KeepPadding(enabled bool) PrinterOption {
 			p.keepPadding = true
 			p.cols.Writer = p.bufWriter.(*bufio.Writer)
 			p.bufWriter = &p.cols
-
 		} else if !enabled && p.keepPadding {
 			// Ensure we reset the state to that of NewPrinter.
 			p.keepPadding = false
@@ -1496,7 +1495,6 @@ func (e *extraIndenter) WriteByte(b byte) error {
 		e.firstIndent = lineIndent
 		e.firstChange = e.baseIndent - lineIndent
 		lineIndent = e.baseIndent
-
 	} else if lineIndent < e.firstIndent {
 		// This line did not have enough indentation; simply indent it
 		// like the first line.
diff --git a/syntax/typedjson/json_test.go b/syntax/typedjson/json_test.go
index 5292fa7c..53b1c773 100644
--- a/syntax/typedjson/json_test.go
+++ b/syntax/typedjson/json_test.go
@@ -26,7 +26,6 @@ func TestRoundtrip(t *testing.T) {
 	shellPaths, err := filepath.Glob(filepath.Join(dir, "*.sh"))
 	qt.Assert(t, err, qt.IsNil)
 	for _, shellPath := range shellPaths {
-
 		shellPath := shellPath // do not reuse the range var
 		name := strings.TrimSuffix(filepath.Base(shellPath), ".sh")
 		jsonPath := filepath.Join(dir, name+".json")

The first two don't seem right to me - I want to split the "else if", much like I might do with different cases in a switch. You might prefer to not use that empty line yourself, but I don't think that falls under "an empty line there is definitely pointless and gofumpt should remove it".

I'd be fine with strenghtening this rule as long as it leaves empty lines at the end of blocks followed by an else-if. All other removed lines I can see across my projects seem fine to me. Happy to review a PR with the above tweak as well as tests for all the various edge cases; note that #286 has no tests and so it isn't enough on its own. Look at testdata/script/block-single.txtar, for example.

The README would also need to be tweaked, but I can do that after the change is merged.

flan6 added a commit to flan6/gofumpt that referenced this issue Aug 23, 2024
ensures that format removes empty lines at the top of a block unconditionally and bottom unless it is followed by an else-if statement.
see issue mvdan#284
flan6 added a commit to flan6/gofumpt that referenced this issue Aug 23, 2024
ensures that empty line before an else-if statement is not removed.
see mvdan#284.
flan6 added a commit to flan6/gofumpt that referenced this issue Aug 23, 2024
see mvdan#284.
trailing empty lines are removed only if it is not followed by an
else-if statement.
@flan6 flan6 linked a pull request Aug 23, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants