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: do not make grouped imports non-grouped when removing #18051

Closed
xh3b4sd opened this issue Nov 25, 2016 · 6 comments
Closed

Comments

@xh3b4sd
Copy link

@xh3b4sd xh3b4sd commented Nov 25, 2016

What did you do?

I have this file.

package main

func main() {
	s := []string{}
	ss := strings.Join(s, ",")

	fmt.Printf("%#v\n", ss)
}

Running goimports does this.

package main

import (
	"fmt"
	"strings"
)

func main() {
	s := []string{}
	ss := strings.Join(s, ",")

	fmt.Printf("%#v\n", ss)
}

Removing the line where the fmt package is used does this when goimports is run.

package main

import "strings"

func main() {
	s := []string{}
	ss := strings.Join(s, ",")

}

What did you expect to see?

I would like the import statement always to be "multiple", no matter how many imports the package has.

import (
	....
)

What did you see instead?

On packages suddenly having only one imported package, the import statement is "single". I am a little OCDish on this one. Diffs would be way more nicer and the import statements across files and packages would be way more consistent when the import statement would have always be the same form.

Cheers, Tim.

@josharian josharian changed the title x/tools/cmd/goimports: x/tools/cmd/goimports: do not make grouped imports non-grouped when removing Nov 25, 2016
@mvdan
Copy link
Member

@mvdan mvdan commented Nov 25, 2016

I think this is consistent with what @bradfitz said in #18027 (comment).

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Nov 25, 2016

I don't care either way, but because the code is one way already, I think it's easiest to not change anything. But if others feel strongly, and others want to do the work, I'm also okay with that.

@bradfitz bradfitz added this to the Unreleased milestone Nov 25, 2016
@josharian
Copy link
Contributor

@josharian josharian commented Nov 27, 2016

After mulling this, I'm weakly inclined to change it, despite finding non-grouped lone imports more aesthetically appealing. The diff/merge argument is fairly compelling, and keeping grouped imports grouped eliminates a rare but obnoxious class of bugs around comment-handling.

If we change it, I suggest the following (probably obvious but still worth specifying) behavior:

  • goimports not rewrite existing non-grouped imports to be grouped, to avoid churn when running goimports on already-goimports-clean code
  • when going from some to zero imports, goimports entirely eliminate the import statement (rather than leaving import ())

I'm unlikely to go do this myself soon, but if someone wants to go tackle this, I'll help/review the CL. Test coverage is pretty good, and the relevant bit of code is pretty simple, so this should be a fairly accessible change for someone wanting to learn this codebase (insofar as anything involving goimports is accessible).

@haya14busa
Copy link
Contributor

@haya14busa haya14busa commented Feb 8, 2017

I investigated this issue a little.

Just commenting out go/ast/astutil/imports.go#L237 fixes this issue.

} else if len(gen.Specs) == 1 {
	// ...
	gen.Lparen = token.NoPos // drop parens
	// ...
}

This line is originally added in src/cmd/gofix/fix.go https://codereview.appspot.com/4630056/diff/9008/src/cmd/gofix/fix.go#newcode528

Comments nor commit message doesn't say why removing paran, but I assume it just intends to make code simple.

So, option 1 is delete go/ast/astutil/imports.go#L237 line.
I prefer removing it if it's possible, but I guess it's not acceptable change...? At least discussion is required, i guess.

Second option is save import statements are grouped or not and restore parens of a import statement which was originally grouped imports.
I think it's possible but I didn't write code yet and it's going to be complecated compared to option 1.

when going from some to zero imports, goimports entirely eliminate the import statement (rather than leaving import ())

btw, test for this behavior already exists https://github.com/golang/tools/blob/master/imports/fix_test.go#L199-L216

@gopherbot
Copy link

@gopherbot gopherbot commented Feb 12, 2017

CL https://golang.org/cl/36853 mentions this issue.

@haya14busa
Copy link
Contributor

@haya14busa haya14busa commented Feb 12, 2017

I wondered this change may not be accepted because it's a breaking change, but more
I looked into the code, the more I inclined to support to change this behavior.

Especially if there were comments for import statements, it would mess up comments.

// comment 1
import (
	f "fmt"
	// comment 2
	i "io"
)

Removing "fmt" import results in following code in current implementation.

// comment 1

// comment 2
import i "io"

I think it should be

// comment 1
import (
	// comment 2
	i "io"
)
@golang golang locked and limited conversation to collaborators Feb 13, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants
You can’t perform that action at this time.