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: package clause insertion mangles comments #12097

Closed
josharian opened this issue Aug 10, 2015 · 10 comments
Closed

x/tools/cmd/goimports: package clause insertion mangles comments #12097

josharian opened this issue Aug 10, 2015 · 10 comments

Comments

@josharian
Copy link
Contributor

@josharian josharian commented Aug 10, 2015

Input:

// a
// b
// c

func main() {
    fmt.Println()
}

gofmt fails appropriately:

$ gofmt x.go 
x.go:5:1: expected 'package', found 'func'

goimports inserts a package clause and mangles the comments:

$ goimports x.go 
package main // a
import "fmt"

// b
// c

func main() {
    fmt.Println()
}
@josharian josharian added this to the Unreleased milestone Aug 10, 2015
@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Aug 10, 2015

The package main insertion is intentional. It was requested and done as a feature at Gophercon 1's hack day.

The comment mangling of course is not.

@josharian josharian changed the title x/tools/cmd/goimports: should fail when package clause not present x/tools/cmd/goimports: package clause insertion mangles comments Aug 10, 2015
@dmitshur
Copy link
Member

@dmitshur dmitshur commented Apr 25, 2016

Is this the same issue or a different issue?

Edit: Moved out into #15432 (comment).

@josharian
Copy link
Contributor Author

@josharian josharian commented Apr 25, 2016

Hard to say whether it is the same issue without looking into the fix. Good to have multiple test cases regardless.

@dmitshur
Copy link
Member

@dmitshur dmitshur commented Apr 25, 2016

Ok, I'll delete the comment and move this into a separate issue then. In case the same fix resolves both issues, great, otherwise they can be looked at separately.

@bradfitz bradfitz added the help wanted label Feb 2, 2017
@danicat
Copy link
Contributor

@danicat danicat commented Jul 24, 2017

Is anybody working on this issue yet? I'm looking for something to contribute and it seems like a good one to start. What's the expected output of the given example?

@dmitshur
Copy link
Member

@dmitshur dmitshur commented Jul 24, 2017

I think the expected output would be:

package main

import "fmt"

// a
// b
// c

func main() {
	fmt.Println()
}

(Unless that conflicts with some existing behaviors.)

@josharian
Copy link
Contributor Author

@josharian josharian commented Jul 24, 2017

Welcome! I don't think anyone is working on it.

I guess I'd expect the package and imports to be inserted at the beginning of the file, with a blank line between the imports and the first comment. But it's not obvious, so if there's a good reason to do otherwise, I'm open to it.

Messing with the AST and comment positioning can be quite fiddly, so I recommend writing all your test cases first. Feel free to ask for help if needed.

Note that the likely reviewers for this (Brad, Robert, me) are all going to be slow, for our own reasons.

@danicat
Copy link
Contributor

@danicat danicat commented Jul 24, 2017

Great! I'll start working on this issue then. There is a better channel to talk to you if I need help or should I just write it on this thread? I promise I won't bug you without exhausting all my options first :)

@josharian
Copy link
Contributor Author

@josharian josharian commented Jul 24, 2017

This is a good place to discuss

@gopherbot
Copy link

@gopherbot gopherbot commented Feb 11, 2018

Change https://golang.org/cl/93235 mentions this issue: imports: fix mangled comments after package clause insertion

@golang golang locked and limited conversation to collaborators Feb 14, 2019
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
5 participants
You can’t perform that action at this time.