Coder Social home page Coder Social logo

Comments (13)

davidmdm avatar davidmdm commented on June 16, 2024 7

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")
  }
}

from gofumpt.

davidmdm avatar davidmdm commented on June 16, 2024 3

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

from gofumpt.

flan6 avatar flan6 commented on June 16, 2024 2

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.

from gofumpt.

LuigiAzevedo avatar LuigiAzevedo commented on June 16, 2024 2

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.

from gofumpt.

seanhagen avatar seanhagen commented on June 16, 2024 2

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.

from gofumpt.

flan6 avatar flan6 commented on June 16, 2024 2

@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)
                }

        }
}

from gofumpt.

mvdan avatar mvdan commented on June 16, 2024 1

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.

from gofumpt.

mvdan avatar mvdan commented on June 16, 2024 1

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.

from gofumpt.

flan6 avatar flan6 commented on June 16, 2024

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()
}

from gofumpt.

flan6 avatar flan6 commented on June 16, 2024

I totally agree @davidmdm

from gofumpt.

cristaloleg avatar cristaloleg commented on June 16, 2024

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

from gofumpt.

davidmdm avatar davidmdm commented on June 16, 2024

@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.

from gofumpt.

cristaloleg avatar cristaloleg commented on June 16, 2024

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.

from gofumpt.

Related Issues (20)

Recommend Projects

  • React photo React

    A declarative, efficient, and flexible JavaScript library for building user interfaces.

  • Vue.js photo Vue.js

    🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.

  • Typescript photo Typescript

    TypeScript is a superset of JavaScript that compiles to clean JavaScript output.

  • TensorFlow photo TensorFlow

    An Open Source Machine Learning Framework for Everyone

  • Django photo Django

    The Web framework for perfectionists with deadlines.

  • D3 photo D3

    Bring data to life with SVG, Canvas and HTML. 📊📈🎉

Recommend Topics

  • javascript

    JavaScript (JS) is a lightweight interpreted programming language with first-class functions.

  • web

    Some thing interesting about web. New door for the world.

  • server

    A server is a program made to process requests and deliver data to clients.

  • Machine learning

    Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.

  • Game

    Some thing interesting about game, make everyone happy.

Recommend Org

  • Facebook photo Facebook

    We are working to build community through open source technology. NB: members must have two-factor auth.

  • Microsoft photo Microsoft

    Open source projects and samples from Microsoft.

  • Google photo Google

    Google ❤️ Open Source for everyone.

  • D3 photo D3

    Data-Driven Documents codes.