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

cmd/gofmt: consider sorting imports in the same way as goimports (separating std lib from third party) #37719

Open
JeremyLoy opened this issue Mar 6, 2020 · 10 comments
Milestone

Comments

@JeremyLoy
Copy link

@JeremyLoy JeremyLoy commented Mar 6, 2020

What version of Go are you using (go version)?

1.14

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

MacOS

What did you do?

ran go fmt ./.. on my codebase

What did you expect to see?

The builtin go fmt formats imports identically to goimports

What did you see instead?

go fmt works differently than goimports

Downloading of goimports is necessary to work in certain repos and adhere to their style guide. One impetus of having go fmt built in is to not have these sorts of disagreements, and instead just rally around a single tool. The existence and continued support for goimports goes against this philosophy. In addition, there is poor visibility in the public eye of goimports. Its not discoverable via the command line, as it isn't built in like vet and fmt. The only way people learn of this other tool is via blog posts or poking around Github.

I propose that the builtin go fmt sorts imports in the same way as goimports starting with next Go versioned release, and development of goimports ceases.

If we want to maintain two different sets of rules, I think a good compromise would be to add a -imports flag to the builtin go fmt so that we aren't maintaining two tools for one job

@JeremyLoy JeremyLoy changed the title We should have one universal format tool (incorporate goimports into gofmt). cmd/go: We should have one universal format tool (incorporate goimports into gofmt). Mar 6, 2020
@JeremyLoy JeremyLoy changed the title cmd/go: We should have one universal format tool (incorporate goimports into gofmt). cmd/go: We should have one universal format tool (incorporate goimports into go fmt). Mar 6, 2020
@josharian
Copy link
Contributor

@josharian josharian commented Mar 7, 2020

Can you give a concrete example of code formatting generated by goimports and then modified by gofmt? Thanks.

@JeremyLoy
Copy link
Author

@JeremyLoy JeremyLoy commented Mar 7, 2020

Original

package main

import (
	"strings"
	"github.com/gorilla/mux"
	"fmt"
)

func main() {
var _ = mux.NewRouter()
	fmt.Println(strings.ToUpper("Hello, world!"))
}

go fmt (note the alphabetized imports)

package main

import (
	"fmt"
	"github.com/gorilla/mux"
	"strings"
)

func main() {
	var _ = mux.NewRouter()
	fmt.Println(strings.ToUpper("Hello, world!"))
}

goimports (note the imports grouped stdlib, newline, external)

package main

import (
	"fmt"
	"strings"

	"github.com/gorilla/mux"
)

func main() {
	var _ = mux.NewRouter()
	fmt.Println(strings.ToUpper("Hello, world!"))
}
@josharian
Copy link
Contributor

@josharian josharian commented Mar 7, 2020

I see. In that case, I believe that this may be a duplicate of #9922.

@JeremyLoy
Copy link
Author

@JeremyLoy JeremyLoy commented Mar 7, 2020

@rsc was pretty clear in that post that gofmt shouldn’t use goimports rules, and I can respect that. That statement was made almost 5 years ago though, so I wanted to bring it up again under a different light.

I created this issue because there are currently two tools maintained in the Golang org, one pinned with Go releases and another under the x banner.

I respect that not everyone may want their code formatted the way goimports does, however I think maintaining two different tools is wasted effort and confusing to newcomers and veterans alike, especially considering how popular goimports is.

Hence, why I’m proposing merging the two with the implementation being an optional flag

@josharian
Copy link
Contributor

@josharian josharian commented Mar 7, 2020

@heschik
Copy link
Contributor

@heschik heschik commented Mar 7, 2020

My immediate reaction is that goimports is too complicated and buggy to be tied to the main Go release cycle. The import grouping specifically has known bugs related to comments. But it has been getting more stable over time, so maybe?

@mvdan
Copy link
Member

@mvdan mvdan commented Mar 9, 2020

I propose that the builtin go fmt sorts imports in the same way as goimports starting with next Go versioned release, and development of goimports ceases.

Perhaps I'm missing something, but goimports does much more than just sorting imports. It also adds missing imports, for example, which is a very complex task with modules.

I'm generally in favor of making the two tools easier to use together, but deprecating goimports entirely wouldn't be as simple as teaching gofmt how to sort imports.

@JeremyLoy
Copy link
Author

@JeremyLoy JeremyLoy commented Mar 9, 2020

Interesting, I didn’t realize it removed/added imports until just now.

My intent with creating this issue was solely to rectify the formatting discrepancy. Perhaps this issue could be more focused on gofmt then.

@dmitshur dmitshur changed the title cmd/go: We should have one universal format tool (incorporate goimports into go fmt). cmd/gofmt: consider sorting imports in the same way as goimports (separating std lib from third party) Mar 9, 2020
@dmitshur dmitshur added this to the Backlog milestone Mar 9, 2020
@bcmills
Copy link
Member

@bcmills bcmills commented Mar 13, 2020

Would it be feasible to refactor cmd/gofmt to share code with goimports, similar to how cmd/vet now uses dependencies vendored from golang.org/x/tools/go/analysis?

That would at least make it easier to keep the two in sync.

@agnivade
Copy link
Contributor

@agnivade agnivade commented Mar 13, 2020

Would it be feasible to refactor cmd/gofmt to share code with goimports

That's a good point. There's even a comment which says: // Hacked up copy of go/ast/import.go in tools/internal/imports/sortimports.go. There was a fix with import ordering that I had done in gofmt, which had to be replicated again in goimports.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
7 participants
You can’t perform that action at this time.