mvdan / gofumpt Goto Github PK

One thing that I often find in style guides is the desire to have lists of objects in order to help with readability.

Would be great if we could add a feature to order strings in slices when they are defined.

eg.

names := []string{
  "bob",
  "claire",
  "alice",
}

goes to:

names := []string{
  "alice",
  "bob",
  "claire",
}

gofumpt re-writes documented one-line top level const and var blocks to a single line. This is not correct if the block in documented. Consider:

// Some foos.
const (
    Foo = 1
)

gofumpt re-writes this to:

// Some foos.

const Foo = 1.

This detaches the comment from the declaration, causing golint to complain that Foo is not documented, whereas it was documented before gofumpt reformatted it.

As the comment cannot be written, I think the only option is to not reformat such blocks.

This might be a bit opinionated, but here it goes anyway. I'm wondering whether it would make sense to remove empty lines after closing braces, if that is followed by further code. E.g.

for i:=0; i<10; i++ {
    fmt.Println(i)
}

// This part will so something something
if err := doSomething(); err != nil {
    return err
}

if err := doSomethingElse(); err != nil {
    return err
}

// Everything done, dunno what next

vs.

for i:=0; i<10; i++ {
    fmt.Println(i)
}
// This part will so something something
if err := doSomething(); err != nil {
    return err
}
if err := doSomethingElse(); err != nil {
    return err
}
// Everything done, dunno what next

