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: Allow "-local" imports to be sorted in between standard library and third party imports #27673

Closed
prashantv opened this issue Sep 14, 2018 · 12 comments

Comments

@prashantv
Copy link
Contributor

@prashantv prashantv commented Sep 14, 2018

goimports supports a -local flag to identify local imports, which are currently sorted after 3rd party import groups. However, this doesn't match the import ordering of some projects which sort local imports last. E.g.,

Should goimports have flags to control the import group ordering, or should local packages always be imported last?

@gopherbot gopherbot added this to the Unreleased milestone Sep 14, 2018
@dmitshur
Copy link
Member

@dmitshur dmitshur commented Sep 14, 2018

I think it's highly beneficial to avoid adding flags if possible. gofmt is already pretty opinionated, and people accept that. It has no flags (more or less), which means all projects are formatted the same way. It's better if goimports follows that and leads to more Go codebases following the same formatting style, and save people from having to make a meaningless decision of where to put local imports.

However, this doesn't match the import ordering of some projects which sort local imports last.

A better way to resolve this would be to change those packages to be consistent with what goimports does (easy), or perhaps change goimports if everyone seems to be doing something else (hard; how to confirm this?).

/cc @bradfitz @josharian per owners.

@prashantv
Copy link
Contributor Author

@prashantv prashantv commented Sep 14, 2018

Yeah, I don't really like the idea of a separate flag, but the -local flag has already added some divergence. It'd be nice to agree on what the preferred grouping is:

(a) 2 groups: "standard" and "everything else"
(b) 3 groups: "standard" "non-local" "local"

@dmitshur
Copy link
Member

@dmitshur dmitshur commented Sep 17, 2018

Related issue is #20818.

@akshayjshah
Copy link

@akshayjshah akshayjshah commented Sep 18, 2018

Worth noting here that the example in the CodeReviewComments section on imports seems to suggest a three-group pattern: standard library, local imports, then third-party imports. It'd be nice to align that wiki page with the default goimports style.

@axw
Copy link
Contributor

@axw axw commented Oct 15, 2018

golang/tools@12a7c31 changed goimports such that groups are combined. e.g.

import (
  "os"

  "github.com/aws/aws-lambda-go/lambdacontext"

  "github.com/elastic/apm-agent-go"
)

becomes

import (
  "os"

  "github.com/aws/aws-lambda-go/lambdacontext"
  "github.com/elastic/apm-agent-go"
)

When using -local, the local packages will be separated as before, such that in the above example, the first grouping would be left intact. This works fine, but then goimports integration with IDEs makes that painful, as IDE-specific configuration will be required to pass in -local. Previously this wasn't any issue, because groups were left alone.

Yeah, I don't really like the idea of a separate flag, but the -local flag has already added some divergence. It'd be nice to agree on what the preferred grouping is:

(a) 2 groups: "standard" and "everything else"
(b) 3 groups: "standard" "non-local" "local"

I'm in favour of one canonical grouping scheme, and my preference is (b). In order for (b) to work, I think we need a way of identifying the local imports without -local, e.g. using a special comment or a config file.

@ashi009
Copy link

@ashi009 ashi009 commented Oct 24, 2018

I'm in favour of one canonical grouping scheme, and my preference is (b). In order for (b) to work, I think we need a way of identifying the local imports without -local, e.g. using a special comment or a config file.

I don't like the idea to have a canonical grouping scheme. Because there is no canonical way to name packages and importpaths, there is no way for goimports to be smart enough to do the grouping in a sensible way.

However, a config file might work -- put packages matching certain rules in a certain group -- but such config sounds troublesome and I'd prefer to avoid.

@smyrman
Copy link

@smyrman smyrman commented Dec 5, 2018

With go modules (go 1.11) One could defer the -local from the go.mod file.

@heschik
Copy link
Contributor

@heschik heschik commented Nov 7, 2019

Is there something specific to be done here? I think we're pretty clear that we don't want any more configuration for goimports unless absolutely necessary.

@ash2k
Copy link

@ash2k ash2k commented Nov 7, 2019

Why not make goimports part of gofmt, removing all configuration? Just sort the imports (and maybe group stdlib and "everything else" separately) and that's it. Why not?

@heschik
Copy link
Contributor

@heschik heschik commented Nov 7, 2019

The full functionality of goimports is way too complicated, subtle, and slow to be integrated into gofmt. Sorting and grouping imports I suppose you could make a case for but that doesn't even work right in goimports today, see #20818.

@prashantv
Copy link
Contributor Author

@prashantv prashantv commented Nov 8, 2019

Is there something specific to be done here? I think we're pretty clear that we don't want any more configuration for goimports unless absolutely necessary.

Makes sense to me to avoid any more flags. I think that means the recommendation is to sort local packages in the 3rd group? It would be nice if this was called out and examples (e.g., style guide) reflected that.

A separate unrelated issue, it may be more reasonable to auto-detect -local based on the go.mod. That would allow a user to set their editor to run goimports -w -autolocal (or whatever the flag may be) and it would work automatically for any project. However, that can be a separate issue.

@prashantv prashantv closed this Nov 8, 2019
@heschik
Copy link
Contributor

@heschik heschik commented Nov 8, 2019

That's https://golang.org/issue/32049, basically.

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
9 participants
You can’t perform that action at this time.