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

gci: fix issues and re-enable autofix #2892

Merged
merged 6 commits into from May 30, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 4 additions & 1 deletion go.mod
Expand Up @@ -20,7 +20,7 @@ require (
github.com/breml/errchkjson v0.3.0
github.com/butuzov/ireturn v0.1.1
github.com/charithe/durationcheck v0.0.9
github.com/daixiang0/gci v0.3.3
github.com/daixiang0/gci v0.3.4
github.com/denis-tingaikin/go-header v0.4.3
github.com/esimonov/ifshort v1.0.4
github.com/fatih/color v1.13.0
Expand Down Expand Up @@ -164,6 +164,9 @@ require (
github.com/tklauser/numcpus v0.4.0 // indirect
github.com/valyala/bytebufferpool v1.0.0 // indirect
github.com/yusufpapurcu/wmi v1.2.2 // indirect
go.uber.org/atomic v1.7.0 // indirect
go.uber.org/multierr v1.6.0 // indirect
go.uber.org/zap v1.17.0 // indirect
golang.org/x/exp/typeparams v0.0.0-20220218215828-6cf2b201936e // indirect
golang.org/x/mod v0.6.0-dev.0.20220419223038-86c51ed26bb4 // indirect
golang.org/x/sync v0.0.0-20210220032951-036812b2e83c // indirect
Expand Down
7 changes: 5 additions & 2 deletions go.sum

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

121 changes: 106 additions & 15 deletions pkg/golinters/gci.go
Expand Up @@ -3,8 +3,11 @@ package golinters
import (
"fmt"
"strings"
"sync"

gci "github.com/daixiang0/gci/pkg/analyzer"
gcicfg "github.com/daixiang0/gci/pkg/configuration"
"github.com/daixiang0/gci/pkg/gci"
"github.com/pkg/errors"
"golang.org/x/tools/go/analysis"

"github.com/golangci/golangci-lint/pkg/config"
Expand All @@ -15,33 +18,121 @@ import (
const gciName = "gci"

func NewGci(settings *config.GciSettings) *goanalysis.Linter {
var linterCfg map[string]map[string]interface{}
var mu sync.Mutex
var resIssues []goanalysis.Issue

analyzer := &analysis.Analyzer{
Name: gciName,
Doc: goanalysis.TheOnlyanalyzerDoc,
Run: goanalysis.DummyRun,
}

var cfg *gci.GciConfiguration
if settings != nil {
cfg := map[string]interface{}{
gci.NoInlineCommentsFlag: settings.NoInlineComments,
gci.NoPrefixCommentsFlag: settings.NoPrefixComments,
gci.SectionsFlag: strings.Join(settings.Sections, gci.SectionDelimiter),
gci.SectionSeparatorsFlag: strings.Join(settings.SectionSeparator, gci.SectionDelimiter),
rawCfg := gci.GciStringConfiguration{
Cfg: gcicfg.FormatterConfiguration{
NoInlineComments: settings.NoInlineComments,
NoPrefixComments: settings.NoPrefixComments,
},
SectionStrings: settings.Sections,
SectionSeparatorStrings: settings.SectionSeparator,
}

if settings.LocalPrefixes != "" {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I want to remove LocalPrefixes here since I do not support that anymore in golangci-lint. @ldez

Copy link
Member

@ldez ldez May 30, 2022

Choose a reason for hiding this comment

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

inside golangci-lint we don't want to break the configuration

prefix := []string{"standard", "default", fmt.Sprintf("prefix(%s)", settings.LocalPrefixes)}
cfg[gci.SectionsFlag] = strings.Join(prefix, gci.SectionDelimiter)
rawCfg.SectionStrings = prefix
}

linterCfg = map[string]map[string]interface{}{
gci.Analyzer.Name: cfg,
}
cfg, _ = rawCfg.Parse()
}

var lock sync.Mutex

return goanalysis.NewLinter(
gciName,
"Gci controls golang package import order and makes it always deterministic.",
[]*analysis.Analyzer{gci.Analyzer},
linterCfg,
[]*analysis.Analyzer{analyzer},
nil,
).WithContextSetter(func(lintCtx *linter.Context) {
if settings.LocalPrefixes != "" {
lintCtx.Log.Warnf("gci: `local-prefixes` is deprecated, use `sections` and `prefix(%s)` instead.", settings.LocalPrefixes)
analyzer.Run = func(pass *analysis.Pass) (interface{}, error) {
issues, err := runGci(pass, lintCtx, cfg, &lock)
if err != nil {
return nil, err
}

if len(issues) == 0 {
return nil, nil
}

mu.Lock()
resIssues = append(resIssues, issues...)
mu.Unlock()

return nil, nil
}
}).WithIssuesReporter(func(*linter.Context) []goanalysis.Issue {
return resIssues
}).WithLoadMode(goanalysis.LoadModeSyntax)
}

func runGci(pass *analysis.Pass, lintCtx *linter.Context, cfg *gci.GciConfiguration, lock *sync.Mutex) ([]goanalysis.Issue, error) {
var fileNames []string
for _, f := range pass.Files {
pos := pass.Fset.PositionFor(f.Pos(), false)
fileNames = append(fileNames, pos.Filename)
}

var diffs []string
err := gci.DiffFormattedFilesToArray(fileNames, *cfg, &diffs, lock)
if err != nil {
return nil, err
}

var issues []goanalysis.Issue

for _, diff := range diffs {
if diff == "" {
continue
}

is, err := extractIssuesFromPatch(diff, lintCtx, gciName)
if err != nil {
return nil, errors.Wrapf(err, "can't extract issues from gci diff output %s", diff)
}

for i := range is {
issues = append(issues, goanalysis.NewIssue(&is[i], pass))
}
}

return issues, nil
}

func getErrorTextForGci(settings config.GciSettings) string {
text := "File is not `gci`-ed"

hasOptions := settings.NoInlineComments || settings.NoPrefixComments || len(settings.Sections) > 0 || len(settings.SectionSeparator) > 0
if !hasOptions {
return text
}

text += " with"

if settings.NoInlineComments {
text += " -NoInlineComments"
}

if settings.NoPrefixComments {
text += " -NoPrefixComments"
}

if len(settings.Sections) > 0 {
text += " -s " + strings.Join(settings.Sections, ",")
}

if len(settings.SectionSeparator) > 0 {
text += " -x " + strings.Join(settings.SectionSeparator, ",")
}

return text
}
19 changes: 10 additions & 9 deletions pkg/golinters/gofmt_common.go
Expand Up @@ -9,6 +9,7 @@ import (
"github.com/pkg/errors"
diffpkg "github.com/sourcegraph/go-diff/diff"

"github.com/golangci/golangci-lint/pkg/config"
"github.com/golangci/golangci-lint/pkg/lint/linter"
"github.com/golangci/golangci-lint/pkg/logutils"
"github.com/golangci/golangci-lint/pkg/result"
Expand Down Expand Up @@ -207,23 +208,25 @@ func (p *hunkChangesParser) parse(h *diffpkg.Hunk) []Change {
return p.ret
}

func getErrorTextForLinter(lintCtx *linter.Context, linterName string) string {
func getErrorTextForLinter(settings *config.LintersSettings, linterName string) string {
text := "File is not formatted"
switch linterName {
case gciName:
text = getErrorTextForGci(settings.Gci)
case gofumptName:
text = "File is not `gofumpt`-ed"
if lintCtx.Settings().Gofumpt.ExtraRules {
if settings.Gofumpt.ExtraRules {
text += " with `-extra`"
}
case gofmtName:
text = "File is not `gofmt`-ed"
if lintCtx.Settings().Gofmt.Simplify {
if settings.Gofmt.Simplify {
text += " with `-s`"
}
case goimportsName:
text = "File is not `goimports`-ed"
if lintCtx.Settings().Goimports.LocalPrefixes != "" {
text += " with -local " + lintCtx.Settings().Goimports.LocalPrefixes
if settings.Goimports.LocalPrefixes != "" {
text += " with -local " + settings.Goimports.LocalPrefixes
}
}
return text
Expand All @@ -247,9 +250,7 @@ func extractIssuesFromPatch(patch string, lintCtx *linter.Context, linterName st
}

for _, hunk := range d.Hunks {
p := hunkChangesParser{
log: lintCtx.Log,
}
p := hunkChangesParser{log: lintCtx.Log}

changes := p.parse(hunk)

Expand All @@ -261,7 +262,7 @@ func extractIssuesFromPatch(patch string, lintCtx *linter.Context, linterName st
Filename: d.NewName,
Line: change.LineRange.From,
},
Text: getErrorTextForLinter(lintCtx, linterName),
Text: getErrorTextForLinter(lintCtx.Settings(), linterName),
Replacement: &change.Replacement,
}
if change.LineRange.From != change.LineRange.To {
Expand Down
8 changes: 4 additions & 4 deletions test/linters_test.go
Expand Up @@ -105,7 +105,7 @@ func TestGciLocal(t *testing.T) {
require.NoError(t, err)

testshared.NewLintRunner(t).RunWithYamlConfig(string(cfg), args...).
ExpectHasIssue("testdata/gci/gci.go:9:1: Expected '\\n', Found '\\t'")
ExpectHasIssue("testdata/gci/gci.go:8: File is not `gci`-ed")
}

func TestMultipleOutputs(t *testing.T) {
Expand All @@ -124,7 +124,7 @@ func TestMultipleOutputs(t *testing.T) {
require.NoError(t, err)

testshared.NewLintRunner(t).RunWithYamlConfig(string(cfg), args...).
ExpectHasIssue("testdata/gci/gci.go:9:1: Expected '\\n', Found '\\t'").
ExpectHasIssue("testdata/gci/gci.go:8: File is not `gci`-ed").
ExpectOutputContains(`"Issues":[`)
}

Expand All @@ -144,7 +144,7 @@ func TestStderrOutput(t *testing.T) {
require.NoError(t, err)

testshared.NewLintRunner(t).RunWithYamlConfig(string(cfg), args...).
ExpectHasIssue("testdata/gci/gci.go:9:1: Expected '\\n', Found '\\t'").
ExpectHasIssue("testdata/gci/gci.go:8: File is not `gci`-ed").
ExpectOutputContains(`"Issues":[`)
}

Expand All @@ -167,7 +167,7 @@ func TestFileOutput(t *testing.T) {
require.NoError(t, err)

testshared.NewLintRunner(t).RunWithYamlConfig(string(cfg), args...).
ExpectHasIssue("testdata/gci/gci.go:9:1: Expected '\\n', Found '\\t'").
ExpectHasIssue("testdata/gci/gci.go:8: File is not `gci`-ed").
ExpectOutputNotContains(`"Issues":[`)

b, err := os.ReadFile(resultPath)
Expand Down
15 changes: 15 additions & 0 deletions test/testdata/fix/in/gci.go
@@ -0,0 +1,15 @@
//args: -Egci
//config_path: testdata/configs/gci.yml
package gci

import (
"github.com/golangci/golangci-lint/pkg/config"
"github.com/pkg/errors"
"fmt"
)

func GoimportsLocalTest() {
fmt.Print("x")
_ = config.Config{}
_ = errors.New("")
}
17 changes: 17 additions & 0 deletions test/testdata/fix/out/gci.go
@@ -0,0 +1,17 @@
//args: -Egci
//config_path: testdata/configs/gci.yml
package gci

import (
"fmt"

"github.com/golangci/golangci-lint/pkg/config"

"github.com/pkg/errors"
)

func GoimportsLocalTest() {
fmt.Print("x")
_ = config.Config{}
_ = errors.New("")
}