From 78d2118c0b119f87e1d72ca2a3b7459ed14d6189 Mon Sep 17 00:00:00 2001
From: Ludovic Fernandez <ldez@users.noreply.github.com>
Date: Wed, 13 Mar 2024 15:49:29 +0100
Subject: [PATCH] dev: clean up (#4495)
---
pkg/config/output.go | 11 +-
pkg/config/output_test.go | 15 ++
pkg/result/processors/sort_results.go | 109 ++++++------
pkg/result/processors/sort_results_test.go | 189 ++++++++++++---------
4 files changed, 187 insertions(+), 137 deletions(-)
diff --git a/pkg/config/output.go b/pkg/config/output.go
index 9f0ee072..a0734ab7 100644
--- a/pkg/config/output.go
+++ b/pkg/config/output.go
@@ -4,6 +4,7 @@ import (
"errors"
"fmt"
"slices"
+ "strings"
)
const (
@@ -49,8 +50,16 @@ func (o *Output) Validate() error {
return errors.New("sort-results should be 'true' to use sort-order")
}
+ validOrders := []string{"linter", "file", "severity"}
+
+ all := strings.Join(o.SortOrder, " ")
+
for _, order := range o.SortOrder {
- if !slices.Contains([]string{"linter", "file", "severity"}, order) {
+ if strings.Count(all, order) > 1 {
+ return fmt.Errorf("the sort-order name %q is repeated several times", order)
+ }
+
+ if !slices.Contains(validOrders, order) {
return fmt.Errorf("unsupported sort-order name %q", order)
}
}
diff --git a/pkg/config/output_test.go b/pkg/config/output_test.go
index eb38d27c..93b56ff3 100644
--- a/pkg/config/output_test.go
+++ b/pkg/config/output_test.go
@@ -32,6 +32,13 @@ func TestOutput_Validate(t *testing.T) {
SortOrder: []string{"severity"},
},
},
+ {
+ desc: "multiple",
+ settings: &Output{
+ SortResults: true,
+ SortOrder: []string{"file", "linter", "severity"},
+ },
+ },
}
for _, test := range testCases {
@@ -66,6 +73,14 @@ func TestOutput_Validate_error(t *testing.T) {
},
expected: `unsupported sort-order name "a"`,
},
+ {
+ desc: "duplicate",
+ settings: &Output{
+ SortResults: true,
+ SortOrder: []string{"file", "linter", "severity", "linter"},
+ },
+ expected: `the sort-order name "linter" is repeated several times`,
+ },
}
for _, test := range testCases {
diff --git a/pkg/result/processors/sort_results.go b/pkg/result/processors/sort_results.go
index 5010fc2c..01ccc994 100644
--- a/pkg/result/processors/sort_results.go
+++ b/pkg/result/processors/sort_results.go
@@ -17,9 +17,9 @@ import (
// rules that can compare different properties of the Issues struct.
const (
- fileOrderName = "file"
- linterOrderName = "linter"
- linterSeverityName = "severity"
+ orderNameFile = "file"
+ orderNameLinter = "linter"
+ orderNameSeverity = "severity"
)
var _ Processor = (*SortResults)(nil)
@@ -35,11 +35,11 @@ func NewSortResults(cfg *config.Config) *SortResults {
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{}},
+ orderNameFile: {&byFileName{}, &byLine{}, &byColumn{}},
// For sorting we are comparing: linter name
- linterOrderName: {&byLinter{}},
+ orderNameLinter: {&byLinter{}},
// For sorting we are comparing: severity
- linterSeverityName: {&bySeverity{}},
+ orderNameSeverity: {&bySeverity{}},
},
cfg: &cfg.Output,
}
@@ -52,7 +52,7 @@ func (sr SortResults) Process(issues []result.Issue) ([]result.Issue, error) {
}
if len(sr.cfg.SortOrder) == 0 {
- sr.cfg.SortOrder = []string{fileOrderName}
+ sr.cfg.SortOrder = []string{orderNameFile}
}
var cmps []comparator
@@ -64,13 +64,13 @@ func (sr SortResults) Process(issues []result.Issue) ([]result.Issue, error) {
}
}
- cmp, err := mergeComparator(cmps)
+ cmp, err := mergeComparators(cmps)
if err != nil {
return nil, err
}
sort.Slice(issues, func(i, j int) bool {
- return cmp.Compare(&issues[i], &issues[j]) == Less
+ return cmp.Compare(&issues[i], &issues[j]) == less
})
return issues, nil
@@ -83,31 +83,31 @@ func (sr SortResults) Finish() {}
type compareResult int
const (
- Less compareResult = iota - 1
- Equal
- Greater
- None
+ less compareResult = iota - 1
+ equal
+ greater
+ none
)
func (c compareResult) isNeutral() bool {
// return true if compare result is incomparable or equal.
- return c == None || c == Equal
+ return c == none || c == equal
}
func (c compareResult) String() string {
switch c {
- case Less:
- return "Less"
- case Equal:
- return "Equal"
- case Greater:
- return "Greater"
+ case less:
+ return "less"
+ case equal:
+ return "equal"
+ case greater:
+ return "greater"
default:
- return "None"
+ return "none"
}
}
-// comparator describe how to implement compare for two "issues" lexicographically
+// comparator describes how to implement compare for two "issues".
type comparator interface {
Compare(a, b *result.Issue) compareResult
Next() comparator
@@ -116,26 +116,25 @@ type comparator interface {
}
var (
- _ comparator = (*byName)(nil)
+ _ comparator = (*byFileName)(nil)
_ comparator = (*byLine)(nil)
_ comparator = (*byColumn)(nil)
_ comparator = (*byLinter)(nil)
_ comparator = (*bySeverity)(nil)
)
-type byName struct{ next comparator }
+type byFileName struct{ next comparator }
-func (cmp *byName) Next() comparator { return cmp.next }
+func (cmp *byFileName) Next() comparator { return cmp.next }
-func (cmp *byName) AddNext(c comparator) comparator {
+func (cmp *byFileName) 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() {
+func (cmp *byFileName) Compare(a, b *result.Issue) compareResult {
+ res := compareResult(strings.Compare(a.FilePath(), b.FilePath()))
+ if !res.isNeutral() {
return res
}
@@ -146,8 +145,8 @@ func (cmp *byName) Compare(a, b *result.Issue) compareResult {
return res
}
-func (cmp *byName) String() string {
- return comparatorToString("byName", cmp)
+func (cmp *byFileName) String() string {
+ return comparatorToString("byFileName", cmp)
}
type byLine struct{ next comparator }
@@ -160,9 +159,8 @@ func (cmp *byLine) AddNext(c comparator) comparator {
}
func (cmp *byLine) Compare(a, b *result.Issue) compareResult {
- var res compareResult
-
- if res = numericCompare(a.Line(), b.Line()); !res.isNeutral() {
+ res := numericCompare(a.Line(), b.Line())
+ if !res.isNeutral() {
return res
}
@@ -187,9 +185,8 @@ func (cmp *byColumn) AddNext(c comparator) comparator {
}
func (cmp *byColumn) Compare(a, b *result.Issue) compareResult {
- var res compareResult
-
- if res = numericCompare(a.Column(), b.Column()); !res.isNeutral() {
+ res := numericCompare(a.Column(), b.Column())
+ if !res.isNeutral() {
return res
}
@@ -214,9 +211,8 @@ func (cmp *byLinter) AddNext(c comparator) comparator {
}
func (cmp *byLinter) Compare(a, b *result.Issue) compareResult {
- var res compareResult
-
- if res = compareResult(strings.Compare(a.FromLinter, b.FromLinter)); !res.isNeutral() {
+ res := compareResult(strings.Compare(a.FromLinter, b.FromLinter))
+ if !res.isNeutral() {
return res
}
@@ -241,9 +237,8 @@ func (cmp *bySeverity) AddNext(c comparator) comparator {
}
func (cmp *bySeverity) Compare(a, b *result.Issue) compareResult {
- var res compareResult
-
- if res = severityCompare(a.Severity, b.Severity); !res.isNeutral() {
+ res := severityCompare(a.Severity, b.Severity)
+ if !res.isNeutral() {
return res
}
@@ -258,7 +253,7 @@ func (cmp *bySeverity) String() string {
return comparatorToString("bySeverity", cmp)
}
-func mergeComparator(cmps []comparator) (comparator, error) {
+func mergeComparators(cmps []comparator) (comparator, error) {
if len(cmps) == 0 {
return nil, errors.New("no comparator")
}
@@ -277,14 +272,22 @@ func severityCompare(a, b string) compareResult {
if slices.Contains(classic, a) && slices.Contains(classic, b) {
switch {
case slices.Index(classic, a) > slices.Index(classic, b):
- return Greater
+ return greater
case slices.Index(classic, a) < slices.Index(classic, b):
- return Less
+ return less
default:
- return Equal
+ return equal
}
}
+ if slices.Contains(classic, a) {
+ return greater
+ }
+
+ if slices.Contains(classic, b) {
+ return less
+ }
+
return compareResult(strings.Compare(a, b))
}
@@ -299,16 +302,16 @@ func numericCompare(a, b int) compareResult {
switch {
case isZeroValuesBoth || isEqual:
- return Equal
+ return equal
case isValuesInvalid || isZeroValueInA || isZeroValueInB:
- return None
+ return none
case a > b:
- return Greater
+ return greater
case a < b:
- return Less
+ return less
}
- return Equal
+ return equal
}
func comparatorToString(name string, c comparator) string {
diff --git a/pkg/result/processors/sort_results_test.go b/pkg/result/processors/sort_results_test.go
index b0d0043f..ef73d5bb 100644
--- a/pkg/result/processors/sort_results_test.go
+++ b/pkg/result/processors/sort_results_test.go
@@ -50,6 +50,27 @@ var issues = []result.Issue{
},
}
+var extraSeverityIssues = []result.Issue{
+ {
+ FromLinter: "c",
+ Severity: "error",
+ Pos: token.Position{
+ Filename: "file_darwin.go",
+ Column: 60,
+ Line: 10,
+ },
+ },
+ {
+ FromLinter: "c",
+ Severity: "aaaa",
+ Pos: token.Position{
+ Filename: "file_darwin.go",
+ Column: 60,
+ Line: 10,
+ },
+ },
+}
+
type compareTestCase struct {
a, b result.Issue
expected compareResult
@@ -69,98 +90,100 @@ func testCompareValues(t *testing.T, cmp comparator, name string, tests []compar
func TestCompareByLine(t *testing.T) {
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
- {issues[0], issues[3], Equal}, // 10 vs 10
- {issues[3], issues[2], Less}, // 10 vs 12
- {issues[1], issues[1], Equal}, // 11 vs 11
- {issues[1], issues[0], Greater}, // 11 vs 10
- {issues[1], issues[2], Less}, // 11 vs 12
- {issues[2], issues[3], Greater}, // 12 vs 10
- {issues[2], issues[1], Greater}, // 12 vs 11
- {issues[2], issues[2], Equal}, // 12 vs 12
+ {issues[0], issues[1], less}, // 10 vs 11
+ {issues[0], issues[0], equal}, // 10 vs 10
+ {issues[3], issues[3], equal}, // 10 vs 10
+ {issues[0], issues[3], equal}, // 10 vs 10
+ {issues[3], issues[2], less}, // 10 vs 12
+ {issues[1], issues[1], equal}, // 11 vs 11
+ {issues[1], issues[0], greater}, // 11 vs 10
+ {issues[1], issues[2], less}, // 11 vs 12
+ {issues[2], issues[3], greater}, // 12 vs 10
+ {issues[2], issues[1], greater}, // 12 vs 11
+ {issues[2], issues[2], equal}, // 12 vs 12
})
}
-func TestCompareByName(t *testing.T) { //nolint:dupl
- 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
- {issues[1], issues[1], Equal}, // file_linux.go vs file_linux.go
- {issues[1], issues[0], Less}, // file_linux.go vs file_windows.go
- {issues[3], issues[2], Equal}, // file_darwin.go vs file_darwin.go
- {issues[2], issues[1], Less}, // file_darwin.go vs file_linux.go
- {issues[0], issues[0], Equal}, // file_windows.go vs file_windows.go
- {issues[2], issues[2], Equal}, // file_darwin.go vs file_darwin.go
- {issues[3], issues[3], Equal}, // file_darwin.go vs file_darwin.go
+func TestCompareByFileName(t *testing.T) { //nolint:dupl
+ testCompareValues(t, &byFileName{}, "Compare By File 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
+ {issues[1], issues[1], equal}, // file_linux.go vs file_linux.go
+ {issues[1], issues[0], less}, // file_linux.go vs file_windows.go
+ {issues[3], issues[2], equal}, // file_darwin.go vs file_darwin.go
+ {issues[2], issues[1], less}, // file_darwin.go vs file_linux.go
+ {issues[0], issues[0], equal}, // file_windows.go vs file_windows.go
+ {issues[2], issues[2], equal}, // file_darwin.go vs file_darwin.go
+ {issues[3], issues[3], equal}, // file_darwin.go vs file_darwin.go
})
}
func TestCompareByColumn(t *testing.T) { //nolint:dupl
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
- {issues[2], issues[3], None}, // zero value vs 60
- {issues[2], issues[1], None}, // zero value vs 70
- {issues[1], issues[0], Less}, // 70 vs 80
- {issues[1], issues[1], Equal}, // 70 vs 70
- {issues[3], issues[2], None}, // vs zero value
- {issues[2], issues[2], Equal}, // zero value vs zero value
- {issues[1], issues[1], Equal}, // 70 vs 70
+ {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
+ {issues[2], issues[3], none}, // zero value vs 60
+ {issues[2], issues[1], none}, // zero value vs 70
+ {issues[1], issues[0], less}, // 70 vs 80
+ {issues[1], issues[1], equal}, // 70 vs 70
+ {issues[3], issues[2], none}, // vs zero value
+ {issues[2], issues[2], equal}, // zero value vs zero value
+ {issues[1], issues[1], equal}, // 70 vs 70
})
}
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
+ {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
+func TestCompareBySeverity(t *testing.T) {
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
+ {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
+ {extraSeverityIssues[0], extraSeverityIssues[1], greater}, // classic vs unknown
+ {extraSeverityIssues[1], extraSeverityIssues[0], less}, // unknown vs classic
})
}
func TestCompareNested(t *testing.T) {
- var cmp = &byName{
+ var cmp = &byFileName{
next: &byLine{
next: &byColumn{},
},
}
testCompareValues(t, cmp, "Nested Comparing", []compareTestCase{
- {issues[1], issues[0], Less}, // file_linux.go vs file_windows.go
- {issues[2], issues[1], Less}, // file_darwin.go vs file_linux.go
- {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[3], issues[2], Less}, // file_darwin.go vs file_darwin.go, 10 vs 12
- {issues[0], issues[0], Equal}, // file_windows.go vs file_windows.go
- {issues[2], issues[3], Greater}, // file_darwin.go vs file_darwin.go, 12 vs 10
- {issues[1], issues[1], Equal}, // file_linux.go vs file_linux.go
- {issues[2], issues[2], Equal}, // file_darwin.go vs file_darwin.go
- {issues[3], issues[3], Equal}, // file_darwin.go vs file_darwin.go
+ {issues[1], issues[0], less}, // file_linux.go vs file_windows.go
+ {issues[2], issues[1], less}, // file_darwin.go vs file_linux.go
+ {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[3], issues[2], less}, // file_darwin.go vs file_darwin.go, 10 vs 12
+ {issues[0], issues[0], equal}, // file_windows.go vs file_windows.go
+ {issues[2], issues[3], greater}, // file_darwin.go vs file_darwin.go, 12 vs 10
+ {issues[1], issues[1], equal}, // file_linux.go vs file_linux.go
+ {issues[2], issues[2], equal}, // file_darwin.go vs file_darwin.go
+ {issues[3], issues[3], equal}, // file_darwin.go vs file_darwin.go
})
}
@@ -169,14 +192,14 @@ func TestNumericCompare(t *testing.T) {
a, b int
expected compareResult
}{
- {0, 0, Equal},
- {0, 1, None},
- {1, 0, None},
- {1, -1, None},
- {-1, 1, None},
- {1, 1, Equal},
- {1, 2, Less},
- {2, 1, Greater},
+ {0, 0, equal},
+ {0, 1, none},
+ {1, 0, none},
+ {1, -1, none},
+ {-1, 1, none},
+ {1, 1, equal},
+ {1, 2, less},
+ {2, 1, greater},
}
t.Parallel()
@@ -214,7 +237,7 @@ func TestSorting(t *testing.T) {
assert.Equal(t, []result.Issue{issues[3], issues[2], issues[1], issues[0]}, results)
}
-func Test_mergeComparator(t *testing.T) {
+func Test_mergeComparators(t *testing.T) {
testCases := []struct {
desc string
cmps []comparator
@@ -227,18 +250,18 @@ func Test_mergeComparator(t *testing.T) {
},
{
desc: "two",
- cmps: []comparator{&byLinter{}, &byName{}},
- expected: "byLinter > byName",
+ cmps: []comparator{&byLinter{}, &byFileName{}},
+ expected: "byLinter > byFileName",
},
{
desc: "all",
- cmps: []comparator{&bySeverity{}, &byLinter{}, &byName{}, &byLine{}, &byColumn{}},
- expected: "bySeverity > byLinter > byName > byLine > byColumn",
+ cmps: []comparator{&bySeverity{}, &byLinter{}, &byFileName{}, &byLine{}, &byColumn{}},
+ expected: "bySeverity > byLinter > byFileName > byLine > byColumn",
},
{
desc: "all reverse",
- cmps: []comparator{&byColumn{}, &byLine{}, &byName{}, &byLinter{}, &bySeverity{}},
- expected: "byColumn > byLine > byName > byLinter > bySeverity",
+ cmps: []comparator{&byColumn{}, &byLine{}, &byFileName{}, &byLinter{}, &bySeverity{}},
+ expected: "byColumn > byLine > byFileName > byLinter > bySeverity",
},
}
@@ -247,7 +270,7 @@ func Test_mergeComparator(t *testing.T) {
t.Run(test.desc, func(t *testing.T) {
t.Parallel()
- cmp, err := mergeComparator(test.cmps)
+ cmp, err := mergeComparators(test.cmps)
require.NoError(t, err)
assert.Equal(t, test.expected, cmp.String())
@@ -255,7 +278,7 @@ func Test_mergeComparator(t *testing.T) {
}
}
-func Test_mergeComparator_error(t *testing.T) {
- _, err := mergeComparator(nil)
+func Test_mergeComparators_error(t *testing.T) {
+ _, err := mergeComparators(nil)
require.EqualError(t, err, "no comparator")
}