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

internal/imports: force to repair imports group regardless of how the… #150

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

zoumo
Copy link
Contributor

@zoumo zoumo commented Sep 5, 2019

What this PR does

Add a new flag repair-group. If true, forcing to repair imports group regardless of how they were originally grouped

Fixed golang/go#20818

Why

What goimports does not do

example:

import (
	"testing"

	"context"

	"github.com/Sirupsen/logrus"
)

The imports will keep intact because "testing" and "context" are considered in two different sections.

Manually correct

If we delete the blank line, goimports will re-sort imports correctly.

input:

import (
	"testing"
	"context"

	"github.com/Sirupsen/logrus"
)

after formatting:

import (
	"context"
	"testing"

	"github.com/Sirupsen/logrus"
)

Root case

When goimports formats the imports, it puts imports into different sections by successive lines, and sorts them in each section. It turns out that sorting will only happens in the same section.

In order to solve this problem, we only need to consider all imports in one group regardless of how they were originally grouped

Result of use repair-group

input:

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

import (
    "context"

    "os"

    "github.com/org/repo1/foo/bar"
 
    "github.com/org/repo1/foo/baz"

    "github.com/org/repo2/quux"

    "github.com/thirdparty/repo"
)

runs

goimports -repair-group -local github.com/org/repo1

output:

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

import (
    "context" 
    "os"

    "github.com/org/repo2/quux"
    "github.com/thirdparty/repo"

    "github.com/org/repo1/foo/bar"
    "github.com/org/repo1/foo/baz"
)

@gopherbot
Copy link
Contributor

This PR (HEAD: 21a9fe9) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/tools/+/193499 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

@gopherbot
Copy link
Contributor

Message from Gobot Gobot:

Patch Set 1:

Congratulations on opening your first change. Thank you for your contribution!

Next steps:
Within the next week or so, a maintainer will review your change and provide
feedback. See https://golang.org/doc/contribute.html#review for more info and
tips to get your patch through code review.

Most changes in the Go project go through a few rounds of revision. This can be
surprising to people new to the project. The careful, iterative review process
is our way of helping mentor contributors and ensuring that their contributions
have a lasting impact.

During May-July and Nov-Jan the Go project is in a code freeze, during which
little code gets reviewed or merged. If a reviewer responds with a comment like
R=go1.11, it means that this CL will be reviewed as part of the next development
cycle. See https://golang.org/s/release for more details.


Please don’t reply on this GitHub thread. Visit golang.org/cl/193499.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Heschi Kreinick:

Patch Set 1:

I think this should be discussed on the issue. My feeling is that if the functionality is reliable it should probably be on by default, and if it's not reliable it shouldn't be merged at all. But others may have other opinions.

I haven't looked at the code yet, but I see that it has no tests. We definitely need some before this can be merged. In particular, whatever triggered the rollback in https://golang.org/cl/144339 should be covered.


Please don’t reply on this GitHub thread. Visit golang.org/cl/193499.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Brad Fitzpatrick:

Patch Set 1:

(1 comment)


Please don’t reply on this GitHub thread. Visit golang.org/cl/193499.
After addressing review feedback, remember to publish your drafts!

@harveyxia
Copy link

Is this PR still being worked on? This functionality would be very useful for us.

@SVilgelm
Copy link

SVilgelm commented Mar 6, 2020

Hey, I'm also interesting in this PR.
@zoumo ?

@zoumo
Copy link
Contributor Author

zoumo commented Mar 12, 2020

@harveyxia @SVilgelm
The maintainers are not inclined to merge this PR before they decide how to repair import grouping/ordering.
This PR can work for me but not nearly perfect, it will mangles comments in imports.
You can still fork it and build your own goimport binary.

@nezorflame
Copy link

nezorflame commented Apr 16, 2020

AFAIK this is not the first attempt at it (example: #68) and it was denied by @bradfitz. Did something change since then?
To be honest, I think that this kind of functionality should just be on by default.

@kamilsk
Copy link

kamilsk commented Apr 16, 2020

as a temporary solution, you can use

curl -sSfL https://bit.ly/install-goimports | sh -s -- -b $(go env GOPATH)/bin

details: https://github.com/kamilsk/go-tools/releases/tag/goimports.
the flag is different: -ungroup

@rubensayshi
Copy link

I think this change would be really helpful, imports get messy quickly, specially when you let IDEs do auto imports.
and most IDEs mimic or even rely on gofmt and/or goimports for keeping the imports clean ...

@gopherbot
Copy link
Contributor

Message from Go Bot:

Patch Set 1:

Congratulations on opening your first change. Thank you for your contribution!

Next steps:
Within the next week or so, a maintainer will review your change and provide
feedback. See https://golang.org/doc/contribute.html#review for more info and
tips to get your patch through code review.

Most changes in the Go project go through a few rounds of revision. This can be
surprising to people new to the project. The careful, iterative review process
is our way of helping mentor contributors and ensuring that their contributions
have a lasting impact.

During May-July and Nov-Jan the Go project is in a code freeze, during which
little code gets reviewed or merged. If a reviewer responds with a comment like
R=go1.11, it means that this CL will be reviewed as part of the next development
cycle. See https://golang.org/s/release for more details.


Please don’t reply on this GitHub thread. Visit golang.org/cl/193499.
After addressing review feedback, remember to publish your drafts!

@nezorflame
Copy link

Is this PR dead?

@m-messiah
Copy link

Can we bump this PR to be merged?

bsponge added a commit to bsponge/tools that referenced this pull request Sep 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

x/tools/cmd/goimports: support repairing import grouping/ordering
9 participants