Skip to content

Commit 8422c8d

Browse files
committed
lint: extract shared config-path helpers to simplify cops
Each of the four cops re-implemented its own path parsing and offense construction. This commit factors the common ground into a single helper file and slims every cop down to its rule. Changes: - New lint/configpath.go centralises: * configDir(filename) — name of the dir under pkg/config/ * versionFromDir("vN") — N or false * versionFromImport(path) — N if path ends in pkg/config/vN * importPath(*ast.ImportSpec) — unquoted import path * offense(c, fset, node, msg) — Offense covering an AST node * highestSiblingVersion(...) — cached scan for the highest vN - ConfigVersionImport: extracted a pure importViolation() function so the rule reads as a flat switch over the import path; folded the two inline checkVersionedImport / checkLatestImport helpers away. - LatestImportsPredecessor: dropped the bespoke mutex+map cache in favour of the shared sync.Map-backed helper. - main.go: cops are now declared in a single var slice, making it obvious where to register new ones. - 4 regexes → 2; ~130 lines of duplication removed; per-cop file size down to 50–90 lines. Behaviour is unchanged: all four cops still flag the same regressions (verified by injecting and reverting four faults across the config chain) and all pkg/config/... tests pass. Assisted-By: docker-agent
1 parent 5152f3b commit 8422c8d

6 files changed

Lines changed: 210 additions & 233 deletions

File tree

lint/config_package_name.go

