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

x/tools/cmd/goimports: support repairing import grouping/ordering #20818

Open
vasi-stripe opened this issue Jun 27, 2017 · 40 comments · May be fixed by golang/tools#150
Open

x/tools/cmd/goimports: support repairing import grouping/ordering #20818

vasi-stripe opened this issue Jun 27, 2017 · 40 comments · May be fixed by golang/tools#150
Labels
NeedsFix The path to resolution is known, but the work has not been done. Proposal Proposal-Accepted Tools This label describes issues relating to any tools in the x/tools repository.
Milestone

Comments

@vasi-stripe
Copy link

vasi-stripe commented Jun 27, 2017

I'm working on a tool to fix up non-conventional or incorrect imports grouping/ordering, but I'd much rather have this be part of goimports. Would it be welcome?

What goimports does now

When goimports adds or removes an import, it heuristically attempts to find a good place for it. So if I add an os.Open() call, and my imports already look like this:

import (
	"context"
	"testing"

	"github.com/Sirupsen/logrus"
)

Then goimports will put "os" in the proper spot, beween "context" and "testing". That's great!

What goimports does not do

If my imports are not grouped and sorted in the conventional way, goimports will not usually fix the problem. For example, if I was using the old context package, my imports might look like this:

import (
	"testing"

	"github.com/Sirupsen/logrus"
	"golang.org/x/net/context"
)

When I edit the context import to the new package, "context", I have this:

import (
	"testing"

	"github.com/Sirupsen/logrus"
	"context"
)

Running goimports will notice that logrus and context don't make a proper group, so it will separate them:

import (
	"testing"

	"context"

	"github.com/Sirupsen/logrus"
)

But this isn't what we really want. Ideally, it would just re-group and re-order all the imports, to yield:

import (
	"context"
	"testing"

	"github.com/Sirupsen/logrus"
)

Work so far

I have a tool that will globally reorder imports according to the grouping rules, including the associated doc-comments; see below. I could also develop similar funcitonality inside goimports.

My tool is not nearly perfect: It doesn't handle multiple import declarations; it doesn't know about import "C". It's not even clear what should happen to random non-doc-comments inside the import block, eg:

import (
	"github.com/Sirupsen/logrus"

	// When we re-order, what happens to this comment?

	"testing"
)

Automatically grouping and ordering would also be a behavioral change for goimports, so it would have to be behind a flag for now.

@gopherbot gopherbot added this to the Proposal milestone Jun 27, 2017
@bradfitz
Copy link
Contributor

Improving goimports is welcome.

@bradfitz bradfitz changed the title proposal: goimports: support repairing import grouping/ordering x/tools/cmd/goimports: support repairing import grouping/ordering Jun 27, 2017
@bradfitz bradfitz added NeedsFix The path to resolution is known, but the work has not been done. Proposal-Accepted labels Jun 27, 2017
@bradfitz bradfitz modified the milestones: Proposal, Unreleased Jun 27, 2017
@meggamind
Copy link

Hey can I please pick this issue ? I would need a bit guidance though.

@vasi-stripe
Copy link
Author

Here's what I did before: https://github.com/vasi-stripe/gogroup . Note that this is not exactly what we want in goimports, it still has a number of issues.

@vasi-stripe
Copy link
Author

So it turns out that goimports already does this, but only in limited cases. If a file is missing an import, and has multiple import decls, they get all merged together and then sorted. The merging seems to lose associated comments, but otherwise this should be possible to repurpose.

See https://github.com/golang/tools/blob/master/go/ast/astutil/imports.go#L151-L173

@wedneyyuri
Copy link

Hi @vasi-stripe, in my opinion the goimports should have an option to remove all blank spaces before formatting. Today, if I have imports unordered and separated by spaces the tool doesn't work as expected.

Example (goimports will not do anything):

import (
	"strings"

	"github.com/pkg/errors"

	"net/http"
)

Expected:

import (		
	"net/http"
        "strings"
        "github.com/pkg/errors"
)

@bradfitz
Copy link
Contributor

@wedneyyuri, I disagree with two things there:

(1) "goimports should have an option". We don't like options in code formatters.
(2) that you expect no blank line between the standard library imports and github stuff. goimports is opinionated in that regard.

I agree that the original bug report at the top is a bug, though. It should try to pack things nicely.

@aaronbee
Copy link

aaronbee commented Nov 28, 2017

I'd like to extend this proposal with a specific grouping of imports: stdlib, VCS repo-local, non-local.

For example,

package foo // github.com/org/repo1/foo

import (
    "context"
    "os"

    "github.com/org/repo1/foo/bar" // Same VCS repo as this file
    "github.com/org/repo1/foo/baz"

    "github.com/org/repo2/quux" // Different VCS repo
    "github.com/thirdparty/repo"
)

I have written a tool, importsort, that achieves this by finding the VCS root directory for the file that it is processing, then it looks at the imports in the file and checks if a path is prefixed by the VCS root directory import path.

@aaronbee
Copy link

aaronbee commented Dec 1, 2017

I just learned that goimports does allow grouping imports with the -local flag, but does so in the order of stdlib, non-local, local. That ordering is fine.

My proposal is determining local/non-local programmatically by looking at the VCS root directory.

