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 · 33 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 · 33 comments · May be fixed by golang/tools#150

Comments

@vasi-stripe
Copy link

@vasi-stripe 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

@bradfitz bradfitz commented Jun 27, 2017

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 modified the milestones: Proposal, Unreleased Jun 27, 2017
@meggamind
Copy link

@meggamind meggamind commented Jun 30, 2017

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

@vasi-stripe
Copy link
Author

@vasi-stripe vasi-stripe commented Jun 30, 2017

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

@vasi-stripe vasi-stripe commented Jul 5, 2017

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

@wedneyyuri wedneyyuri commented Jul 13, 2017

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

@bradfitz bradfitz commented Jul 13, 2017

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

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

@dahankzter dahankzter commented May 24, 2018

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

@bradfitz bradfitz commented May 24, 2018

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

@gopherbot
Copy link

@gopherbot gopherbot commented Jun 6, 2018

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

@lhecker
Copy link

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

@bradfitz bradfitz commented Sep 16, 2018

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

@lhecker
Copy link

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

@akshayjshah akshayjshah commented Sep 17, 2018

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

@saheienko
Copy link

@saheienko saheienko commented Sep 17, 2018

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

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Oct 24, 2018

Nevermind, I rebased it. It's now reverted.

@shuyuwu
Copy link

@shuyuwu shuyuwu commented Dec 3, 2018

@ashi009 Maybe we can allow goimports to give people the right to sort imports in both the old way and the new way. By doing so, everyone's need can be satisfied.

@ash2k
Copy link

@ash2k ash2k commented Dec 4, 2018

gofmt does it one way and is not configurable and people like it that way. Why goimports should work differently? In fact, why imports formatting is not part of gofmt?

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Dec 4, 2018

We are not adding style preference knobs to goimports.

@nhooyr
Copy link
Contributor

@nhooyr nhooyr commented Mar 17, 2019

@bradfitz if I sent a change for determining the local import path with the module path, sorting all imports as the reverted commit does and also fixed the comment issues, is there a high chance of it being merged?

@nhooyr
Copy link
Contributor

@nhooyr nhooyr commented Mar 17, 2019

For anyone who wants this now and doesn't really care that the comments get misplaced, feel free to use go.coder.com/go-tools/cmd/goimports. That fork will be kept up to date for Coder's usage.

@ernado
Copy link
Contributor

@ernado ernado commented Jul 18, 2019

There is already a "knob":

  -local string
    	put imports beginning with this string after 3rd-party packages; comma-separated list

But it is pretty useless without full re-grouping :(

@alecbz
Copy link

@alecbz alecbz commented May 13, 2020

Is there any prescriptive behavior that goimports could adopt here that doesn't break the use-cases above?

One idea that addresses the original concern is to maybe reorganize imports more prescriptively from any import block that's "wrong" (contains imports from multiple sources) but leave any other blocks alone. So, e.g.:

  1.  import (
         "testing"
    
         "github.com/Sirupsen/logrus"
         "context"
     )

    Second block contains a mix of stdlib and 3rdparty, and should be broken up. goimports sees that there's an existing stdlib block, so it moves "context" there:

     import (
         "context"
         "testing"
    
         "github.com/Sirupsen/logrus"
     )
  2.  import (
         "testing"
    
         "github.com/Sirupsen/logrus"
    
         "github.com/org/repo1/foo/bar"
     )

    No blocks contain a mix of sources: nothing to do.

@alecbz
Copy link

@alecbz alecbz commented May 13, 2020

Though honestly, I would prefer a tool that was more prescriptive.

I see the benefits to not having knobs (things are more similar across different codebases, no need to argue about the values of the knobs), but by both not being prescriptive by default and not allowing for prescriptive knobs, we're back to needing humans to catch style violations in code review, needing authors to fix things manually, and maintaining a style guide, all of which feel worse than having knobs.

And as @ernado points out, there already is a knob fairly related to this functionality.

Spitballing: instead of a -local knob, maybe there could be a more generic knob that takes some specification of what sections are desired? Something like -sections stdlib,3rdparty,gitlab.com/mycompany/repo,gitlab.com/mycompany/repo/go/genproto

@gopherbot
Copy link

@gopherbot gopherbot commented May 20, 2021

Change https://golang.org/cl/321409 mentions this issue: internal/imports: merge mergeable import groups

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.