Lines changed: 11 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -4,8 +4,6 @@ import (
44
"fmt"
55
"go/ast"
66
"go/token"
7-
"path/filepath"
8-
"regexp"
97
"strings"
108

119
"github.com/dgageot/rubocop-go/cop"
@@ -30,38 +28,24 @@ func (*ConfigPackageName) Description() string {
3028
}
3129
func (*ConfigPackageName) Severity() cop.Severity { return cop.Error }
3230

33-
// configDirRe matches files under pkg/config/<dir>/. The captured group is
34-
// the directory name immediately under pkg/config/. It accepts both
35-
// absolute and relative paths.
36-
var configDirRe = regexp.MustCompile(`(?:^|/)pkg/config/([^/]+)/[^/]+\.go$`)
37-
3831
func (c *ConfigPackageName) Check(fset *token.FileSet, file *ast.File) []cop.Offense {
3932
filename := fset.Position(file.Package).Filename
40-
normalized := filepath.ToSlash(filename)
41-
42-
m := configDirRe.FindStringSubmatch(normalized)
43-
if m == nil {
44-
return nil
45-
}
46-
47-
dir := m[1]
48-
49-
// pkg/config/<dir> contains the parsers/dispatcher; skip files that live
50-
// directly in pkg/config/.
51-
if strings.HasSuffix(dir, ".go") {
33+
dir := configDir(filename)
34+
if dir == "" {
5235
return nil
5336
}
5437

55-
expected := dir
5638
got := file.Name.Name
57-
if got == expected {
58-
return nil
59-
}
60-
// Black-box test packages (<dir>_test) are a legitimate Go convention.
61-
if strings.HasSuffix(filename, "_test.go") && got == expected+"_test" {
39+
switch got {
40+
case dir:
6241
return nil
42+
case dir + "_test":
43+
// Black-box test packages are a legitimate Go convention.
44+
if strings.HasSuffix(filename, "_test.go") {
45+
return nil
46+
}
6347
}
6448

65-
return []cop.Offense{cop.NewOffense(c, fset, file.Name.Pos(), file.Name.End(),
66-
fmt.Sprintf("file in pkg/config/%s/ must declare package %s, got %s", dir, expected, got))}
49+
return []cop.Offense{offense(c, fset, file.Name,
50+
fmt.Sprintf("file in pkg/config/%s/ must declare package %s, got %s", dir, dir, got))}
6751
}

lint/config_version_constant.go

Lines changed: 26 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -4,22 +4,19 @@ import (
44
"fmt"
55
"go/ast"
66
"go/token"
7-
"path/filepath"
8-
"regexp"
97
"strconv"
10-
"strings"
118

129
"github.com/dgageot/rubocop-go/cop"
1310
)
1411

1512
// ConfigVersionConstant enforces that a file under pkg/config/vN/ declaring a
1613
// `const Version = "<value>"` uses "<N>" as its value.
1714
//
18-
// This is a robustness check against the very common mistake of bumping the
19-
// directory name without bumping the constant (or vice-versa) when freezing
20-
// the work-in-progress config and creating a new "latest". A mismatch would
21-
// break the parser dispatch in pkg/config/versions.go silently, since the
22-
// runtime registers parsers keyed by `Version`.
15+
// This guards against the common mistake of bumping the directory name
16+
// without bumping the constant (or vice versa) when freezing the
17+
// work-in-progress config and creating a new "latest". A mismatch would
18+
// silently break the parser dispatch in pkg/config/versions.go, which
19+
// registers parsers keyed by `Version`.
2320
//
2421
// Files under pkg/config/latest/ are intentionally exempt: their `Version`
2522
// is the next, work-in-progress value (one greater than the highest vN).
@@ -31,21 +28,29 @@ func (*ConfigVersionConstant) Description() string {
3128
}
3229
func (*ConfigVersionConstant) Severity() cop.Severity { return cop.Error }
3330

34-
// vDirRe captures the numeric version in pkg/config/vN/.
35-
// It accepts both absolute and relative paths.
36-
var vDirRe = regexp.MustCompile(`(?:^|/)pkg/config/v(\d+)/[^/]+\.go$`)
37-
3831
func (c *ConfigVersionConstant) Check(fset *token.FileSet, file *ast.File) []cop.Offense {
39-
filename := fset.Position(file.Package).Filename
40-
normalized := filepath.ToSlash(filename)
41-
42-
m := vDirRe.FindStringSubmatch(normalized)
43-
if m == nil {
32+
dirVersion, ok := versionFromDir(configDir(fset.Position(file.Package).Filename))
33+
if !ok {
4434
return nil
4535
}
46-
expected := m[1]
36+
expected := strconv.Itoa(dirVersion)
4737

4838
var offenses []cop.Offense
39+
for _, lit := range versionStringLiterals(file) {
40+
got, err := strconv.Unquote(lit.Value)
41+
if err != nil || got == expected {
42+
continue
43+
}
44+
offenses = append(offenses, offense(c, fset, lit,
45+
fmt.Sprintf("Version in pkg/config/v%s/ must be %q, got %q", expected, expected, got)))
46+
}
47+
return offenses
48+
}
49+
50+
// versionStringLiterals returns the value literal of every top-level
51+
// `const Version = "<string>"` declaration in file.
52+
func versionStringLiterals(file *ast.File) []*ast.BasicLit {
53+
var lits []*ast.BasicLit
4954
for _, decl := range file.Decls {
5055
gen, ok := decl.(*ast.GenDecl)
5156
if !ok || gen.Tok != token.CONST {
@@ -57,33 +62,16 @@ func (c *ConfigVersionConstant) Check(fset *token.FileSet, file *ast.File) []cop
5762
continue
5863
}
5964
for i, name := range vs.Names {
60-
if name.Name != "Version" {
61-
continue
62-
}
63-
if i >= len(vs.Values) {
65+
if name.Name != "Version" || i >= len(vs.Values) {
6466
continue
6567
}
6668
lit, ok := vs.Values[i].(*ast.BasicLit)
6769
if !ok || lit.Kind != token.STRING {
6870
continue
6971
}
70-
got, err := strconv.Unquote(lit.Value)
71-
if err != nil {
72-
continue
73-
}
74-
if got == expected {
75-
continue
76-
}
77-
offenses = append(offenses, cop.NewOffense(c, fset, lit.Pos(), lit.End(),
78-
fmt.Sprintf("Version in pkg/config/v%s/ must be %q, got %q", expected, expected, got)))
72+
lits = append(lits, lit)
7973
}
8074
}
8175
}
82-
83-
return offenses
84-
}
85-
86-
// isLatestPath reports whether the given path lives under pkg/config/latest/.
87-
func isLatestPath(normalized string) bool {
88-
return strings.Contains(normalized, "pkg/config/latest/")
76+
return lits
8977
}

lint/config_version_import.go

Lines changed: 40 additions & 80 deletions
Original file line numberDiff line numberDiff line change
@@ -4,17 +4,14 @@ import (
44
"fmt"
55
"go/ast"
66
"go/token"
7-
"path/filepath"
8-
"regexp"
9-
"strconv"
107
"strings"
118

129
"github.com/dgageot/rubocop-go/cop"
1310
)
1411

15-
// ConfigVersionImport enforces that config version packages (pkg/config/vN)
16-
// only import their immediate predecessor (pkg/config/v{N-1}) and the shared
17-
// types package (pkg/config/types). This preserves the strict migration chain:
12+
// ConfigVersionImport enforces that config version packages (pkg/config/vN
13+
// and pkg/config/latest) only import their immediate predecessor and the
14+
// shared types package, preserving the strict migration chain:
1815
// v0 → v1 → v2 → … → latest.
1916
type ConfigVersionImport struct{}
2017

@@ -24,106 +21,69 @@ func (*ConfigVersionImport) Description() string {
2421
}
2522
func (*ConfigVersionImport) Severity() cop.Severity { return cop.Error }
2623

27-
// configVersionRe matches "pkg/config/vN" at the end of an import path.
28-
var configVersionRe = regexp.MustCompile(`pkg/config/v(\d+)$`)
29-
30-
// Check inspects import declarations in config version packages.
3124
func (c *ConfigVersionImport) Check(fset *token.FileSet, file *ast.File) []cop.Offense {
3225
if len(file.Imports) == 0 {
3326
return nil
3427
}
3528

36-
// Determine which config version package this file belongs to.
37-
filename := fset.Position(file.Package).Filename
38-
dirVersion, isVersioned := extractDirVersion(filename)
39-
dirIsLatest := isLatestDir(filename)
40-
41-
if !isVersioned && !dirIsLatest {
29+
dir := configDir(fset.Position(file.Package).Filename)
30+
if dir == "" {
4231
return nil
4332
}
44-
45-
// Black-box test files (package <dir>_test) live alongside the package
46-
// but are external to it; they're allowed to import what they please.
33+
// Black-box test files (package <dir>_test) are external to the package
34+
// and may import what they please.
4735
if strings.HasSuffix(file.Name.Name, "_test") {
4836
return nil
4937
}
5038

51-
var offenses []cop.Offense
39+
dirVersion, isVersioned := versionFromDir(dir)
40+
isLatest := dir == "latest"
41+
if !isVersioned && !isLatest {
42+
return nil
43+
}
5244

45+
var offenses []cop.Offense
5346
for _, imp := range file.Imports {
54-
importPath := strings.Trim(imp.Path.Value, `"`)
55-
56-
if !strings.Contains(importPath, "pkg/config/") {
57-
continue
58-
}
47+
path := importPath(imp)
5948

60-
if strings.HasSuffix(importPath, "pkg/config/types") {
49+
if !strings.Contains(path, "pkg/config/") || strings.HasSuffix(path, "pkg/config/types") {
6150
continue
6251
}
63-
64-
if isVersioned {
65-
offenses = append(offenses, c.checkVersionedImport(fset, imp, importPath, dirVersion)...)
66-
} else if dirIsLatest {
67-
offenses = append(offenses, c.checkLatestImport(fset, imp, importPath)...)
52+
if msg := importViolation(path, dirVersion, isLatest); msg != "" {
53+
offenses = append(offenses, offense(c, fset, imp.Path, msg))
6854
}
6955
}
70-
7156
return offenses
7257
}
7358

74-
func (c *ConfigVersionImport) checkVersionedImport(fset *token.FileSet, imp *ast.ImportSpec, importPath string, dirVersion int) []cop.Offense {
75-
if strings.HasSuffix(importPath, "pkg/config/latest") {
76-
return []cop.Offense{cop.NewOffense(c, fset, imp.Path.Pos(), imp.Path.End(),
77-
fmt.Sprintf("config v%d must not import pkg/config/latest", dirVersion))}
59+
// importViolation returns a non-empty error message if the given import path
60+
// is forbidden inside a config-version package, or "" if the import is fine.
61+
// dirVersion is the importing package's N (only meaningful when !isLatest).
62+
func importViolation(path string, dirVersion int, isLatest bool) string {
63+
if isLatest {
64+
// pkg/config/latest may only import other config-version packages.
65+
// (The "must be the immediate predecessor" rule lives in the
66+
// LatestImportsPredecessor cop.)
67+
if _, ok := versionFromImport(path); ok {
68+
return ""
69+
}
70+
return "pkg/config/latest should only import config version or types packages, not " + path
7871
}
7972

80-
m := configVersionRe.FindStringSubmatch(importPath)
81-
if m == nil {
82-
return nil
73+
// Versioned package (vN).
74+
if strings.HasSuffix(path, "pkg/config/latest") {
75+
return fmt.Sprintf("config v%d must not import pkg/config/latest", dirVersion)
76+
}
77+
imported, ok := versionFromImport(path)
78+
if !ok {
79+
return ""
8380
}
84-
85-
importedVersion, _ := strconv.Atoi(m[1])
8681
expected := dirVersion - 1
87-
8882
if expected < 0 {
89-
return []cop.Offense{cop.NewOffense(c, fset, imp.Path.Pos(), imp.Path.End(),
90-
"config v0 must not import other config version packages")}
91-
}
92-
93-
if importedVersion != expected {
94-
return []cop.Offense{cop.NewOffense(c, fset, imp.Path.Pos(), imp.Path.End(),
95-
fmt.Sprintf("config v%d must import v%d (its predecessor), not v%d", dirVersion, expected, importedVersion))}
96-
}
97-
98-
return nil
99-
}
100-
101-
func (c *ConfigVersionImport) checkLatestImport(fset *token.FileSet, imp *ast.ImportSpec, importPath string) []cop.Offense {
102-
if configVersionRe.MatchString(importPath) {
103-
return nil
83+
return "config v0 must not import other config version packages"
10484
}
105-
106-
return []cop.Offense{cop.NewOffense(c, fset, imp.Path.Pos(), imp.Path.End(),
107-
"pkg/config/latest should only import config version or types packages, not "+importPath)}
108-
}
109-
110-
func extractDirVersion(filename string) (int, bool) {
111-
normalized := filepath.ToSlash(filename)
112-
113-
re := regexp.MustCompile(`(?:^|/)pkg/config/v(\d+)/`)
114-
m := re.FindStringSubmatch(normalized)
115-
if m == nil {
116-
return 0, false
85+
if imported != expected {
86+
return fmt.Sprintf("config v%d must import v%d (its predecessor), not v%d", dirVersion, expected, imported)
11787
}
118-
119-
v, err := strconv.Atoi(m[1])
120-
if err != nil {
121-
return 0, false
122-
}
123-
return v, true
124-
}
125-
126-
func isLatestDir(filename string) bool {
127-
normalized := filepath.ToSlash(filename)
128-
return strings.Contains(normalized, "pkg/config/latest/")
88+
return ""
12989
}

0 commit comments

Comments
 (0)