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

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

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
Contributor

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

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

@dmitshur
Copy link
Contributor

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.

@danicat
Copy link
Contributor

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
Contributor

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

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

This is a good place to discuss

@gopherbot
Copy link

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
Development

No branches or pull requests

5 participants