Repository: daixiang0/gci
Pull request #192 (2024-03-02 11:26:06)

Enhance LocalModule when no go sources found in project root
👋 

Four days ago, a remarkable feature was added — LocalModule. It's an incredibly cool addition that has proven to be useful to many, including myself and my teams.

However, there might be instances where the Go source files are not located in the project's root folder. I've demonstrated what this could look like in a test.

Such an enhancement would be very beneficial. I'm eager to receive feedback from the community and, if possible, apply the suggested improvement.

daixiang0: Please fix DCO as well.
petr-korobeinikov: > Please fix DCO as well.

Signed.
daixiang0: Codes looks good to me.

In case the minimum reproducible example in #193 is not covered, please wait it and update tests if necessary, then we can make it land.
petr-korobeinikov: > Thanks for fixing this up, and nice work on the extra tests! Since making the change I started having doubts whether the package listing approach was best (:facepalm: I should've added more tests)

Don't worry about it. The changes resolved a real existing issue. It's difficult to get everything right on the first try, even with tests.
matthewhughes934: > In case the minimum reproducible example in https://github.com/daixiang0/gci/issues/193 is not covered, please wait it and update tests if necessary, then we can make it land.

I think one of the added test cases can be used to reproduce:

```console
$ git clone https://github.com/petr-korobeinikov/gci.git
$ cd gci/pkg/gci/testdata/module_canonical/
$ go install github.com/daixiang0/gci@v0.13.0 # make sure we run using the version with the bug
$ gci write -s standard -s default -s localmodule cmd/client/main.go 
Error: error reading local packages: [-: no Go files in /tmp/gci/pkg/gci/testdata/module_canonical]
Usage:
...
```
petr-korobeinikov: As of force push of windows error check the pipeline needs to be retriggered.
petr-korobeinikov: Looks like PR polished for now. Any suggestions in grammar/syntax on comments, variable naming? Should I squash all commits in one?
CarlosRDomin: Is there anything missing to get this merged?
daixiang0: Please fix DCO then we can make it land.

Patch:
diff --git a/README.md b/README.md
index 0f37fa5..b68248d 100644
--- a/README.md
+++ b/README.md
@@ -50,8 +50,8 @@ Since v0.9.0, GCI always puts C import block as the first.
 
 ### LocalModule
 
-Local module detection is done via listing packages from *the directory where
-`gci` is invoked* and reading the modules off these. This means:
+Local module detection is done via reading the module name from the `go.mod`
+file in *the directory where `gci` is invoked*. This means:
 
   - This mode works when `gci` is invoked from a module root (i.e. directory
     containing `go.mod`)
diff --git a/go.mod b/go.mod
index 68161f1..ea8cc8f 100644
--- a/go.mod
+++ b/go.mod
@@ -8,6 +8,7 @@ require (
 	github.com/spf13/cobra v1.6.1
 	github.com/stretchr/testify v1.8.1
 	go.uber.org/zap v1.24.0
+	golang.org/x/mod v0.8.0
 	golang.org/x/sync v0.1.0
 	golang.org/x/tools v0.6.0
 	gopkg.in/yaml.v3 v3.0.1
@@ -19,6 +20,5 @@ require (
 	github.com/spf13/pflag v1.0.5 // indirect
 	go.uber.org/atomic v1.7.0 // indirect
 	go.uber.org/multierr v1.6.0 // indirect
-	golang.org/x/mod v0.8.0 // indirect
 	golang.org/x/sys v0.5.0 // indirect
 )
diff --git a/pkg/section/local_module.go b/pkg/section/local_module.go
index 4e1edcb..07821fe 100644
--- a/pkg/section/local_module.go
+++ b/pkg/section/local_module.go
@@ -2,9 +2,10 @@ package section
 
 import (
 	"fmt"
+	"os"
 	"strings"
 
-	"golang.org/x/tools/go/packages"
+	"golang.org/x/mod/modfile"
 
 	"github.com/daixiang0/gci/pkg/parse"
 	"github.com/daixiang0/gci/pkg/specificity"
@@ -13,15 +14,12 @@ import (
 const LocalModuleType = "localmodule"
 
 type LocalModule struct {
-	Paths []string
+	Path string
 }
 
 func (m *LocalModule) MatchSpecificity(spec *parse.GciImports) specificity.MatchSpecificity {
-	for _, modPath := range m.Paths {
-		// also check path etc.
-		if strings.HasPrefix(spec.Path, modPath) {
-			return specificity.LocalModule{}
-		}
+	if strings.HasPrefix(spec.Path, m.Path) {
+		return specificity.LocalModule{}
 	}
 
 	return specificity.MisMatch{}
@@ -38,40 +36,21 @@ func (m *LocalModule) Type() string {
 // Configure configures the module section by finding the module
 // for the current path
 func (m *LocalModule) Configure() error {
-	modPaths, err := findLocalModules()
+	modPath, err := findLocalModule()
 	if err != nil {
-		return err
+		return fmt.Errorf("finding local modules for `localModule` configuration: %w", err)
 	}
-	m.Paths = modPaths
+
+	m.Path = modPath
+
 	return nil
 }
 
-func findLocalModules() ([]string, error) {
-	packages, err := packages.Load(
-		// find the package in the current dir and load its module
-		// NeedFiles so there is some more info in package errors
-		&packages.Config{Mode: packages.NeedModule | packages.NeedFiles},
-		".",
-	)
+func findLocalModule() (string, error) {
+	b, err := os.ReadFile("go.mod")
 	if err != nil {
-		return nil, fmt.Errorf("failed to load local modules: %v", err)
-	}
-
-	uniqueModules := make(map[string]struct{})
-
-	for _, pkg := range packages {
-		if len(pkg.Errors) != 0 {
-			return nil, fmt.Errorf("error reading local packages: %v", pkg.Errors)
-		}
-		if pkg.Module != nil {
-			uniqueModules[pkg.Module.Path] = struct{}{}
-		}
-	}
-
-	modPaths := make([]string, 0, len(uniqueModules))
-	for mod := range uniqueModules {
-		modPaths = append(modPaths, mod)
+		return "", fmt.Errorf("reading go.mod: %w", err)
 	}
 
-	return modPaths, nil
+	return modfile.ModulePath(b), nil
 }


Test patch:
diff --git a/pkg/gci/gci_test.go b/pkg/gci/gci_test.go
index 1ba9d30..f6db3e0 100644
--- a/pkg/gci/gci_test.go
+++ b/pkg/gci/gci_test.go
@@ -21,10 +21,6 @@ func init() {
 }
 
 func TestRun(t *testing.T) {
-	// if runtime.GOOS == "windows" {
-	// 	t.Skip("Skipping test on Windows")
-	// }
-
 	for i := range testCases {
 		t.Run(fmt.Sprintf("run case: %s", testCases[i].name), func(t *testing.T) {
 			config, err := config.ParseConfig(testCases[i].config)
@@ -56,35 +52,65 @@ func chdir(t *testing.T, dir string) {
 func readConfig(t *testing.T, configPath string) *config.Config {
 	rawConfig, err := os.ReadFile(configPath)
 	require.NoError(t, err)
-	config, err := config.ParseConfig(string(rawConfig))
+	cfg, err := config.ParseConfig(string(rawConfig))
 	require.NoError(t, err)
 
-	return config
+	return cfg
 }
 
 func TestRunWithLocalModule(t *testing.T) {
-	moduleDir := filepath.Join("testdata", "module")
-	// files with a corresponding '*.out.go' file containing the expected
-	// result of formatting
-	testedFiles := []string{
-		"main.go",
-		filepath.Join("internal", "foo", "lib.go"),
+	tests := []struct {
+		name      string
+		moduleDir string
+		// files with a corresponding '*.out.go' file containing the expected
+		// result of formatting
+		testedFiles []string
+	}{
+		{
+			name:      `default module test case`,
+			moduleDir: filepath.Join("testdata", "module"),
+			testedFiles: []string{
+				"main.go",
+				filepath.Join("internal", "foo", "lib.go"),
+			},
+		},
+		{
+			name:      `canonical module without go sources in root dir`,
+			moduleDir: filepath.Join("testdata", "module_canonical"),
+			testedFiles: []string{
+				filepath.Join("cmd", "client", "main.go"),
+				filepath.Join("cmd", "server", "main.go"),
+				filepath.Join("internal", "foo", "lib.go"),
+			},
+		},
+		{
+			name:      `non-canonical module without go sources in root dir`,
+			moduleDir: filepath.Join("testdata", "module_noncanonical"),
+			testedFiles: []string{
+				filepath.Join("cmd", "client", "main.go"),
+				filepath.Join("cmd", "server", "main.go"),
+				filepath.Join("internal", "foo", "lib.go"),
+			},
+		},
 	}
-
-	// run subtests for expected module loading behaviour
-	chdir(t, moduleDir)
-	cfg := readConfig(t, "config.yaml")
-
-	for _, path := range testedFiles {
-		t.Run(path, func(t *testing.T) {
-			// *.go -> *.out.go
-			expected, err := os.ReadFile(strings.TrimSuffix(path, ".go") + ".out.go")
-			require.NoError(t, err)
-
-			_, got, err := LoadFormatGoFile(io.File{path}, *cfg)
-
-			require.NoError(t, err)
-			require.Equal(t, string(expected), string(got))
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			// run subtests for expected module loading behaviour
+			chdir(t, tt.moduleDir)
+			cfg := readConfig(t, "config.yaml")
+
+			for _, path := range tt.testedFiles {
+				t.Run(path, func(t *testing.T) {
+					// *.go -> *.out.go
+					expected, err := os.ReadFile(strings.TrimSuffix(path, ".go") + ".out.go")
+					require.NoError(t, err)
+
+					_, got, err := LoadFormatGoFile(io.File{path}, *cfg)
+
+					require.NoError(t, err)
+					require.Equal(t, string(expected), string(got))
+				})
+			}
 		})
 	}
 }
@@ -96,18 +122,5 @@ func TestRunWithLocalModuleWithPackageLoadFailure(t *testing.T) {
 
 	chdir(t, dir)
 	_, err := config.ParseConfig(configContent)
-	require.ErrorContains(t, err, "failed to load local modules: ")
-}
-
-func TestRunWithLocalModuleWithModuleLookupError(t *testing.T) {
-	dir := t.TempDir()
-	// error from trying to list packages under module with no go files
-	configContent := "sections:\n  - LocalModule\n"
-	goModContent := "module example.com/foo\n"
-	require.NoError(t, os.WriteFile(filepath.Join(dir, "go.mod"), []byte(goModContent), 0o644))
-
-	chdir(t, dir)
-	_, err := config.ParseConfig(configContent)
-	require.ErrorContains(t, err, "error reading local packages: ")
-	require.ErrorContains(t, err, dir)
+	require.ErrorContains(t, err, "go.mod: open go.mod:")
 }
diff --git a/pkg/gci/testdata.go b/pkg/gci/testdata.go
index 9723981..4f60e88 100644
--- a/pkg/gci/testdata.go
+++ b/pkg/gci/testdata.go
@@ -1293,29 +1293,6 @@ import (
 	"fmt"
 	"net"
 )
-`,
-	},
-	{
-		"basic module",
-		// implicitly relies on the local module name being: github.com/daixiang0/gci
-		`sections:
-  - Standard
-  - LocalModule
-`,
-		`package main
-
-import (
-	"os"
-	"github.com/daixiang0/gci/cmd/gci"
-)
-`,
-		`package main
-
-import (
-	"os"
-
-	"github.com/daixiang0/gci/cmd/gci"
-)
 `,
 	},
 }
diff --git a/pkg/gci/testdata/module_canonical/.gitattributes b/pkg/gci/testdata/module_canonical/.gitattributes
new file mode 100644
index 0000000..b63e502
--- /dev/null
+++ b/pkg/gci/testdata/module_canonical/.gitattributes
@@ -0,0 +1,4 @@
+# try and stop git running on Windows from converting line endings from
+# in all Go files under this directory. This is to avoid inconsistent test
+# results when `gci` strips "\r" characters
+**/*.go eol=lf
diff --git a/pkg/gci/testdata/module_canonical/cmd/client/main.go b/pkg/gci/testdata/module_canonical/cmd/client/main.go
new file mode 100644
index 0000000..10c9826
--- /dev/null
+++ b/pkg/gci/testdata/module_canonical/cmd/client/main.go
@@ -0,0 +1,9 @@
+package main
+
+import (
+	"example.com/service/internal"
+	"example.com/service/internal/bar"
+	"example.com/service/internal/foo"
+	"golang.org/x/net"
+	"log"
+)
diff --git a/pkg/gci/testdata/module_canonical/cmd/client/main.out.go b/pkg/gci/testdata/module_canonical/cmd/client/main.out.go
new file mode 100644
index 0000000..52c3cef
--- /dev/null
+++ b/pkg/gci/testdata/module_canonical/cmd/client/main.out.go
@@ -0,0 +1,11 @@
+package main
+
+import (
+	"log"
+
+	"golang.org/x/net"
+
+	"example.com/service/internal"
+	"example.com/service/internal/bar"
+	"example.com/service/internal/foo"
+)
diff --git a/pkg/gci/testdata/module_canonical/cmd/server/main.go b/pkg/gci/testdata/module_canonical/cmd/server/main.go
new file mode 100644
index 0000000..10c9826
--- /dev/null
+++ b/pkg/gci/testdata/module_canonical/cmd/server/main.go
@@ -0,0 +1,9 @@
+package main
+
+import (
+	"example.com/service/internal"
+	"example.com/service/internal/bar"
+	"example.com/service/internal/foo"
+	"golang.org/x/net"
+	"log"
+)
diff --git a/pkg/gci/testdata/module_canonical/cmd/server/main.out.go b/pkg/gci/testdata/module_canonical/cmd/server/main.out.go
new file mode 100644
index 0000000..52c3cef
--- /dev/null
+++ b/pkg/gci/testdata/module_canonical/cmd/server/main.out.go
@@ -0,0 +1,11 @@
+package main
+
+import (
+	"log"
+
+	"golang.org/x/net"
+
+	"example.com/service/internal"
+	"example.com/service/internal/bar"
+	"example.com/service/internal/foo"
+)
diff --git a/pkg/gci/testdata/module_canonical/config.yaml b/pkg/gci/testdata/module_canonical/config.yaml
new file mode 100644
index 0000000..5ae453b
--- /dev/null
+++ b/pkg/gci/testdata/module_canonical/config.yaml
@@ -0,0 +1,4 @@
+sections:
+  - Standard
+  - Default
+  - LocalModule
diff --git a/pkg/gci/testdata/module_canonical/go.mod b/pkg/gci/testdata/module_canonical/go.mod
new file mode 100644
index 0000000..8299e9f
--- /dev/null
+++ b/pkg/gci/testdata/module_canonical/go.mod
@@ -0,0 +1,3 @@
+module example.com/service
+
+go 1.20
diff --git a/pkg/gci/testdata/module_canonical/internal/bar/lib.go b/pkg/gci/testdata/module_canonical/internal/bar/lib.go
new file mode 100644
index 0000000..ddac0fa
--- /dev/null
+++ b/pkg/gci/testdata/module_canonical/internal/bar/lib.go
@@ -0,0 +1,1 @@
+package bar
diff --git a/pkg/gci/testdata/module_canonical/internal/foo/lib.go b/pkg/gci/testdata/module_canonical/internal/foo/lib.go
new file mode 100644
index 0000000..07a3ead
--- /dev/null
+++ b/pkg/gci/testdata/module_canonical/internal/foo/lib.go
@@ -0,0 +1,6 @@
+package foo
+
+import (
+	"example.com/service/internal/bar"
+	"log"
+)
diff --git a/pkg/gci/testdata/module_canonical/internal/foo/lib.out.go b/pkg/gci/testdata/module_canonical/internal/foo/lib.out.go
new file mode 100644
index 0000000..de5b6f5
--- /dev/null
+++ b/pkg/gci/testdata/module_canonical/internal/foo/lib.out.go
@@ -0,0 +1,7 @@
+package foo
+
+import (
+	"log"
+
+	"example.com/service/internal/bar"
+)
diff --git a/pkg/gci/testdata/module_canonical/internal/lib.go b/pkg/gci/testdata/module_canonical/internal/lib.go
new file mode 100644
index 0000000..5bf0569
--- /dev/null
+++ b/pkg/gci/testdata/module_canonical/internal/lib.go
@@ -0,0 +1,1 @@
+package internal
diff --git a/pkg/gci/testdata/module_noncanonical/.gitattributes b/pkg/gci/testdata/module_noncanonical/.gitattributes
new file mode 100644
index 0000000..b63e502
--- /dev/null
+++ b/pkg/gci/testdata/module_noncanonical/.gitattributes
@@ -0,0 +1,4 @@
+# try and stop git running on Windows from converting line endings from
+# in all Go files under this directory. This is to avoid inconsistent test
+# results when `gci` strips "\r" characters
+**/*.go eol=lf
diff --git a/pkg/gci/testdata/module_noncanonical/cmd/client/main.go b/pkg/gci/testdata/module_noncanonical/cmd/client/main.go
new file mode 100644
index 0000000..aa41ef3
--- /dev/null
+++ b/pkg/gci/testdata/module_noncanonical/cmd/client/main.go
@@ -0,0 +1,9 @@
+package main
+
+import (
+	"golang.org/x/net"
+	"log"
+	"service/internal"
+	"service/internal/bar"
+	"service/internal/foo"
+)
diff --git a/pkg/gci/testdata/module_noncanonical/cmd/client/main.out.go b/pkg/gci/testdata/module_noncanonical/cmd/client/main.out.go
new file mode 100644
index 0000000..96d4449
--- /dev/null
+++ b/pkg/gci/testdata/module_noncanonical/cmd/client/main.out.go
@@ -0,0 +1,11 @@
+package main
+
+import (
+	"log"
+
+	"golang.org/x/net"
+
+	"service/internal"
+	"service/internal/bar"
+	"service/internal/foo"
+)
diff --git a/pkg/gci/testdata/module_noncanonical/cmd/server/main.go b/pkg/gci/testdata/module_noncanonical/cmd/server/main.go
new file mode 100644
index 0000000..aa41ef3
--- /dev/null
+++ b/pkg/gci/testdata/module_noncanonical/cmd/server/main.go
@@ -0,0 +1,9 @@
+package main
+
+import (
+	"golang.org/x/net"
+	"log"
+	"service/internal"
+	"service/internal/bar"
+	"service/internal/foo"
+)
diff --git a/pkg/gci/testdata/module_noncanonical/cmd/server/main.out.go b/pkg/gci/testdata/module_noncanonical/cmd/server/main.out.go
new file mode 100644
index 0000000..96d4449
--- /dev/null
+++ b/pkg/gci/testdata/module_noncanonical/cmd/server/main.out.go
@@ -0,0 +1,11 @@
+package main
+
+import (
+	"log"
+
+	"golang.org/x/net"
+
+	"service/internal"
+	"service/internal/bar"
+	"service/internal/foo"
+)
diff --git a/pkg/gci/testdata/module_noncanonical/config.yaml b/pkg/gci/testdata/module_noncanonical/config.yaml
new file mode 100644
index 0000000..5ae453b
--- /dev/null
+++ b/pkg/gci/testdata/module_noncanonical/config.yaml
@@ -0,0 +1,4 @@
+sections:
+  - Standard
+  - Default
+  - LocalModule
diff --git a/pkg/gci/testdata/module_noncanonical/go.mod b/pkg/gci/testdata/module_noncanonical/go.mod
new file mode 100644
index 0000000..7deade2
--- /dev/null
+++ b/pkg/gci/testdata/module_noncanonical/go.mod
@@ -0,0 +1,3 @@
+module service
+
+go 1.20
diff --git a/pkg/gci/testdata/module_noncanonical/internal/bar/lib.go b/pkg/gci/testdata/module_noncanonical/internal/bar/lib.go
new file mode 100644
index 0000000..ddac0fa
--- /dev/null
+++ b/pkg/gci/testdata/module_noncanonical/internal/bar/lib.go
@@ -0,0 +1,1 @@
+package bar
diff --git a/pkg/gci/testdata/module_noncanonical/internal/foo/lib.go b/pkg/gci/testdata/module_noncanonical/internal/foo/lib.go
new file mode 100644
index 0000000..a56f1c2
--- /dev/null
+++ b/pkg/gci/testdata/module_noncanonical/internal/foo/lib.go
@@ -0,0 +1,6 @@
+package foo
+
+import (
+	"log"
+	"service/internal/bar"
+)
diff --git a/pkg/gci/testdata/module_noncanonical/internal/foo/lib.out.go b/pkg/gci/testdata/module_noncanonical/internal/foo/lib.out.go
new file mode 100644
index 0000000..e4282eb
--- /dev/null
+++ b/pkg/gci/testdata/module_noncanonical/internal/foo/lib.out.go
@@ -0,0 +1,7 @@
+package foo
+
+import (
+	"log"
+
+	"service/internal/bar"
+)
diff --git a/pkg/gci/testdata/module_noncanonical/internal/lib.go b/pkg/gci/testdata/module_noncanonical/internal/lib.go
new file mode 100644
index 0000000..5bf0569
--- /dev/null
+++ b/pkg/gci/testdata/module_noncanonical/internal/lib.go
@@ -0,0 +1,1 @@
+package internal