Enable linter goimports to tidy imports #1037
Conversation
goimports makes imports deterministic, putting them in 3 sections: builtin, 3rd-party, local packages. It saves reviewers's effort to comment the order of imports. It doesn't allow imports named "v1" unless the alias name is specified as "v1", this patch adds the parent packages of v1 packages as the prefix of the import names.
|
Thanks for your PR. The following commands are available:
These commands can only be run by members of the vmware-tanzu organization. |
|
/test-all |
|
/test-windows-conformance |
2 similar comments
|
/test-windows-conformance |
|
/test-windows-conformance |
There was a problem hiding this comment.
Thanks for the change @tnqn. Definitely a time saver. Just a couple of questions.
| @@ -14,3 +18,4 @@ linters: | |||
| - deadcode | |||
| - staticcheck | |||
| - gosec | |||
| - goimports | |||
There was a problem hiding this comment.
Do we still need to enable gofmt as goimports also formats the file?
Following is from https://godoc.org/golang.org/x/tools/imports:
func Process
func Process(filename string, src []byte, opt *Options) ([]byte, error)
Process formats and adjusts imports for the provided file. If opt is nil the defaults are used, and if src is nil the source is read from the filesystem.
There was a problem hiding this comment.
I do agree that goimports does everything gofmt does. That being said, I don't think there is much harm in keeping it in the list. Makes it explicit that we are running this linters, even if it means we run it twice. Or we can remove it but add a comment to the yml file to indicate that goimports supersedes gofmt.
There was a problem hiding this comment.
@srikartati thanks for pointing it out, I didn't know goimports supersedes gofmt. I also think there is no much harm to keep gofmt explicitly as it's more well known and widely adopted?
There was a problem hiding this comment.
Sure.. format can be done two times. I was looking at the API and found out about it--thought I will check with you.
| @@ -21,7 +21,7 @@ import ( | |||
| "time" | |||
|
|
|||
| "github.com/containernetworking/plugins/pkg/ip" | |||
| "k8s.io/api/core/v1" | |||
| corev1 "k8s.io/api/core/v1" | |||
There was a problem hiding this comment.
Out of curiosity.. is there a rule to determine the package name as prevDirectory+currentDirectory.
There was a problem hiding this comment.
Adding this because goimports think "v1" is not a good import name because of lack of package information which I also agree. But the way it "fixes" it is adding "v1" as the import alias.
v1 "k8s.io/api/core/v1"
I'm not sure if this is a bug or a workaround that makes goimports ignore it. But I feel having a prefix can improve the readability as well as in compliance with goimports.
Both "corev1", "v1" are used in kubernetes:
https://github.com/kubernetes/kubernetes/blob/master/pkg/master/controller.go#L26
https://github.com/kubernetes/kubernetes/blob/master/pkg/kubelet/kubelet.go#L42
There was a problem hiding this comment.
Thanks for the explanation. corev1 sounds good.
| @@ -21,7 +21,7 @@ import ( | |||
| "time" | |||
|
|
|||
| "github.com/containernetworking/plugins/pkg/ip" | |||
| "k8s.io/api/core/v1" | |||
| corev1 "k8s.io/api/core/v1" | |||
There was a problem hiding this comment.
Thanks for the explanation. corev1 sounds good.
| @@ -14,3 +18,4 @@ linters: | |||
| - deadcode | |||
| - staticcheck | |||
| - gosec | |||
| - goimports | |||
There was a problem hiding this comment.
Sure.. format can be done two times. I was looking at the API and found out about it--thought I will check with you.
goimports makes imports deterministic, putting them in 3 sections: builtin, 3rd-party, local packages. It saves reviewers's effort to comment the order of imports. It doesn't allow imports named "v1" unless the alias name is specified as "v1", this patch adds the parent packages of v1 packages as the prefix of the import names.
goimports makes imports deterministic, putting them in 3 sections:
builtin, 3rd-party, local packages. It saves reviewers's effort to
comment the order of imports.
It doesn't allow imports named "v1" unless the alias name is specified
as "v1", this patch adds the parent packages of v1 packages as the
prefix of the import names.
Fixes #1038