Skip to content

Commit

Permalink
Add basic attempt at local module handling (#179)
Browse files Browse the repository at this point in the history
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. This could be improved, if we know the name of the file whose
imports we're processing we could check if that file is under the
directory of a given local module (possible improvement for the future).

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>
  • Loading branch information
matthewhughes934 committed Feb 27, 2024
1 parent 3e4f5ad commit ad52d54
Show file tree
Hide file tree
Showing 17 changed files with 305 additions and 13 deletions.
42 changes: 40 additions & 2 deletions README.md
Expand Up @@ -27,16 +27,17 @@ 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)
- default: All rest import blocks
- 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.
Expand All @@ -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 -
Expand Down Expand Up @@ -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
Expand Down
28 changes: 22 additions & 6 deletions pkg/config/config.go
Expand Up @@ -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 {
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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
}
73 changes: 73 additions & 0 deletions pkg/gci/gci_test.go
Expand Up @@ -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"
)

Expand Down Expand Up @@ -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)
}
23 changes: 23 additions & 0 deletions pkg/gci/testdata.go
Expand Up @@ -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"
)
`,
},
}
4 changes: 4 additions & 0 deletions 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
4 changes: 4 additions & 0 deletions pkg/gci/testdata/module/config.yaml
@@ -0,0 +1,4 @@
sections:
- Standard
- Default
- LocalModule
3 changes: 3 additions & 0 deletions pkg/gci/testdata/module/go.mod
@@ -0,0 +1,3 @@
module example.com/simple

go 1.20
1 change: 1 addition & 0 deletions pkg/gci/testdata/module/internal/bar/lib.go
@@ -0,0 +1 @@
package bar
6 changes: 6 additions & 0 deletions pkg/gci/testdata/module/internal/foo/lib.go
@@ -0,0 +1,6 @@
package foo

import (
"example.com/simple/internal/bar"
"log"
)
7 changes: 7 additions & 0 deletions pkg/gci/testdata/module/internal/foo/lib.out.go
@@ -0,0 +1,7 @@
package foo

import (
"log"

"example.com/simple/internal/bar"
)
1 change: 1 addition & 0 deletions pkg/gci/testdata/module/internal/lib.go
@@ -0,0 +1 @@
package internal
9 changes: 9 additions & 0 deletions 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"
)
11 changes: 11 additions & 0 deletions 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"
)
77 changes: 77 additions & 0 deletions 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
}
3 changes: 3 additions & 0 deletions pkg/section/parser.go
Expand Up @@ -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)
}
Expand Down
15 changes: 15 additions & 0 deletions 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
}

0 comments on commit ad52d54

Please sign in to comment.