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

Performance: Use FormatOnly on imports #277

Merged
merged 3 commits into from
Sep 3, 2024
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion v2/generator/execute.go
Original file line number Diff line number Diff line change
@@ -26,6 +26,7 @@ import (
"strings"

"golang.org/x/tools/imports"

"k8s.io/gengo/v2/namer"
"k8s.io/gengo/v2/types"
"k8s.io/klog/v2"
@@ -114,7 +115,13 @@ func assembleGoFile(w io.Writer, f *File) {
}

func importsWrapper(src []byte) ([]byte, error) {
return imports.Process("", src, nil)
opt := imports.Options{
Comments: true,
TabIndent: true,
TabWidth: 8,
FormatOnly: true, // Disable the insertion and deletion of imports
}
return imports.Process("", src, &opt)
}

func NewGoFile() *DefaultFileType {
15 changes: 11 additions & 4 deletions v2/generator/import_tracker.go
Original file line number Diff line number Diff line change
@@ -18,6 +18,7 @@ package generator

import (
"go/token"
"path/filepath"
"strings"

"k8s.io/klog/v2"
@@ -45,7 +46,7 @@ import (
func NewImportTrackerForPackage(local string, typesToAdd ...*types.Type) *namer.DefaultImportTracker {
tracker := namer.NewDefaultImportTracker(types.Name{Package: local})
tracker.IsInvalidType = func(*types.Type) bool { return false }
tracker.LocalName = func(name types.Name) string { return goTrackerLocalName(&tracker, name) }
tracker.LocalName = func(name types.Name) string { return goTrackerLocalName(&tracker, local, name) }
tracker.PrintImport = func(path, name string) string { return name + " \"" + path + "\"" }

tracker.AddTypes(typesToAdd...)
@@ -56,14 +57,15 @@ func NewImportTracker(typesToAdd ...*types.Type) *namer.DefaultImportTracker {
return NewImportTrackerForPackage("", typesToAdd...)
}

func goTrackerLocalName(tracker namer.ImportTracker, t types.Name) string {
func goTrackerLocalName(tracker namer.ImportTracker, localPkg string, t types.Name) string {
path := t.Package

// Using backslashes in package names causes gengo to produce Go code which
// will not compile with the gc compiler. See the comment on GoSeperator.
if strings.ContainsRune(path, '\\') {
klog.Warningf("Warning: backslash used in import path '%v', this is unsupported.\n", path)
}
localLeaf := filepath.Base(localPkg)

dirs := strings.Split(path, namer.GoSeperator)
for n := len(dirs) - 1; n >= 0; n-- {
@@ -74,8 +76,13 @@ func goTrackerLocalName(tracker namer.ImportTracker, t types.Name) string {
// packages, but aren't legal go names. So we'll sanitize.
name = strings.ReplaceAll(name, ".", "")
name = strings.ReplaceAll(name, "-", "")
if _, found := tracker.PathOf(name); found {
// This name collides with some other package
if _, found := tracker.PathOf(name); found || name == localLeaf {
// This name collides with some other package.
// Or, this name is tne same name as the local package,
// which we avoid because it can be confusing. For example,
// if the local package is v1, we to avoid importing
Comment on lines +81 to +83
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very minor:

Suggested change
// Or, this name is tne same name as the local package,
// which we avoid because it can be confusing. For example,
// if the local package is v1, we to avoid importing
// Or, this name is the same name as the local package,
// which we avoid because it can be confusing. For example,
// if the local package is v1, we try to avoid importing

// another package using the v1 name, and instead import
// it with a more qualified name, such as metav1.
continue
}

2 changes: 1 addition & 1 deletion v2/generator/import_tracker_test.go
Original file line number Diff line number Diff line change
@@ -72,7 +72,7 @@ func TestNewImportTracker(t *testing.T) {
{Name: types.Name{Package: "bar.com/external/pkg"}},
},
expectedImports: []string{
`pkg "bar.com/external/pkg"`,
`externalpkg "bar.com/external/pkg"`,
},
},
}