segmentio / golines

A golang formatter that fixes long lines
MIT License
903 stars 56 forks source link

The gofumpt formatter doesn't seem to work as a base formatter #100

Open fgimian opened 1 year ago

fgimian commented 1 year ago

Hey there, hope you're doing well! I can't seem to get gofumpt working as the base formatter when using golines (both gofmt and golines work perfectly).

I've used the number 0644 as a test for gofumpt which would be reformatted to 0o644.

~\source\go\test via 🐹 v1.20.2
🕙 [ 11:13:41 AM ] ❯ type .\main.go
package main

import "fmt"

func main() {
        fmt.Println("This is an extremely long line", "with multiple strings", "which I'm hoping will be", "split up by golines")
        x := 0644
        fmt.Println(x)
}

~\source\go\test via 🐹 v1.20.2
🕙 [ 11:13:54 AM ] ❯ gofumpt.exe .\main.go
package main

import "fmt"

func main() {
        fmt.Println("This is an extremely long line", "with multiple strings", "which I'm hoping will be", "split up by golines")
        x := 0o644
        fmt.Println(x)
}

~\source\go\test via 🐹 v1.20.2
🕙 [ 11:13:58 AM ] ❯ golines.exe .\main.go
package main

import "fmt"

func main() {
        fmt.Println(
                "This is an extremely long line",
                "with multiple strings",
                "which I'm hoping will be",
                "split up by golines",
        )
        x := 0644
        fmt.Println(x)
}

~\source\go\test via 🐹 v1.20.2
🕙 [ 11:14:02 AM ] ❯ golines.exe --base-formatter=gofumpt main.go
package main

import "fmt"

func main() {
        fmt.Println(
                "This is an extremely long line",
                "with multiple strings",
                "which I'm hoping will be",
                "split up by golines",
        )
        x := 0644
        fmt.Println(x)
}

Versions may be found below:

~\source\go\test via 🐹 v1.20.2
🕙 [ 11:15:41 AM ] ❯ gofumpt.exe --version
v0.4.0 (go1.20.2)

~\source\go\test via 🐹 v1.20.2
🕙 [ 11:15:43 AM ] ❯ golines.exe --version
golines v0.11.0

~\source\go\test via 🐹 v1.20.2
🕙 [ 11:15:45 AM ] ❯ go version
go version go1.20.2 windows/amd64

Thanks heaps Fotis

telemachus commented 1 year ago

tl;dr—try golines --base-formatter='gofumpt .'

This problem (I think) has to do with that specific rule in gofumpt. On gofumpt's README, that rule is specified this way: "Octal integer literals should use the 0o prefix on modules using Go 1.13 and later" (emphasis mine). In order to know whether the code is from a module using Go 1.13 or later, I'm assuming gofumpt checks the contents of a go.mod file. But if you run golines normally, it feeds files to the formatter one at a time (I believe). gofumpt will not apply that rule to a single file since it cannot determine what version of Go the file runs under.

On my machine, a workaround is to tell gofumpt explicitly to look at the whole directory (i.e., check for a go.mod file). Invoke golines as golines --base-formatter='gofumpt .' to make sure it checks the entire directory.

Unfortunately, this may lead to added work in a very large directory because gofumpt may end up being invoked over and over for each individual file, each time checking the entire directory as well. I'd have to recheck the internals of golines to be sure about that, and I'm not sure if there's a straightforward or quick fix. (Ideally, perhaps, golines should first use the formatter on the entire directory and then run through each file itself one at a time?)

That said, this should fix your immediate problem in the short term. (Caveat: I am on a Mac, and I'm only familiar with shell-quoting rules on Unix-like machines. There may be some other exact quoting you need to pass gofumpt . to your shell on Windows.)

Even if your problem is fixed, perhaps leave this issue open for @yolken to take a look at. There may be a useful optimization to add to golines here concerning how formatters are applied to directories. I will try to take a look at this myself when I get a chance.

Thanks for the detailed bug-report.

fgimian commented 1 year ago

tl;dr—try golines --base-formatter='gofumpt .'

This problem (I think) has to do with that specific rule in gofumpt. On gofumpt's README, that rule is specified this way: "Octal integer literals should use the 0o prefix on modules using Go 1.13 and later" (emphasis mine). In order to know whether the code is from a module using Go 1.13 or later, I'm assuming gofumpt checks the contents of a go.mod file. But if you run golines normally, it feeds files to the formatter one at a time (I believe). gofumpt will not apply that rule to a single file since it cannot determine what version of Go the file runs under.

On my machine, a workaround is to tell gofumpt explicitly to look at the whole directory (i.e., check for a go.mod file). Invoke golines as golines --base-formatter='gofumpt .' to make sure it checks the entire directory.

Unfortunately, this may lead to added work in a very large directory because gofumpt may end up being invoked over and over for each individual file, each time checking the entire directory as well. I'd have to recheck the internals of golines to be sure about that, and I'm not sure if there's a straightforward or quick fix. (Ideally, perhaps, golines should first use the formatter on the entire directory and then run through each file itself one at a time?)

That said, this should fix your immediate problem in the short term. (Caveat: I am on a Mac, and I'm only familiar with shell-quoting rules on Unix-like machines. There may be some other exact quoting you need to pass gofumpt . to your shell on Windows.)

Even if your problem is fixed, perhaps leave this issue open for @yolken to take a look at. There may be a useful optimization to add to golines here concerning how formatters are applied to directories. I will try to take a look at this myself when I get a chance.

Thanks for the detailed bug-report.

Thank you so so much for the reply and explanation, that makes sense. 😄

My thought process was to attenpt to disable the LSP-based formatting and use the run on save extension to only run golines, otherwise we're effectively running gofmt / gofumpt / goimports twice, once via the LSP server and once via golines.

Thank you so much and keep up the great work!! Fotis