IMHO, the first code falls apart. The closing brace line } (it's essentially an 99% empty line) already acts as a visual separator, so adding an extra empty line only further emphasizes that something else is happening. But this emphasis is imho not needed:

• If the following code is a logical continuation, the extra empty line just breaks the visual flow.
• If the following code is a completely different thing that warrants visual emphasis, a comment can help create both more readable code and also add the extra visual break to highlight it.

Now, the reason I'm asking for this to be considered in this tool is because this is one of those things that as a maintainer I'd like to enforce, but it's both painful and annoying for everyone to manually write this down on every PR at 25 different locations explaining why this is good. An automated tool could both clean up the repo automatically and also enforce in CI.

Caveat: This may just be personal preference, so feel free to close if you think it is too controversial a change, but it does comply with the "preserve gofmt compatibility" so maybe it is OK...

--

I'd like gofumpt to either reformat all single-line struct initialisations into multi-line per field initialisation – or if that is too big a change, I'd like it to reformat them if some threshold of number of fields being initialised is exceeded. Even better if it were to include the functionality of keyify to convert any unkeyed fields to keyed ones.

i.e.,

diff --git a/a/main.go b/b/main.go
index 3ddfc0e..46cccae 100644
--- a/a/main.go
+++ b/b/main.go
@@ -10,19 +10,33 @@ type person struct {
 
 func main() {
 	// This syntax creates a new struct.
-	fmt.Println(person{"Bob", 20})
+	fmt.Println(person{
+		name: "Bob",
+		age:  20,
+	})
 
 	// You can name the fields when initializing a struct.
-	fmt.Println(person{name: "Alice", age: 30})
+	fmt.Println(person{
+		name: "Alice",
+		age:  30,
+	})
 
 	// Omitted fields will be zero-valued.
-	fmt.Println(person{name: "Fred"})
+	fmt.Println(person{
+		name: "Fred",
+	})
 
 	// An `&` prefix yields a pointer to the struct.
-	fmt.Println(&person{name: "Ann", age: 40})
+	fmt.Println(&person{
+		name: "Ann",
+		age:  40,
+	})
 
 	// Access struct fields with a dot.
-	s := person{name: "Sean", age: 50}
+	s := person{
+		name: "Sean",
+		age:  50,
+	}
 	fmt.Println(s.name)
 
 	// You can also use dots with struct pointers - the

I don't recall a Go README ever suggesting mktemp or 'go mod init'. Why?

The following import group

import (
	_ "expvar" // metrics
	"net/http"
	"net/http/pprof"

	"github.com/gorilla/mux"
)

is now being reordered as

import (
	"net/http"
	"net/http/pprof"

         _ "expvar" // metrics

	"github.com/gorilla/mux"
)

breaking our linter

./pkg/metrics/metrics.go: Import groups are not in the proper order: ["Std" "Std" "Third party"]

Just a quick note about an awkward edge case. gofumpt replaces var _ = ... with _ := ..., which errors with no new variables on left side of :=.

I think I found a bug :-)

Before formatting:

const Hello1 = "h1"
const Hello2 = "h2" // h2
const Hello3 = "h3" // h3

After formatting:

const (
	Hello1 = "h1"
	Hello2 = "h2" // h2
	Hello3 = "h3"
) // h3

Expected formatting:

const (
	Hello1 = "h1"
	Hello2 = "h2" // h2
	Hello3 = "h3" // h3
)

This applies also to var, type and ... ?

https://downforeveryoneorjustme.com/mvdan.cc reports that mvdan.cc is down for everyone, not just me.

I run go get mvdan.cc/gofumpt as part of my CI so I can check that everything is formatted correctly. mvdan.cc being down breaks my CI.

Any idea when mvdan.cc will be back up?

Refs #9.

This way, if one wants to shadow the system's gofmt with this one, they could just do:

go get mvdan.cc/gofumpt/gofmt

Perhaps do something similar with goimports? @josharian brought up that lots of people use goimports on save, so it could be difficult or slow to have that plus gofumpt.

Hey there, Thank you for you work on this more stricter tool!

accounts is a global variable.

Running gofumpt -w -s .

Have:

var accounts map[string][]string = map[string][]string{
	"000099999": {"WARENVERKAUF", RECORD_TYPE_TURNOVER},
}

Want:

var accounts = map[string][]string{
	"000099999": {"WARENVERKAUF", RECORD_TYPE_TURNOVER},
}

Is there an interest in implementing breaking long lines into multiple lines?

For example, if there is this line in code

	if err := chromedp.Run(ctx, tasks.ExtractAccountDataFromDropdown(&accountTypeStr, &name, &accountNo, &currency, &balance, idx)); err != nil {

which is 146 chars long (with the first tab), break it into

	if err := chromedp.Run(
		ctx,
		tasks.ExtractAccountDataFromDropdown(&accountTypeStr, &name, &accountNo, &currency, &balance, idx),
	); err != nil {

or even into

	if err := chromedp.Run(
		ctx,
		tasks.ExtractAccountDataFromDropdown(
			&accountTypeStr,
			&name,
			&accountNo,
			&currency,
			&balance,
			idx,
		),
	); err != nil {

I might look into that as an exercise, but I am not sure if there is interest in it :)

the same logic with long function definitions, of course

That is, forbid:

func foo() {
     // body
}
func bar() {
     // body
}

I think this should only be allowed if the global declarations are single-line.

Input

package main


import (
        "os" // comment to OS

        "fmt"
)

Expected: Either leave input as-is, or

package main


import (
        "fmt"
        "os" // comment to OS
)

Actual output (with #57; without it, fmt and os are switched, but comment still below):

package main

import (
        "fmt"
        "os"
        // comment to OS
)

I hit this issue when trying testcases for #55 (without the option)

foo, err := doFoo()

if err != nil {
    return nil, fmt.Errorf("could not do foo: %v", err)
}

That empty line should go.

Exceptions:

• When the check is more complex than err != nil
• When there are comments directly before the error check
• When the if statement has an else

This is real edgecase from our repo

// BankCredential is (...)
//require (...) seperated by the
//'type' field.

This is not spaced, because gofumpt still thinks it's code (because the third line starts with ').

I have a simple program:

package main

import "fmt"

// All the things that can go wrong.
// TODO: add more
const (
	InvalidID = "invalid ID number given"
)

// Message IDs that map to beautiful, long-winded status messages.
// TODO: add more
const (
	WelcomeBack = iota // Welcome the user back after a long time being away.
)

func main() {
	fmt.Println("Hello, world!")
}

One of my linters will complain if I have no comments on exported consts or their containing const block, so I put comments on them. However, when I have only a single const that gets coalesced to const foo = "bar", the per-block comment doesn't really apply to a block anymore:

diff --git a/main.go b/main.go
index 283ba85..1e04736 100644
--- a/main.go
+++ b/main.go
@@ -4,15 +4,13 @@ import "fmt"
 
 // All the things that can go wrong.
 // TODO: add more
-const (
-	InvalidID = "invalid ID number given"
-)
+
+const InvalidID = "invalid ID number given"
 
 // Message IDs that map to beautiful, long-winded status messages.
 // TODO: add more
-const (
-	WelcomeBack = iota // Welcome the user back after a long time being away.
-)
+
+const WelcomeBack = iota // Welcome the user back after a long time being away.
 
 func main() {
 	fmt.Println("Hello, world!")

This leaves me with a floating comment per former block that no longer obviously applies to anything in particular:

package main

import "fmt"

// All the things that can go wrong.
// TODO: add more

const InvalidID = "invalid ID number given"

// Message IDs that map to beautiful, long-winded status messages.
// TODO: add more

const WelcomeBack = iota // Welcome the user back after a long time being away.

func main() {
	fmt.Println("Hello, world!")
}

I don't have much of a problem assigning to _ to temporarily silence unused-variable errors for fifteen minutes while I debug something. However, I'd resent having to preemptively add a dummy unexported constant to keep my doc comments from becoming orphaned by a block-eliminating change.

That is, rewrite var name = value into name := value. The same would happen for var n1, n2 = v1, v2, which would become n1, n2 := v1, v2.

Exceptions:

• Globals, because name := value is only valid as a statement
• var name Type = value and var name Type

The only edge case I see is var name = zerovalue, like var s = "". Rewriting that to s := "" is fine, but some people might prefer var s string. Then again, if people prefer that form, they can use it directly and the tool won't change it.

See golang/go#34582.

For example:

var _ = []struct{ a int }{
	{
		a: 1,
	},
	{
		a: 2,
	}, { // inconsistent
		a: 3,
	},
}

A bit bizare bug.

I have this file (with tabs)

package main

import (
        "os"

        "io"
)

When I run gofumpt -w foo.go, I get

package main

import (
        "os"
        "io"
)

and when I run for a second time, I get

package main

import (
        "io"
        "os"
)

The tests did not catch this, since in the testscripts, there is always

gofumpt -w foo.go .

which actually visits the file twice, once as a foo.go and a second time when traversing the directory ..

A simple hacky fix is to format each file twice.

An empty line between the switch statement start and the first case statement is similar the blank line after function/method declaration, not useful. An empty line before the switch can separate the whole block if needed, but after separates the code block awkwardly.

Standardizing on the format:

switch {
case x:

makes for a more coherent code block.

err := foo()
if err != nil {
    return err
}

Since both lines are short, I'd personally write this like:

if err := foo(); err != nil {
    return err
}

Advantages:

• Shorter and simpler
• We don't pollute the parent scope with err
• Impossible to use the wrong err value later by mistake

However, it's not clear what we should do if the code uses err = foo(), or if changing the code would break the parent scope as err is no longer declared. For example:

err := foo() // merging the lines would stop declaring err
if err != nil { ... }
b, err = bar()
if err != nil { ... }

I'd love for gofumpt to be able to do this, as I see code like this too often. However, it's going to be hard to do it correctly, as the tool doesn't have any type information.

And even if it did, one could argue that any rule needing type information isn't a formatting rule. Perhaps it fits better into a "code simplification" tool or some other static analysis tool.

Filing this issue to get more opinions.

Currently, gofumpt adds a space after // and that causes staticcheck to not recognize the directive.

A convention I and coworkers often follow is a further separation of imports by relationship to the current project. So anything from the app/library itself is grouped separately so you can easily see all the external dependencies without them getting mixed with internal dependencies.

import (
	"bytes"
	"encoding/json"
	"fmt"
	"io"
	"net/http"

	"github.com/go-chi/chi"
	"github.com/pkg/errors"
	"github.com/lib/pq"

	"github.com/our-company-org/our-project/stuff"
	"github.com/our-company-org/our-project/internal/other-stuff"
	"github.com/our-company-org/our-project/internal/secret/generics"
)

It would be neat for gofumpt to read the current package from go.mod — which in the example above, would be github.com/our-company-org/our-project — then group any package with that prefix at the bottom of the import block.

Maybe this is too specific though, I'm not sure if anyone else does this.

Some examples:

chan (int)
f((3))

We don't want to do this in all cases where a parentheses is unambiguous, though. For example, within binary expressions like (foo && bar) && baz it can help readability and maintainability, even if the parentheses don't actually change the evaluation.

Shouldn't rewrite comments in import block. Example:

test.go

package test
  
import (
        "crypto/hmac"
        // #nosec
        "crypto/md5"
        // #nosec
        "crypto/sha1"
        "encoding/base64"
)

Results in ($ gofumpt -s -w test.go):

package test

import (
	"crypto/hmac"
	"crypto/md5"
	"crypto/sha1"
	"encoding/base64"

	// #nosec

	// #nosec

)

The // #nosec comments shouldn't be moved around (need for gosec). This linter is conflicting with impi -- can't make them them both happy at the same time.

IMHO, it would be better if we can ensure new line creation after if parentheses, but this shouldn't be added if the if statement doesn't have continuity / codes.

No new line is added

func main() {
     if true {
        // logic
     }  
} // no new line

New line is added to separate if

func main() {
     if x == 5 {
        // logic
     }  
    // <---   Here new line is automatically added, to separate if logic
    x = 10
} // no new line

I hope it won't be too aggressive and can be switched off if the user doesn't want it.

Starting with aaa7156 , gofumpt output stopped "passing goimports" (meaning, goimports still does changes on it).

Small example

package main

import (
	status "google.golang.org/grpc/status"
	math "math"
)

func main() {
	status.Code(nil)
	math.Abs(0)
}

before aaa7156, the import part has result (which "passes" goimports)

import (
        math "math"

        status "google.golang.org/grpc/status"
)

after the commit, it stays the same; and goimports changes it to the code above; so gofumpt output does not "pass" goimports check.

It would be nice to be able to build gofumports out of the github repo, unless there's a particularly compelling reason to keep it on mvdan.cc

github.com/mvdan/gofumpt/gofumports$ go install
goimports.go:25:2: cannot find package "mvdan.cc/gofumpt/internal" in any of:
        /Users/awalker/opt/go/src/mvdan.cc/gofumpt/internal (from $GOROOT)
        /Users/awalker/opsgo/src/mvdan.cc/gofumpt/internal (from $GOPATH)

gofumpt is excellent, I'm using in my personal projects, and introducing it at work. I'd like to add CI checks to ensure that all code is formatted with gofumpt. This requires defining a version of gofumpt to use.

At the moment, there are no official releases of gofumpt, e.g. there are no tagged versions.

Would you consider a PR that adds a configuration for goreleaser? This would mean that when you push a tag to GitHub you get tarballs, packages, install scripts, etc. built automatically and uploaded to GitHub. goreleaser is working extremely well for me in my chezmoi project, and I'd be happy to pay @joemiller's contribution forward.

Hey Daniel,

We've got some protos, graphql stuff, json schema and moq generated files with the canonical generated header golang/go#13560 (comment) and I'd like to not apply gofumpt to those for CI purposes.

I could try with some find + grep but maybe you have something better under your hat?

Thanks!

Right now, we decide if one single comment line is definitely not plain English given its first character, and then decide not to add a space prefix to it.

If that's the case, the same should apply to its whole "paragraph", i.e. the consecutive comment lines (before or after) which aren't empty.

i.e.,

//This does that...

becomes

// This does that...

Howdie, love your work, and just stumbled across this which also looks awesome :)

I use prettier with JavaScript, and cargo fmt with Rust, and they both automatically (and consistently) break up long lines

Example func:

func somethingTooLong(ctx context.Context, longArgument string, anotherOne func(time.Duration) bool) (int, error) {
	// foo
}

func somethingTooLong(
	ctx context.Context,
	longArgument string,
	anotherOne func(time.Duration) bool
) (int, error) {
	// foo
}

gofmt doesn't do this, but it also doesn't undo it where I've done this manually, so at least this particular example is gofmt-compatible

My Makefile used to say do this:

 .PHONY: fmt
 fmt:
       go get golang.org/x/tools/cmd/goimports
       ./bin/goimports -w .
       go fmt ./...

I've now substituted that by:

 .PHONY: fmt
 fmt:
       go get mvdan.cc/gofumpt
       go get mvdan.cc/gofumpt/gofumports
       ./bin/gofumports -w .
       ./bin/gofumpt  ./...

When I run make fmt, I get this output:

$ make fmt
go get mvdan.cc/gofumpt
go: finding mvdan.cc/gofumpt latest
go get mvdan.cc/gofumpt/gofumports
go: finding mvdan.cc/gofumpt/gofumports latest
go: finding mvdan.cc/gofumpt latest
./bin/gofumports -w .
./bin/gofumpt  ./...
stat ./...: no such file or directory
Makefile:26: recipe for target 'fmt' failed
make: *** [fmt] Error 2

It appears however as if gofumpt did reformat quite a bunch of files nonetheless, but still it should not generate an error.

Hi, thanks for this and for your contributions to Golang.

I am trying this package with the hope of finding a solution to the imports order in go files. I have this:

Original code:

package grpcserver_test

import (
	"context"
	"testing"

	"github.com/xxx/xxx/pkg/grpcserver"


	"google.golang.org/grpc"
	"google.golang.org/grpc/codes"

	"google.golang.org/grpc/status"
)

And after running gofumpt and gofumports:

package grpcserver_test

import (
	"context"
	"testing"

	"github.com/xxx/xxx/pkg/grpcserver"

	"google.golang.org/grpc"
	"google.golang.org/grpc/codes"

	"google.golang.org/grpc/status"
)

"My" expected behaviour:

• std imports must be in a separate group at the top
• non std imports in a single separate group at the bottom

Example:

package grpcserver_test

import (
	"context"
	"testing"

	"github.com/xxx/xxx/pkg/grpcserver"
	"google.golang.org/grpc"
	"google.golang.org/grpc/codes"
	"google.golang.org/grpc/status"
)

This is something that also happens with the official goimports.

Did you had the intention to fix this as well? I mean, all non std imports in a single separate group at the bottom and not in different groups?

Thanks.

$ cat f.go
package p

func f() {
	var (
		i int
	)
}
$ cat f.go | gofumpt
package p

func f() {

	var i int

}
$ cat f.go | gofumpt | gofumpt
package p

func f() {
	var i int
}

One shouldn't require multiple runs of gofumpt to get the desired outcome. We should fix this edge case.

When I run gofumpt or gofumports over our code, the diffs are huge because it regroups all the imports. For instance:

@@ -7,11 +7,10 @@ import (
        "encoding/binary"
        "errors"
        "hash"
+       "liftoff/go/bitset"
        "math"

        "github.com/cespare/xxhash"
-
-       "liftoff/go/bitset"
 )

This happens with gofumpt and with gofumports executed with

gofumpt -w .
gofumports -local 'liftoff/' -w .

but not with gofmt or goimports executed with

gofmt -w .
goimports -local 'liftoff/' -w .

The following are equivalent:

func add(x int, y int) int

func add(x, y int) int

The second, however is shorter. Maybe gofumpt should rewrite the first to be the second?

gofumpt panics when grouping unindented standard library and third-party imports.

Tested on the following Go versions:

• go1.12.6
• go1.13-adcb2b1e7a

Steps to reproduce:

Run gofumpt with the following input:

import (
"a"
"a.b"
)

Expected output:

import (
	"a"

	"a.b"
)

Actual output:

panic: could not set lines to [0 19 23 23 29] [recovered]
	panic: could not set lines to [0 19 23 23 29]

goroutine 1 [running]:
golang.org/x/tools/go/ast/astutil.Apply.func1(0xc0000d40c0, 0xc0000bfc50)
	golang.org/x/[email protected]/go/ast/astutil/rewrite.go:47 +0xa3
panic(0x1152ae0, 0xc0000d40f0)
	runtime/panic.go:663 +0x1b2
mvdan.cc/gofumpt/internal.(*fumpter).addNewline(0xc0000ce160, 0x2e)
	mvdan.cc/gofumpt@/internal/gofumpt.go:123 +0x584
mvdan.cc/gofumpt/internal.(*fumpter).joinStdImports(0xc0000ce160, 0xc0000da080)
	mvdan.cc/gofumpt@/internal/gofumpt.go:409 +0x1fc
mvdan.cc/gofumpt/internal.(*fumpter).applyPre(0xc0000ce160, 0xc000082130)
	mvdan.cc/gofumpt@/internal/gofumpt.go:241 +0x98d
mvdan.cc/gofumpt/internal.Gofumpt.func1(0xc000082130, 0xc0000d01a0)
	mvdan.cc/gofumpt@/internal/gofumpt.go:50 +0x39
golang.org/x/tools/go/ast/astutil.(*application).apply(0xc000082120, 0x11c0a40, 0xc0000de100, 0x119047d, 0x5, 0xc000082168, 0x11c0b80, 0xc0000da080)
	golang.org/x/[email protected]/go/ast/astutil/rewrite.go:198 +0x3c18
golang.org/x/tools/go/ast/astutil.(*application).applyList(0xc000082120, 0x11c0a40, 0xc0000de100, 0x119047d, 0x5)
	golang.org/x/[email protected]/go/ast/astutil/rewrite.go:473 +0xa5
golang.org/x/tools/go/ast/astutil.(*application).apply(0xc000082120, 0x11c1240, 0xc0000d40c0, 0x119031a, 0x4, 0x0, 0x11c0a40, 0xc0000de100)
	golang.org/x/[email protected]/go/ast/astutil/rewrite.go:424 +0x13d7
golang.org/x/tools/go/ast/astutil.Apply(0x11c0a40, 0xc0000de100, 0xc0000d40a0, 0xc0000d40b0, 0x0, 0x0)
	golang.org/x/[email protected]/go/ast/astutil/rewrite.go:52 +0x14a
mvdan.cc/gofumpt/internal.Gofumpt(0xc0000b2240, 0xc0000de100)
	mvdan.cc/gofumpt@/internal/gofumpt.go:62 +0x10f
main.processFile(0x1192301, 0x10, 0x11bfea0, 0xc000078000, 0x11bfec0, 0xc000080000, 0x1, 0x0, 0x0)
	mvdan.cc/gofumpt@/gofmt.go:121 +0x1ff
main.gofumptMain()
	mvdan.cc/gofumpt@/gofmt.go:215 +0x2e6
main.main()
	mvdan.cc/gofumpt@/gofmt.go:186 +0x22

Temporary workaround: gofmt | gofumpt

Also panics on import("a";"b"), but the behavior is consistent with gofmt 🐛.

Ideally, it would be best to group all vars/consts that are defined close to each other.

Example:

bad:

var a = "a"
var b = "b"

good:

var (
	a = "a"
	b = "b"
)

As discussed in #51, gofumpt should pick up the Go version from a module's go.mod file so it can apply formatting that is specific to a range of Go version.

I’d love something like

https://go-review.googlesource.com/c/go/+/7593

and

https://go-review.googlesource.com/8050

But a bit more aggressive. I’ve been manually cleaning up excessive vertical whitespace in the compile for years because people were nervous about it.

Is there any way to use this with Jetbrains Goland or as a stand-alone CLI tool?

Hi!

Thanks for the project, it's really what I was looking for a long time!

What do you think about implementing a sort of enforcement for max line length in Go in a way how google-java-format does it (splitting lines).

Go 1.13 introduced 0o as a new prefix for octal integer literals while continuing to support the existing 0 prefix.

Right now gofumpt leaves both unchanged:

const _ = 0755
const _ = 0o755

(at the time of writing, GitHub's Go syntax highlighting does not recognize the new prefix)

gofumpt could ensure that all octal integer literals use a single style of prefix.

Currently gofumpt leaves empty declarations like

const ()

type ()

var ()

unchanged. Perhaps they should be removed?

Issue from #57

package main

import (
"std"
"non.std/pkg"
)

Expected:

package main

import (
    "std"

    "non.std/pkg"
)

Got:

package main

import (
    "std"
    "non.std/pkg"
)

which transform into the one above on second run of gofumpt.

import (
	"github.com/b/xxxx"
	"context"

	"time"

	"github.com/a/xxx"
)

I hope following code after gofumports.

import (
	"context"
	"time"

	"github.com/a/xxx"
	"github.com/b/xxxx"
)