From 0683d45142d88a87dc87ead26292a89505a91633 Mon Sep 17 00:00:00 2001 From: Ludovic Fernandez Date: Fri, 8 Mar 2024 22:06:20 +0100 Subject: [PATCH] feat: add sort-order option (#4467) --- .golangci.reference.yml | 21 ++- pkg/commands/flagsets.go | 2 + pkg/config/config.go | 1 + pkg/config/output.go | 35 +++- pkg/config/output_test.go | 80 ++++++++ pkg/result/processors/sort_results.go | 207 ++++++++++++++++++--- pkg/result/processors/sort_results_test.go | 96 +++++++++- 7 files changed, 402 insertions(+), 40 deletions(-) create mode 100644 pkg/config/output_test.go diff --git a/.golangci.reference.yml b/.golangci.reference.yml index 36c8de81..7ac8e56e 100644 --- a/.golangci.reference.yml +++ b/.golangci.reference.yml @@ -109,14 +109,33 @@ output: # Default: "" path-prefix: "" - # Sort results by: filepath, line and column. + # Sort results by the order defined in `sort-order`. # Default: false sort-results: true + # Order to use when sorting results. + # Require `sort-results` to `true`. + # Possible values: `file`, `linter`, and `severity`. + # + # If the severity values are inside the following list, they are ordered in this order: + # 1. error + # 2. warning + # 3. high + # 4. medium + # 5. low + # Either they are sorted alphabetically. + # + # Default: ["file"] + sort-order: + - linter + - severity + - file # filepath, line, and column. + # Show statistics per linter. # Default: false show-stats: true + # All available settings of specific linters. linters-settings: asasalint: diff --git a/pkg/commands/flagsets.go b/pkg/commands/flagsets.go index e341b9fe..8ec309c6 100644 --- a/pkg/commands/flagsets.go +++ b/pkg/commands/flagsets.go @@ -71,6 +71,8 @@ func setupOutputFlagSet(v *viper.Viper, fs *pflag.FlagSet) { color.GreenString("Make issues output unique by line")) internal.AddFlagAndBind(v, fs, fs.Bool, "sort-results", "output.sort-results", false, color.GreenString("Sort linter results")) + internal.AddFlagAndBind(v, fs, fs.StringSlice, "sort-order", "output.sort-order", nil, + color.GreenString("Sort order of linter results")) internal.AddFlagAndBind(v, fs, fs.String, "path-prefix", "output.path-prefix", "", color.GreenString("Path prefix to add to output")) internal.AddFlagAndBind(v, fs, fs.Bool, "show-stats", "output.show-stats", false, color.GreenString("Show statistics per linter")) diff --git a/pkg/config/config.go b/pkg/config/config.go index 3bf61e89..386049da 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -37,6 +37,7 @@ func (c *Config) Validate() error { c.Severity.Validate, c.LintersSettings.Validate, c.Linters.Validate, + c.Output.Validate, } for _, v := range validators { diff --git a/pkg/config/output.go b/pkg/config/output.go index c882e215..9f0ee072 100644 --- a/pkg/config/output.go +++ b/pkg/config/output.go @@ -1,5 +1,11 @@ package config +import ( + "errors" + "fmt" + "slices" +) + const ( OutFormatJSON = "json" OutFormatLineNumber = "line-number" @@ -28,11 +34,26 @@ var OutFormats = []string{ } type Output struct { - Format string `mapstructure:"format"` - PrintIssuedLine bool `mapstructure:"print-issued-lines"` - PrintLinterName bool `mapstructure:"print-linter-name"` - UniqByLine bool `mapstructure:"uniq-by-line"` - SortResults bool `mapstructure:"sort-results"` - PathPrefix string `mapstructure:"path-prefix"` - ShowStats bool `mapstructure:"show-stats"` + Format string `mapstructure:"format"` + PrintIssuedLine bool `mapstructure:"print-issued-lines"` + PrintLinterName bool `mapstructure:"print-linter-name"` + UniqByLine bool `mapstructure:"uniq-by-line"` + SortResults bool `mapstructure:"sort-results"` + SortOrder []string `mapstructure:"sort-order"` + PathPrefix string `mapstructure:"path-prefix"` + ShowStats bool `mapstructure:"show-stats"` +} + +func (o *Output) Validate() error { + if !o.SortResults && len(o.SortOrder) > 0 { + return errors.New("sort-results should be 'true' to use sort-order") + } + + for _, order := range o.SortOrder { + if !slices.Contains([]string{"linter", "file", "severity"}, order) { + return fmt.Errorf("unsupported sort-order name %q", order) + } + } + + return nil } diff --git a/pkg/config/output_test.go b/pkg/config/output_test.go new file mode 100644 index 00000000..eb38d27c --- /dev/null +++ b/pkg/config/output_test.go @@ -0,0 +1,80 @@ +package config + +import ( + "testing" + + "github.com/stretchr/testify/require" +) + +func TestOutput_Validate(t *testing.T) { + testCases := []struct { + desc string + settings *Output + }{ + { + desc: "file", + settings: &Output{ + SortResults: true, + SortOrder: []string{"file"}, + }, + }, + { + desc: "linter", + settings: &Output{ + SortResults: true, + SortOrder: []string{"linter"}, + }, + }, + { + desc: "severity", + settings: &Output{ + SortResults: true, + SortOrder: []string{"severity"}, + }, + }, + } + + for _, test := range testCases { + test := test + t.Run(test.desc, func(t *testing.T) { + t.Parallel() + + err := test.settings.Validate() + require.NoError(t, err) + }) + } +} + +func TestOutput_Validate_error(t *testing.T) { + testCases := []struct { + desc string + settings *Output + expected string + }{ + { + desc: "sort-results false and sort-order", + settings: &Output{ + SortOrder: []string{"file"}, + }, + expected: "sort-results should be 'true' to use sort-order", + }, + { + desc: "invalid sort-order", + settings: &Output{ + SortResults: true, + SortOrder: []string{"a"}, + }, + expected: `unsupported sort-order name "a"`, + }, + } + + for _, test := range testCases { + test := test + t.Run(test.desc, func(t *testing.T) { + t.Parallel() + + err := test.settings.Validate() + require.EqualError(t, err, test.expected) + }) + } +} diff --git a/pkg/result/processors/sort_results.go b/pkg/result/processors/sort_results.go index 740c4fa8..5010fc2c 100644 --- a/pkg/result/processors/sort_results.go +++ b/pkg/result/processors/sort_results.go @@ -1,6 +1,9 @@ package processors import ( + "errors" + "fmt" + "slices" "sort" "strings" @@ -13,41 +16,69 @@ import ( // by sorting results.Issues using processor step, and chain based // rules that can compare different properties of the Issues struct. +const ( + fileOrderName = "file" + linterOrderName = "linter" + linterSeverityName = "severity" +) + var _ Processor = (*SortResults)(nil) type SortResults struct { - cmp comparator - cfg *config.Config + cmps map[string][]comparator + + cfg *config.Output } func NewSortResults(cfg *config.Config) *SortResults { - // For sorting we are comparing (in next order): file names, line numbers, - // position, and finally - giving up. return &SortResults{ - cmp: ByName{ - next: ByLine{ - next: ByColumn{}, - }, + cmps: map[string][]comparator{ + // For sorting we are comparing (in next order): + // file names, line numbers, position, and finally - giving up. + fileOrderName: {&byName{}, &byLine{}, &byColumn{}}, + // For sorting we are comparing: linter name + linterOrderName: {&byLinter{}}, + // For sorting we are comparing: severity + linterSeverityName: {&bySeverity{}}, }, - cfg: cfg, + cfg: &cfg.Output, } } // Process is performing sorting of the result issues. func (sr SortResults) Process(issues []result.Issue) ([]result.Issue, error) { - if !sr.cfg.Output.SortResults { + if !sr.cfg.SortResults { return issues, nil } + if len(sr.cfg.SortOrder) == 0 { + sr.cfg.SortOrder = []string{fileOrderName} + } + + var cmps []comparator + for _, name := range sr.cfg.SortOrder { + if c, ok := sr.cmps[name]; ok { + cmps = append(cmps, c...) + } else { + return nil, fmt.Errorf("unsupported sort-order name %q", name) + } + } + + cmp, err := mergeComparator(cmps) + if err != nil { + return nil, err + } + sort.Slice(issues, func(i, j int) bool { - return sr.cmp.Compare(&issues[i], &issues[j]) == Less + return cmp.Compare(&issues[i], &issues[j]) == Less }) return issues, nil } func (sr SortResults) Name() string { return "sort_results" } -func (sr SortResults) Finish() {} + +func (sr SortResults) Finish() {} type compareResult int @@ -71,28 +102,37 @@ func (c compareResult) String() string { return "Equal" case Greater: return "Greater" + default: + return "None" } - - return "None" } // comparator describe how to implement compare for two "issues" lexicographically type comparator interface { Compare(a, b *result.Issue) compareResult Next() comparator + AddNext(comparator) comparator + fmt.Stringer } var ( - _ comparator = (*ByName)(nil) - _ comparator = (*ByLine)(nil) - _ comparator = (*ByColumn)(nil) + _ comparator = (*byName)(nil) + _ comparator = (*byLine)(nil) + _ comparator = (*byColumn)(nil) + _ comparator = (*byLinter)(nil) + _ comparator = (*bySeverity)(nil) ) -type ByName struct{ next comparator } +type byName struct{ next comparator } -func (cmp ByName) Next() comparator { return cmp.next } +func (cmp *byName) Next() comparator { return cmp.next } -func (cmp ByName) Compare(a, b *result.Issue) compareResult { +func (cmp *byName) AddNext(c comparator) comparator { + cmp.next = c + return cmp +} + +func (cmp *byName) Compare(a, b *result.Issue) compareResult { var res compareResult if res = compareResult(strings.Compare(a.FilePath(), b.FilePath())); !res.isNeutral() { @@ -106,11 +146,20 @@ func (cmp ByName) Compare(a, b *result.Issue) compareResult { return res } -type ByLine struct{ next comparator } +func (cmp *byName) String() string { + return comparatorToString("byName", cmp) +} -func (cmp ByLine) Next() comparator { return cmp.next } +type byLine struct{ next comparator } -func (cmp ByLine) Compare(a, b *result.Issue) compareResult { +func (cmp *byLine) Next() comparator { return cmp.next } + +func (cmp *byLine) AddNext(c comparator) comparator { + cmp.next = c + return cmp +} + +func (cmp *byLine) Compare(a, b *result.Issue) compareResult { var res compareResult if res = numericCompare(a.Line(), b.Line()); !res.isNeutral() { @@ -124,11 +173,20 @@ func (cmp ByLine) Compare(a, b *result.Issue) compareResult { return res } -type ByColumn struct{ next comparator } +func (cmp *byLine) String() string { + return comparatorToString("byLine", cmp) +} -func (cmp ByColumn) Next() comparator { return cmp.next } +type byColumn struct{ next comparator } -func (cmp ByColumn) Compare(a, b *result.Issue) compareResult { +func (cmp *byColumn) Next() comparator { return cmp.next } + +func (cmp *byColumn) AddNext(c comparator) comparator { + cmp.next = c + return cmp +} + +func (cmp *byColumn) Compare(a, b *result.Issue) compareResult { var res compareResult if res = numericCompare(a.Column(), b.Column()); !res.isNeutral() { @@ -142,6 +200,94 @@ func (cmp ByColumn) Compare(a, b *result.Issue) compareResult { return res } +func (cmp *byColumn) String() string { + return comparatorToString("byColumn", cmp) +} + +type byLinter struct{ next comparator } + +func (cmp *byLinter) Next() comparator { return cmp.next } + +func (cmp *byLinter) AddNext(c comparator) comparator { + cmp.next = c + return cmp +} + +func (cmp *byLinter) Compare(a, b *result.Issue) compareResult { + var res compareResult + + if res = compareResult(strings.Compare(a.FromLinter, b.FromLinter)); !res.isNeutral() { + return res + } + + if next := cmp.Next(); next != nil { + return next.Compare(a, b) + } + + return res +} + +func (cmp *byLinter) String() string { + return comparatorToString("byLinter", cmp) +} + +type bySeverity struct{ next comparator } + +func (cmp *bySeverity) Next() comparator { return cmp.next } + +func (cmp *bySeverity) AddNext(c comparator) comparator { + cmp.next = c + return cmp +} + +func (cmp *bySeverity) Compare(a, b *result.Issue) compareResult { + var res compareResult + + if res = severityCompare(a.Severity, b.Severity); !res.isNeutral() { + return res + } + + if next := cmp.Next(); next != nil { + return next.Compare(a, b) + } + + return res +} + +func (cmp *bySeverity) String() string { + return comparatorToString("bySeverity", cmp) +} + +func mergeComparator(cmps []comparator) (comparator, error) { + if len(cmps) == 0 { + return nil, errors.New("no comparator") + } + + for i := 0; i < len(cmps)-1; i++ { + cmps[i].AddNext(cmps[i+1]) + } + + return cmps[0], nil +} + +func severityCompare(a, b string) compareResult { + // The position inside the slice define the importance (lower to higher). + classic := []string{"low", "medium", "high", "warning", "error"} + + if slices.Contains(classic, a) && slices.Contains(classic, b) { + switch { + case slices.Index(classic, a) > slices.Index(classic, b): + return Greater + case slices.Index(classic, a) < slices.Index(classic, b): + return Less + default: + return Equal + } + } + + return compareResult(strings.Compare(a, b)) +} + func numericCompare(a, b int) compareResult { var ( isValuesInvalid = a < 0 || b < 0 @@ -164,3 +310,12 @@ func numericCompare(a, b int) compareResult { return Equal } + +func comparatorToString(name string, c comparator) string { + s := name + if c.Next() != nil { + s += " > " + c.Next().String() + } + + return s +} diff --git a/pkg/result/processors/sort_results_test.go b/pkg/result/processors/sort_results_test.go index 141db723..b0d0043f 100644 --- a/pkg/result/processors/sort_results_test.go +++ b/pkg/result/processors/sort_results_test.go @@ -14,6 +14,8 @@ import ( var issues = []result.Issue{ { + FromLinter: "b", + Severity: "medium", Pos: token.Position{ Filename: "file_windows.go", Column: 80, @@ -21,6 +23,8 @@ var issues = []result.Issue{ }, }, { + FromLinter: "a", + Severity: "low", Pos: token.Position{ Filename: "file_linux.go", Column: 70, @@ -28,12 +32,16 @@ var issues = []result.Issue{ }, }, { + FromLinter: "c", + Severity: "high", Pos: token.Position{ Filename: "file_darwin.go", Line: 12, }, }, { + FromLinter: "c", + Severity: "high", Pos: token.Position{ Filename: "file_darwin.go", Column: 60, @@ -60,7 +68,7 @@ func testCompareValues(t *testing.T, cmp comparator, name string, tests []compar } func TestCompareByLine(t *testing.T) { - testCompareValues(t, ByLine{}, "Compare By Line", []compareTestCase{ + testCompareValues(t, &byLine{}, "Compare By Line", []compareTestCase{ {issues[0], issues[1], Less}, // 10 vs 11 {issues[0], issues[0], Equal}, // 10 vs 10 {issues[3], issues[3], Equal}, // 10 vs 10 @@ -76,7 +84,7 @@ func TestCompareByLine(t *testing.T) { } func TestCompareByName(t *testing.T) { //nolint:dupl - testCompareValues(t, ByName{}, "Compare By Name", []compareTestCase{ + testCompareValues(t, &byName{}, "Compare By Name", []compareTestCase{ {issues[0], issues[1], Greater}, // file_windows.go vs file_linux.go {issues[1], issues[2], Greater}, // file_linux.go vs file_darwin.go {issues[2], issues[3], Equal}, // file_darwin.go vs file_darwin.go @@ -91,7 +99,7 @@ func TestCompareByName(t *testing.T) { //nolint:dupl } func TestCompareByColumn(t *testing.T) { //nolint:dupl - testCompareValues(t, ByColumn{}, "Compare By Column", []compareTestCase{ + testCompareValues(t, &byColumn{}, "Compare By Column", []compareTestCase{ {issues[0], issues[1], Greater}, // 80 vs 70 {issues[1], issues[2], None}, // 70 vs zero value {issues[3], issues[3], Equal}, // 60 vs 60 @@ -105,10 +113,40 @@ func TestCompareByColumn(t *testing.T) { //nolint:dupl }) } +func TestCompareByLinter(t *testing.T) { //nolint:dupl + testCompareValues(t, &byLinter{}, "Compare By Linter", []compareTestCase{ + {issues[0], issues[1], Greater}, // b vs a + {issues[1], issues[2], Less}, // a vs c + {issues[2], issues[3], Equal}, // c vs c + {issues[1], issues[1], Equal}, // a vs a + {issues[1], issues[0], Less}, // a vs b + {issues[3], issues[2], Equal}, // c vs c + {issues[2], issues[1], Greater}, // c vs a + {issues[0], issues[0], Equal}, // b vs b + {issues[2], issues[2], Equal}, // a vs a + {issues[3], issues[3], Equal}, // c vs c + }) +} + +func TestCompareBySeverity(t *testing.T) { //nolint:dupl + testCompareValues(t, &bySeverity{}, "Compare By Severity", []compareTestCase{ + {issues[0], issues[1], Greater}, // medium vs low + {issues[1], issues[2], Less}, // low vs high + {issues[2], issues[3], Equal}, // high vs high + {issues[1], issues[1], Equal}, // low vs low + {issues[1], issues[0], Less}, // low vs medium + {issues[3], issues[2], Equal}, // high vs high + {issues[2], issues[1], Greater}, // high vs low + {issues[0], issues[0], Equal}, // medium vs medium + {issues[2], issues[2], Equal}, // low vs low + {issues[3], issues[3], Equal}, // high vs high + }) +} + func TestCompareNested(t *testing.T) { - var cmp = ByName{ - next: ByLine{ - next: ByColumn{}, + var cmp = &byName{ + next: &byLine{ + next: &byColumn{}, }, } @@ -175,3 +213,49 @@ func TestSorting(t *testing.T) { require.NoError(t, err) assert.Equal(t, []result.Issue{issues[3], issues[2], issues[1], issues[0]}, results) } + +func Test_mergeComparator(t *testing.T) { + testCases := []struct { + desc string + cmps []comparator + expected string + }{ + { + desc: "one", + cmps: []comparator{&byLinter{}}, + expected: "byLinter", + }, + { + desc: "two", + cmps: []comparator{&byLinter{}, &byName{}}, + expected: "byLinter > byName", + }, + { + desc: "all", + cmps: []comparator{&bySeverity{}, &byLinter{}, &byName{}, &byLine{}, &byColumn{}}, + expected: "bySeverity > byLinter > byName > byLine > byColumn", + }, + { + desc: "all reverse", + cmps: []comparator{&byColumn{}, &byLine{}, &byName{}, &byLinter{}, &bySeverity{}}, + expected: "byColumn > byLine > byName > byLinter > bySeverity", + }, + } + + for _, test := range testCases { + test := test + t.Run(test.desc, func(t *testing.T) { + t.Parallel() + + cmp, err := mergeComparator(test.cmps) + require.NoError(t, err) + + assert.Equal(t, test.expected, cmp.String()) + }) + } +} + +func Test_mergeComparator_error(t *testing.T) { + _, err := mergeComparator(nil) + require.EqualError(t, err, "no comparator") +}