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

Jump to bottom

x/tools/cmd/goimports: moves comment to inappropriate line #23709

Closed
dfawley opened this issue Feb 5, 2018 · 11 comments
Closed

x/tools/cmd/goimports: moves comment to inappropriate line #23709

dfawley opened this issue Feb 5, 2018 · 11 comments
Labels
FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Unreleased

Comments

@dfawley
Copy link

dfawley commented Feb 5, 2018

Create a file with the following contents:

package main

import (
	"math" // fun
)

func main() {
	x := math.MaxInt64
	fmt.Println(strings.Join(",", []string{"hi"}), x)
}
$ goimports -d /tmp/a.go
diff -u /tmp/a.go.orig /tmp/a.go
--- /tmp/a.go.orig	2018-02-05 15:44:54.612402902 -0800
+++ /tmp/a.go	2018-02-05 15:44:54.612402902 -0800
@@ -1,7 +1,9 @@
 package main
 
 import (
-	"math" // fun
+	"fmt"
+	"math"
+	"strings" // fun
 )
 
 func main() {

Expected:

$ goimports -d /tmp/a.go
diff -u /tmp/a.go.orig /tmp/a.go
--- /tmp/a.go.orig	2018-02-05 15:44:54.612402902 -0800
+++ /tmp/a.go	2018-02-05 15:44:54.612402902 -0800
@@ -1,7 +1,9 @@
 package main
 
 import (
+	"fmt"
	"math" // fun
+	"strings"
 )
 
 func main() {

Versions tested:

go version go1.10rc1 linux/amd64
go version go1.9.3 linux/amd64
@gopherbot gopherbot added this to the Unreleased milestone Feb 5, 2018
@bradfitz bradfitz added help wanted NeedsFix The path to resolution is known, but the work has not been done. labels Feb 6, 2018
@gopherbot
Copy link

Change https://golang.org/cl/93055 mentions this issue: imports: fixup comments on import lines correctly

@cuonglm
Copy link
Member

cuonglm commented Feb 9, 2018

@bradfitz Can you take a look?

@lukelafountaine
Copy link

I would like to start contributing to Go. Would this be a good issue to cut my teeth on?

@ianlancetaylor
Copy link
Contributor

@lukelafountaine it looks like someone may have already sent a patch for this--see the comment about CL 93055 above.

@cuonglm
Copy link
Member

cuonglm commented Jun 29, 2018

I'm not sure it's a problem, but this line:

https://github.com/golang/tools/blob/master/go/ast/astutil/imports.go#L141

set all imported line to the same position in file, then when sorting it, the current position is tracked:

https://github.com/golang/tools/blob/master/imports/sortimports.go#L106

In this example, math and strings both have EndPos at 38, the same for // fun, so it ends up display beside strings.

@gopherbot
Copy link

Change https://golang.org/cl/121878 mentions this issue: imports: fixup comments on import lines correctly

@cuonglm
Copy link
Member

cuonglm commented Jul 2, 2018

cc @bradfitz I sent new CL for this issue, please take a look

@ncw
Copy link
Contributor

ncw commented Jul 3, 2018

This change appears to have broken goimports in a different way putting spurious blank lines after import lines with comments.

~/go/src/github.com/ncw/rclone$ find . -name *.go | grep -v /vendor/ | xargs goimports -d

diff -u ./cmd/serve/webdav/webdav.go.orig ./cmd/serve/webdav/webdav.go
--- ./cmd/serve/webdav/webdav.go.orig	2018-07-03 22:24:12.339811431 +0100
+++ ./cmd/serve/webdav/webdav.go	2018-07-03 22:24:12.339811431 +0100
@@ -16,6 +16,7 @@
 	"github.com/ncw/rclone/vfs/vfsflags"
 	"github.com/spf13/cobra"
 	"golang.org/x/net/context" // switch to "context" when we stop supporting go1.8
+
 	"golang.org/x/net/webdav"
 )
 
diff -u ./fs/rc/rc.go.orig ./fs/rc/rc.go
--- ./fs/rc/rc.go.orig	2018-07-03 22:24:13.099814169 +0100
+++ ./fs/rc/rc.go	2018-07-03 22:24:13.099814169 +0100
@@ -12,6 +12,7 @@
 	"io"
 	"net/http"
 	_ "net/http/pprof" // install the pprof http handlers
+
 	"strings"
 
 	"github.com/ncw/rclone/cmd/serve/httplib"

I don't know if that was intended but before this change goimports didn't insert that spurious blank line.

ncw added a commit to rclone/rclone that referenced this issue Jul 3, 2018
@ncw
build: fix for goimports format change
8c0335a 
See golang/go#23709
@ncw ncw mentioned this issue Jul 3, 2018
Merged
@bradfitz
Copy link
Contributor

bradfitz commented Jul 6, 2018

@ncw, could you file a new bug and cc the author on it and reference this bug?

@ncw ncw mentioned this issue Jul 6, 2018
Closed
@ncw
Copy link
Contributor

ncw commented Jul 6, 2018

@bradfitz I've made a new issue here #26246

@gopherbot
Copy link

Change https://golang.org/cl/122538 mentions this issue: imports: fix fixup comment insert extra blank line

gopherbot pushed a commit to golang/tools that referenced this issue Jul 9, 2018
@cuonglm @bradfitz
imports: fix unexpected blank line after package comment
d600f31 
The fix in golang/go#23709 introduced a separate bug where extra blank
lines were sometimes inserted. This fixes that newly introduced bug.

Fixes golang/go#26246

Change-Id: I78131cc1d01ae246922ed9e4336ebb31d1c6cfa1
Reviewed-on: https://go-review.googlesource.com/122538
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@golang golang locked and limited conversation to collaborators Jul 9, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
7 participants
@bradfitz @ncw @ianlancetaylor @cuonglm @lukelafountaine @gopherbot @dfawley