@ash2k
Copy link
Contributor

ash2k commented Dec 5, 2017

I think ideally ordering should only depend on contents of the imports block. Reasons why not VCS/etc:

  • vendor vs GOPATH vs GOROOT vs ... Package may reside in various places and it may or may not be under VCS control. Different users of the same project may have different setup of their dependencies and goimports must (IMO) produce exactly the same output for them so it cannot rely on assumptions like this. Also some other tools store packages in completely different places - Google Bazel is an example (see Working with external dependencies).
  • I don't know how people use it but I personally like to run goimports before tests/builds to remove unnecessary imports, etc. The more work goimports needs to do to format the imports the less viable this option is. Doing more disk I/O is not great.

Why not something simple like this

import (
    "context" // Std lib goest first
    "os"

    "github.com/org/repo1/foo/bar" // Everything else goes second after one blank line
    "github.com/org/repo1/foo/baz"
    "github.com/org/repo2/quux" // Comment X
    "github.com/thirdparty/repo"
)

I would really like goimports to be as deterministic and as opinionated as possible. Like gofmt - nobody's favourite formatting but one of the most liked features. During PR reviews I've never had to say anything about formatting but many times about the "wrong" grouping/ordering of imports. Would be great to solve this problem once and for everybody.

@dahankzter
Copy link

Is anyone working on this? The algos for groupings is one thing but repairing holes in the simples possible stdlib/otherlibs can perhaps be done first?

@bradfitz
Copy link
Contributor

@dahankzter, I don't know of anybody working on this. They would generally announce so here.

@gopherbot
Copy link

Change https://golang.org/cl/116795 mentions this issue: x/tools/cmd/goimports: support repairing import grouping/ordering

@lhecker
Copy link

lhecker commented Sep 16, 2018

@bradfitz Is anything blocking the above "pull request"?
I've been using Serhii's changeset for a while now and IMO it makes working with goimports quite significantly more pleasant to use. I'd be nice if we could merge the changes sooner than later. 🙂

@bradfitz
Copy link
Contributor

@lhecker, vacation is. I'm back in a week. I'll get to it at some point after then.

@lhecker
Copy link

lhecker commented Sep 16, 2018

@bradfitz Oh I'm sorry. Don't bother with my silly question - Please do enjoy your vacation instead. 😄
We'll surely get around to this at some point either way. 🙂

@akshayjshah
Copy link
Contributor

Thanks for taking this on, @lhecker - I was just looking at how to integrate exactly this functionality into goimports.

@saheienko
Copy link

I'm using this too. There were some cases when it doesn't work properly, so it's better to use the last patch.

@thockin
Copy link

thockin commented Aug 17, 2021

I'm +1 on being more prescriptive and making goimports more aggressively normalize import blocks. There should be one true way to do it, including whitespace.

If we really need to add a knob, I'd want it to be IN the codebase (per file or per module) rather than a flag to the binary.

IMO, running goimports with no special args should be the canonical guidance to import ordering.

@rinchsan
Copy link

rinchsan commented Oct 6, 2021

As a workaround, I am using https://github.com/rinchsan/gosimports as an alternative goimports to make an import block style more consistent, but I am so happy if goimports gets more prescriptive and say goodbye to https://github.com/rinchsan/gosimports .

@pete-woods
Copy link

I've never seen so many engineers agree with each other! We at CircleCI are also planning to switch away from this tool to gosimports because it actually enforces the 3 sections, and we're wasting too much time manually correcting the results of goimports

yamamoto-febc added a commit to yamamoto-febc/iaas-service-go that referenced this issue Mar 16, 2022
golang/go#20818 のように空行が入る問題を回避するために
github.com/rinchsan/gosimports へ乗り換える。
yamamoto-febc added a commit to yamamoto-febc/iaas-service-go that referenced this issue Mar 16, 2022
golang/go#20818 のように空行が入る問題を回避するために
github.com/rinchsan/gosimports へ乗り換える。
yamamoto-febc added a commit to sacloud/iaas-service-go that referenced this issue Mar 16, 2022
golang/go#20818 のように空行が入る問題を回避するために
github.com/rinchsan/gosimports へ乗り換える。
@daixiang0
Copy link

daixiang0 commented Sep 1, 2022

GCI can do this and it is built-in in golangci-lint.

@yujiachen-y
Copy link

GCI can do this and it is built-in in golangci-lint.

@daixiang0 But GCI does not support multiple prefixes in one group daixiang0/gci#107

@daixiang0
Copy link

@yjc567 you can implement it in GCI.

@NonLogicalDev
Copy link

NonLogicalDev commented Jun 30, 2023

I have recently implemented a tool and framework for programmatically managing imports ordering that is

  • Deterministic
  • Handles tricky whitespace around imports gracefully
  • Handles comments gracefully
  • Handles user import groupings gracefully
  • Highly configurable and easy to extend and build on top of
  • Compatible with https://pkg.go.dev/golang.org/x/tools/go/analysis
  • Built directly on top of golang AST

https://github.com/NonLogicalDev/gofancyimports

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsFix The path to resolution is known, but the work has not been done. Proposal Proposal-Accepted Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Development

Successfully merging a pull request may close this issue.