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
Comments
I think this is consistent with what @bradfitz said in #18027 (comment). |
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. |
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:
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). |
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. Second option is save import statements are grouped or not and restore parens of a import statement which was originally grouped imports.
btw, test for this behavior already exists https://github.com/golang/tools/blob/master/imports/fix_test.go#L199-L216 |
CL https://golang.org/cl/36853 mentions this issue. |
I wondered this change may not be accepted because it's a breaking change, but more 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"
) |
What did you do?
I have this file.
Running
goimports
does this.Removing the line where the
fmt
package is used does this whengoimports
is run.What did you expect to see?
I would like the import statement always to be "multiple", no matter how many imports the package has.
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.
The text was updated successfully, but these errors were encountered: