Repository: daixiang0/gci
Pull request #179 (2023-10-28 16:22:26)

Add basic attempt at local module handling
The idea here is to support a new formatting section, `module`, which is
the module we're currently running in as a replacement for
`prefix(module/we/are/running/in)`.

Since this is dependent on the directory structure and where things are
run, some tests have been added which run on a real module structure.

This approach just focusses on the use-case of running `gci` from
the top level of a module. It does not, for example support:

* Discovering modules with a nested structure
* Discovering modules defined in a `go.work` file

It was considered to get the module from `go list -m`, but this would
drag in all modules defined under the current directory (i.e. those in
`go.work`) which might be confusing if those modules are importing each
other.

Another alternative to consider would be using query patterns with
`golang.org/x/tools/go/packages` e.g.

    config := packages.Config{Mode: packages.NeedModule | packages.NeedFiles | packages.NeedName}
    // where each "file=" maps to a file given as an argument to gci
    packages, err := packages.Load(
        &config,
        "file=./path/to/file.go",
        "file=./path/to/other.go",
    )

Signed-off-by: Matthew Hughes <matthewhughes934@gmail.com>

matthewhughes934: Some thoughts:

* Maybe we can move the module loading into Configuration parse time, rather than just after, this avoids setting then mutating configuration state
* Module finding should just search up from the current working directory, though I'm not sure how this would work if there are more modules defined below the current path
daixiang0: Focus on the main module is enough, sub project should be store in other places, one project one module is more common.

Also please fix DCO.
daixiang0: > * Maybe we can move the module loading into Configuration parse time, rather than just after, this avoids setting then mutating configuration state

Please do it then pass the module name to config parse.



matthewhughes934: > > * Maybe we can move the module loading into Configuration parse time, rather than just after, this avoids setting then mutating configuration state
> 
> Please do it then pass the module name to config parse.

I gave this a shot (see latest changes). There's a bit of a disconnect where `Module.Populate` _could_ take a list of files but we'd need to parse them down from `Config.Parse` and I'm not sure how nice that would be.
matthewhughes934: @daixiang0 I've settled on an approach and cleaned everything up, so marking this as ready for review :slightly_smiling_face: 
matthewhughes934: I've added https://github.com/daixiang0/gci/pull/179/commits/3521f2dc4dd31ea324eab9ceea74a543770ae0e6 to try and address the test failures on Windows. I don't have a Windows machine at the moment, but I should later this week
matthewhughes934: In the latest changed I've renamed things from just `module` to `localModule` because this felt a bit clearer
delatorrejuanchi: It'd be nice to get this merged 🙏 
daixiang0: Please sign your commits, check this [doc](https://docs.github.com/articles/about-gpg/)
matthewhughes934: > Please sign your commits, check this [doc](https://docs.github.com/articles/about-gpg/)

Sorry, I didn't realise this was necessary. My latest commit (rebasing on top of `master`) has been signed
snicol: @daixiang0 I am really sorry for chasing, but is it possible to take a look at merging now commits are signed? Quite a few of my team are eagerly awaiting this release 🙌
daixiang0: ping @matthewhughes934 please fix conflicts. I have no access to update it.
matthewhughes934: > ping @matthewhughes934 please fix conflicts.

Done with [65217e1](https://github.com/daixiang0/gci/pull/179/commits/65217e1c3ffa2bffb8f53e7917f3ca5b754705a6)

Patch:
diff --git a/README.md b/README.md
index 9d9a04d..0f37fa5 100644
--- a/README.md
+++ b/README.md
@@ -27,7 +27,7 @@ import (
 )
 ```
 
-GCI splits all import blocks into different sections, now support five section type:
+GCI splits all import blocks into different sections, now support six section type:
 
 - standard: Go official imports, like "fmt"
 - custom: Custom section, use full and the longest match (match full string first, if multiple matches, use the longest one)
@@ -35,8 +35,9 @@ GCI splits all import blocks into different sections, now support five section t
 - blank: Put blank imports together in a separate group
 - dot: Put dot imports together in a separate group
 - alias: Put alias imports together in a separate group
+- localmodule: Put imports from local packages in a separate group
 
-The priority is standard > default > custom > blank > dot > alias, all sections sort alphabetically inside.
+The priority is standard > default > custom > blank > dot > alias > localmodule, all sections sort alphabetically inside.
 By default, blank , dot and alias sections are not used and the corresponding lines end up in the other groups.
 
 All import blocks use one TAB(`\t`) as Indent.
@@ -47,6 +48,17 @@ Since v0.9.0, GCI always puts C import block as the first.
 
 `nolint` is hard to handle at section level, GCI will consider it as a single comment.
 
+### LocalModule
+
+Local module detection is done via listing packages from *the directory where
+`gci` is invoked* and reading the modules off these. This means:
+
+  - This mode works when `gci` is invoked from a module root (i.e. directory
+    containing `go.mod`)
+  - This mode doesn't work with a multi-module setup, i.e. when `gci` is invoked
+    from a directory containing `go.work` (though it would work if invoked from
+    within one of the modules in the workspace)
+
 ## Installation
 
 To download and install the highest available release version -
@@ -321,6 +333,32 @@ import (
 )
 ```
 
+### with localmodule grouping enabled
+
+Assuming this is run on the root of this repo (i.e. where
+`github.com/daixiang0/gci` is a local module)
+
+```go
+package main
+
+import (
+	"os"
+	"github.com/daixiang0/gci/cmd/gci"
+)
+```
+
+to
+
+```go
+package main
+
+import (
+	"os"
+
+	"github.com/daixiang0/gci/cmd/gci"
+)
+```
+
 ## TODO
 
 - Ensure only one blank between `Name` and `Path` in an import block
diff --git a/pkg/config/config.go b/pkg/config/config.go
index 51f6ccf..18b9c74 100644
--- a/pkg/config/config.go
+++ b/pkg/config/config.go
@@ -10,12 +10,13 @@ import (
 )
 
 var defaultOrder = map[string]int{
-	section.StandardType: 0,
-	section.DefaultType:  1,
-	section.CustomType:   2,
-	section.BlankType:    3,
-	section.DotType:      4,
-	section.AliasType:    5,
+	section.StandardType:    0,
+	section.DefaultType:     1,
+	section.CustomType:      2,
+	section.BlankType:       3,
+	section.DotType:         4,
+	section.AliasType:       5,
+	section.LocalModuleType: 6,
 }
 
 type BoolConfig struct {
@@ -49,6 +50,9 @@ func (g YamlConfig) Parse() (*Config, error) {
 	if sections == nil {
 		sections = section.DefaultSections()
 	}
+	if err := configureSections(sections); err != nil {
+		return nil, err
+	}
 
 	// if default order sorted sections
 	if !g.Cfg.CustomOrder {
@@ -88,3 +92,15 @@ func ParseConfig(in string) (*Config, error) {
 
 	return gciCfg, nil
 }
+
+func configureSections(sections section.SectionList) error {
+	for _, sec := range sections {
+		switch s := sec.(type) {
+		case *section.LocalModule:
+			if err := s.Configure(); err != nil {
+				return err
+			}
+		}
+	}
+	return nil
+}
diff --git a/pkg/section/local_module.go b/pkg/section/local_module.go
new file mode 100644
index 0000000..4e1edcb
--- /dev/null
+++ b/pkg/section/local_module.go
@@ -0,0 +1,77 @@
+package section
+
+import (
+	"fmt"
+	"strings"
+
+	"golang.org/x/tools/go/packages"
+
+	"github.com/daixiang0/gci/pkg/parse"
+	"github.com/daixiang0/gci/pkg/specificity"
+)
+
+const LocalModuleType = "localmodule"
+
+type LocalModule struct {
+	Paths []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{}
+		}
+	}
+
+	return specificity.MisMatch{}
+}
+
+func (m *LocalModule) String() string {
+	return LocalModuleType
+}
+
+func (m *LocalModule) Type() string {
+	return LocalModuleType
+}
+
+// Configure configures the module section by finding the module
+// for the current path
+func (m *LocalModule) Configure() error {
+	modPaths, err := findLocalModules()
+	if err != nil {
+		return err
+	}
+	m.Paths = modPaths
+	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},
+		".",
+	)
+	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 modPaths, nil
+}
diff --git a/pkg/section/parser.go b/pkg/section/parser.go
index 38435f5..62ed158 100644
--- a/pkg/section/parser.go
+++ b/pkg/section/parser.go
@@ -35,6 +35,9 @@ func Parse(data []string) (SectionList, error) {
 			list = append(list, Blank{})
 		} else if s == "alias" {
 			list = append(list, Alias{})
+		} else if s == "localmodule" {
+			// pointer because we need to mutate the section at configuration time
+			list = append(list, &LocalModule{})
 		} else {
 			errString += fmt.Sprintf(" %s", s)
 		}
diff --git a/pkg/specificity/local_module.go b/pkg/specificity/local_module.go
new file mode 100644
index 0000000..ae482fe
--- /dev/null
+++ b/pkg/specificity/local_module.go
@@ -0,0 +1,15 @@
+package specificity
+
+type LocalModule struct{}
+
+func (m LocalModule) IsMoreSpecific(than MatchSpecificity) bool {
+	return isMoreSpecific(m, than)
+}
+
+func (m LocalModule) Equal(to MatchSpecificity) bool {
+	return equalSpecificity(m, to)
+}
+
+func (LocalModule) class() specificityClass {
+	return LocalModuleClass
+}
diff --git a/pkg/specificity/specificity.go b/pkg/specificity/specificity.go
index 842da18..4a188b3 100644
--- a/pkg/specificity/specificity.go
+++ b/pkg/specificity/specificity.go
@@ -3,11 +3,12 @@ package specificity
 type specificityClass int
 
 const (
-	MisMatchClass = 0
-	DefaultClass  = 10
-	StandardClass = 20
-	MatchClass    = 30
-	NameClass     = 40
+	MisMatchClass    = 0
+	DefaultClass     = 10
+	StandardClass    = 20
+	MatchClass       = 30
+	NameClass        = 40
+	LocalModuleClass = 50
 )
 
 // MatchSpecificity is used to determine which section matches an import best


Test patch:
diff --git a/pkg/gci/gci_test.go b/pkg/gci/gci_test.go
index 86b9510..1ba9d30 100644
--- a/pkg/gci/gci_test.go
+++ b/pkg/gci/gci_test.go
@@ -2,11 +2,16 @@ package gci
 
 import (
 	"fmt"
+	"os"
+	"path/filepath"
+	"strings"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 
 	"github.com/daixiang0/gci/pkg/config"
+	"github.com/daixiang0/gci/pkg/io"
 	"github.com/daixiang0/gci/pkg/log"
 )
 
@@ -38,3 +43,71 @@ func TestRun(t *testing.T) {
 		})
 	}
 }
+
+func chdir(t *testing.T, dir string) {
+	oldWd, err := os.Getwd()
+	require.NoError(t, err)
+	require.NoError(t, os.Chdir(dir))
+
+	// change back at the end of the test
+	t.Cleanup(func() { os.Chdir(oldWd) })
+}
+
+func readConfig(t *testing.T, configPath string) *config.Config {
+	rawConfig, err := os.ReadFile(configPath)
+	require.NoError(t, err)
+	config, err := config.ParseConfig(string(rawConfig))
+	require.NoError(t, err)
+
+	return config
+}
+
+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"),
+	}
+
+	// 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))
+		})
+	}
+}
+
+func TestRunWithLocalModuleWithPackageLoadFailure(t *testing.T) {
+	// just a directory with no Go modules
+	dir := t.TempDir()
+	configContent := "sections:\n  - LocalModule\n"
+
+	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)
+}
diff --git a/pkg/gci/testdata.go b/pkg/gci/testdata.go
index 4f60e88..9723981 100644
--- a/pkg/gci/testdata.go
+++ b/pkg/gci/testdata.go
@@ -1293,6 +1293,29 @@ 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/.gitattributes b/pkg/gci/testdata/module/.gitattributes
new file mode 100644
index 0000000..b63e502
--- /dev/null
+++ b/pkg/gci/testdata/module/.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/config.yaml b/pkg/gci/testdata/module/config.yaml
new file mode 100644
index 0000000..5ae453b
--- /dev/null
+++ b/pkg/gci/testdata/module/config.yaml
@@ -0,0 +1,4 @@
+sections:
+  - Standard
+  - Default
+  - LocalModule
diff --git a/pkg/gci/testdata/module/go.mod b/pkg/gci/testdata/module/go.mod
new file mode 100644
index 0000000..8531479
--- /dev/null
+++ b/pkg/gci/testdata/module/go.mod
@@ -0,0 +1,3 @@
+module example.com/simple
+
+go 1.20
diff --git a/pkg/gci/testdata/module/internal/bar/lib.go b/pkg/gci/testdata/module/internal/bar/lib.go
new file mode 100644
index 0000000..ddac0fa
--- /dev/null
+++ b/pkg/gci/testdata/module/internal/bar/lib.go
@@ -0,0 +1,1 @@
+package bar
diff --git a/pkg/gci/testdata/module/internal/foo/lib.go b/pkg/gci/testdata/module/internal/foo/lib.go
new file mode 100644
index 0000000..099f27e
--- /dev/null
+++ b/pkg/gci/testdata/module/internal/foo/lib.go
@@ -0,0 +1,6 @@
+package foo
+
+import (
+	"example.com/simple/internal/bar"
+	"log"
+)
diff --git a/pkg/gci/testdata/module/internal/foo/lib.out.go b/pkg/gci/testdata/module/internal/foo/lib.out.go
new file mode 100644
index 0000000..f132d6b
--- /dev/null
+++ b/pkg/gci/testdata/module/internal/foo/lib.out.go
@@ -0,0 +1,7 @@
+package foo
+
+import (
+	"log"
+
+	"example.com/simple/internal/bar"
+)
diff --git a/pkg/gci/testdata/module/internal/lib.go b/pkg/gci/testdata/module/internal/lib.go
new file mode 100644
index 0000000..5bf0569
--- /dev/null
+++ b/pkg/gci/testdata/module/internal/lib.go
@@ -0,0 +1,1 @@
+package internal
diff --git a/pkg/gci/testdata/module/main.go b/pkg/gci/testdata/module/main.go
new file mode 100644
index 0000000..14409f0
--- /dev/null
+++ b/pkg/gci/testdata/module/main.go
@@ -0,0 +1,9 @@
+package lib
+
+import (
+	"golang.org/x/net"
+	"example.com/simple/internal"
+	"example.com/simple/internal/foo"
+	"example.com/simple/internal/bar"
+	"log"
+)
diff --git a/pkg/gci/testdata/module/main.out.go b/pkg/gci/testdata/module/main.out.go
new file mode 100644
index 0000000..ad3a9d0
--- /dev/null
+++ b/pkg/gci/testdata/module/main.out.go
@@ -0,0 +1,11 @@
+package lib
+
+import (
+	"log"
+
+	"golang.org/x/net"
+
+	"example.com/simple/internal"
+	"example.com/simple/internal/bar"
+	"example.com/simple/internal/foo"
+)