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
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. Tools This label describes issues relating to any tools in the x/tools repository.
Milestone

Comments

@prashantv
Copy link
Contributor

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
Contributor

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.

@dmitshur dmitshur added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Sep 14, 2018
@prashantv
Copy link
Contributor Author

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
Contributor

Related issue is #20818.

@akshayjshah
Copy link
Contributor

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 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 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 commented Dec 5, 2018

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

@gopherbot gopherbot added the Tools This label describes issues relating to any tools in the x/tools repository. label Sep 12, 2019
@heschi
Copy link
Contributor

heschi 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
Contributor

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?

@heschi
Copy link
Contributor

heschi 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

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.

@heschi
Copy link
Contributor

heschi commented Nov 8, 2019

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

@golang golang locked and limited conversation to collaborators Nov 7, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Development

No branches or pull requests

9 participants