Skip to content

Commit 872435a

Browse files
jedudenclaude
andauthored
arch-fix: move Flavor and convention types to internal/convention (#280)
* refactor(arch): relocate convention types from markdownflavor to internal/convention Hoist Convention, RulePreset, and Flavor data shapes plus ParseFlavor / Lookup / Names out of the markdownflavor rule into a new internal/convention package. internal/config now imports internal/convention instead of reaching into a rule package, restoring the layering direction recorded in the architecture hub. The markdownflavor rule keeps its rule.Rule impl and the Feature/support table that maps flavor → which features it accepts; r.Flavor is now convention.Flavor and Supports is a free function in the rule package taking a convention.Flavor. Adds TestConfigDoesNotImportRules to guard the new direction: it parses every non-test file under internal/config/ and fails if any import path contains internal/rules/. Closes plan/155. https://claude.ai/code/session_01SEJY8ZzkXWkinVyUYMiiKB * fix(convention): tighten Flavor.IsValid to reject unknown values Flavor.IsValid previously returned true for any non-zero value, so an out-of-range cast like Flavor(999) would slip past the check and reach the markdownflavor rule, which would then treat every feature as unsupported. The docstring already promised "reports whether f names a recognised flavor", so the fix is to delegate to String — every recognised flavor has a name, every other value returns "". Adds a Flavor(999) case to TestFlavorIsValid. https://claude.ai/code/session_01SEJY8ZzkXWkinVyUYMiiKB * fix(plan): use numeric:id sort to keep PLAN.md in plan-id order The catalog directive header reverted to `sort: id` after the rebase, which sorts lexicographically — so plan 155 lands above plan 52 in the rendered table. The `numeric:id` prefix was added in 8d37e44 specifically for this case; restore it and regenerate. https://claude.ai/code/session_01SEJY8ZzkXWkinVyUYMiiKB --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 62e6e31 commit 872435a

15 files changed

Lines changed: 297 additions & 235 deletions

PLAN.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -83,5 +83,5 @@ footer: |
8383
| 153 || sonnet | [Catalog directive — accept `..` globs within project root](plan/153_catalog-dotdot-globs.md) |
8484
| 153 || opus | [Unify linkgraph and the LSP symbol index](plan/153_unify-linkgraph-and-lsp-index.md) |
8585
| 154 | 🔲 | sonnet | [arch-fix: extract cross-rule helpers](plan/154_arch-fix-rule-helper-extraction.md) |
86-
| 155 | 🔲 | sonnet | [arch-fix: relocate convention types out of markdownflavor](plan/155_arch-fix-convention-config-ownership.md) |
86+
| 155 | | sonnet | [arch-fix: relocate convention types out of markdownflavor](plan/155_arch-fix-convention-config-ownership.md) |
8787
<?/catalog?>

docs/development/architecture-audit.md

Lines changed: 20 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -58,33 +58,29 @@ helper packages (e.g.
5858
donor and consumer rules. Scheduled by
5959
[plan/154](../../plan/154_arch-fix-rule-helper-extraction.md).
6060

61+
### resolved by plan/155
62+
6163
Config imports a rule package.
6264

6365
[`internal/config/convention.go`](../../internal/config/convention.go)
64-
imports `internal/rules/markdownflavor`.
65-
66-
The imported symbols are:
67-
68-
- `Convention`
69-
- `RulePreset`
70-
- `ParseFlavor`
71-
- `Lookup`
72-
- `ConventionNames`
73-
74-
This violates dependency direction. The
75-
[layering map](architecture/index.md)
76-
puts rules at the lowest layer. Config
77-
is a mid-layer.
78-
79-
Severity: blocker.
80-
81-
Fix by hoisting the convention and flavor
82-
data shapes into a layer config can own.
83-
A new `internal/convention` package
84-
works. The markdownflavor rule then
85-
consumes those types instead of defining
86-
them. Scheduled by
87-
[plan/155](../../plan/155_arch-fix-convention-config-ownership.md).
66+
imported `internal/rules/markdownflavor`
67+
to use `Convention`, `RulePreset`,
68+
`ParseFlavor`, `Lookup`, and
69+
`ConventionNames`.
70+
71+
[plan/155](../../plan/155_arch-fix-convention-config-ownership.md)
72+
hoisted those shapes into a new
73+
[internal/convention package](../../internal/convention/convention.go).
74+
The markdownflavor rule now imports
75+
`internal/convention` for the `Flavor`
76+
type. The config package depends on
77+
`internal/convention`, not on a rule.
78+
79+
`TestConfigDoesNotImportRules` guards
80+
the new direction. It parses every
81+
non-test file under `internal/config/`.
82+
It fails if any import path contains
83+
`internal/rules/`.
8884

8985
### tax
9086

internal/config/config.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -53,8 +53,8 @@ type Config struct {
5353
// values: "portable", "github", "plain". User-defined
5454
// conventions may also be referenced here after being declared
5555
// under the top-level `conventions:` key. Empty means no
56-
// convention. See internal/rules/markdownflavor/conventions.go
57-
// and docs/reference/conventions.md.
56+
// convention. See internal/convention/convention.go and
57+
// docs/reference/conventions.md.
5858
Convention string `yaml:"convention,omitempty"`
5959

6060
// Conventions holds user-defined convention bundles declared

internal/config/convention.go

Lines changed: 15 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -6,8 +6,8 @@ import (
66

77
"gopkg.in/yaml.v3"
88

9+
"github.com/jeduden/mdsmith/internal/convention"
910
"github.com/jeduden/mdsmith/internal/rule"
10-
"github.com/jeduden/mdsmith/internal/rules/markdownflavor"
1111
)
1212

1313
// applyConvention reads the top-level Convention selector from the
@@ -42,7 +42,7 @@ func applyConvention(cfg *Config) error {
4242
if cfg.Convention == "" {
4343
return nil
4444
}
45-
convention, err := markdownflavor.Lookup(cfg.Convention, userMap)
45+
conv, err := convention.Lookup(cfg.Convention, userMap)
4646
if err != nil {
4747
return fmt.Errorf("convention: %w", err)
4848
}
@@ -53,16 +53,16 @@ func applyConvention(cfg *Config) error {
5353
if err != nil {
5454
return err
5555
}
56-
if userFlavor != "" && userFlavor != convention.Flavor.String() {
56+
if userFlavor != "" && userFlavor != conv.Flavor.String() {
5757
return fmt.Errorf(
5858
"rules.markdown-flavor: convention %q requires flavor %q, but flavor is set to %q",
59-
convention.Name, convention.Flavor, userFlavor,
59+
conv.Name, conv.Flavor, userFlavor,
6060
)
6161
}
6262
}
6363

64-
preset := make(map[string]RuleCfg, len(convention.Rules))
65-
for ruleName, p := range convention.Rules {
64+
preset := make(map[string]RuleCfg, len(conv.Rules))
65+
for ruleName, p := range conv.Rules {
6666
preset[ruleName] = RuleCfg{
6767
Enabled: p.Enabled,
6868
Settings: cloneSettings(p.Settings),
@@ -103,7 +103,7 @@ func copyConventionPreset(p map[string]RuleCfg) map[string]RuleCfg {
103103
}
104104

105105
// buildUserConventionMap validates every entry in cfg.Conventions and
106-
// returns them as a map[string]markdownflavor.Convention ready for
106+
// returns them as a map[string]convention.Convention ready for
107107
// Lookup. Validation checks:
108108
// - The name must not be a reserved built-in name.
109109
// - The flavor must be a recognised flavor string.
@@ -112,17 +112,17 @@ func copyConventionPreset(p map[string]RuleCfg) map[string]RuleCfg {
112112
// check (called on a cloned instance so the registry is unaffected).
113113
//
114114
// Returns nil when cfg.Conventions is empty.
115-
func buildUserConventionMap(cfg *Config) (map[string]markdownflavor.Convention, error) {
115+
func buildUserConventionMap(cfg *Config) (map[string]convention.Convention, error) {
116116
if len(cfg.Conventions) == 0 {
117117
return nil, nil
118118
}
119119

120-
reserved := make(map[string]bool, len(markdownflavor.ConventionNames()))
121-
for _, name := range markdownflavor.ConventionNames() {
120+
reserved := make(map[string]bool, len(convention.Names()))
121+
for _, name := range convention.Names() {
122122
reserved[name] = true
123123
}
124124

125-
result := make(map[string]markdownflavor.Convention, len(cfg.Conventions))
125+
result := make(map[string]convention.Convention, len(cfg.Conventions))
126126
for name, uc := range cfg.Conventions {
127127
if reserved[name] {
128128
return nil, fmt.Errorf(
@@ -131,15 +131,15 @@ func buildUserConventionMap(cfg *Config) (map[string]markdownflavor.Convention,
131131
)
132132
}
133133

134-
fl, ok := markdownflavor.ParseFlavor(uc.Flavor)
134+
fl, ok := convention.ParseFlavor(uc.Flavor)
135135
if !ok {
136136
return nil, fmt.Errorf(
137137
"convention %q: unknown flavor %q",
138138
name, uc.Flavor,
139139
)
140140
}
141141

142-
rules := make(map[string]markdownflavor.RulePreset, len(uc.Rules))
142+
rules := make(map[string]convention.RulePreset, len(uc.Rules))
143143
for ruleName, rc := range uc.Rules {
144144
r := rule.ByName(ruleName)
145145
if r == nil {
@@ -153,13 +153,13 @@ func buildUserConventionMap(cfg *Config) (map[string]markdownflavor.Convention,
153153
return nil, err
154154
}
155155
}
156-
rules[ruleName] = markdownflavor.RulePreset{
156+
rules[ruleName] = convention.RulePreset{
157157
Enabled: rc.Enabled,
158158
Settings: cloneSettings(rc.Settings),
159159
}
160160
}
161161

162-
result[name] = markdownflavor.Convention{
162+
result[name] = convention.Convention{
163163
Name: name,
164164
Flavor: fl,
165165
Rules: rules,

internal/config/convention_test.go

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,9 @@
11
package config
22

33
import (
4+
"go/parser"
5+
"go/token"
6+
"io/fs"
47
"os"
58
"path/filepath"
69
"strings"
@@ -16,6 +19,30 @@ import (
1619
_ "github.com/jeduden/mdsmith/internal/rules/nohardtabs"
1720
)
1821

22+
// TestConfigDoesNotImportRules guards the dependency direction
23+
// recorded in docs/development/architecture/index.md: rules sit at
24+
// the lowest layer, config sits above them, so config must not
25+
// import any rule package. The convention and flavor data types
26+
// live in internal/convention so this constraint can be enforced.
27+
// Test files are exempt because they need to register rules to
28+
// exercise rule.ByName lookups.
29+
func TestConfigDoesNotImportRules(t *testing.T) {
30+
fset := token.NewFileSet()
31+
pkgs, err := parser.ParseDir(fset, ".", func(fi fs.FileInfo) bool {
32+
return !strings.HasSuffix(fi.Name(), "_test.go")
33+
}, parser.ImportsOnly)
34+
require.NoError(t, err)
35+
for _, pkg := range pkgs {
36+
for fname, file := range pkg.Files {
37+
for _, imp := range file.Imports {
38+
path := strings.Trim(imp.Path.Value, `"`)
39+
assert.NotContains(t, path, "internal/rules/",
40+
"%s imports a rule package: %s", fname, path)
41+
}
42+
}
43+
}
44+
}
45+
1946
func TestApplyConvention_NoConventionSet_NoOp(t *testing.T) {
2047
cfg := &Config{
2148
Rules: map[string]RuleCfg{

internal/rules/markdownflavor/conventions.go renamed to internal/convention/convention.go

Lines changed: 10 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
package markdownflavor
1+
package convention
22

33
import (
44
"fmt"
@@ -32,8 +32,8 @@ type Convention struct {
3232

3333
// RulePreset is a convention's preset for a single rule. It mirrors
3434
// the shape of config.RuleCfg without depending on the config
35-
// package, so the markdownflavor package can declare convention
36-
// tables without the import cycle that would otherwise result.
35+
// package, so this package can declare convention tables without the
36+
// import cycle that would otherwise result.
3737
type RulePreset struct {
3838
Enabled bool
3939
Settings map[string]any
@@ -176,7 +176,7 @@ func Lookup(name string, userConventions map[string]Convention) (Convention, err
176176
}
177177
c, ok := conventions[name]
178178
if !ok {
179-
names := conventionNamesWithUser(userConventions)
179+
names := namesWithUser(userConventions)
180180
return Convention{}, fmt.Errorf(
181181
"unknown convention %q (valid: %s)",
182182
name, strings.Join(names, ", "),
@@ -185,10 +185,10 @@ func Lookup(name string, userConventions map[string]Convention) (Convention, err
185185
return cloneConvention(c), nil
186186
}
187187

188-
// conventionNamesWithUser returns a sorted list of all available
189-
// convention names — built-in names plus user-defined names.
190-
func conventionNamesWithUser(userConventions map[string]Convention) []string {
191-
names := ConventionNames()
188+
// namesWithUser returns a sorted list of all available convention
189+
// names — built-in names plus user-defined names.
190+
func namesWithUser(userConventions map[string]Convention) []string {
191+
names := Names()
192192
for name := range userConventions {
193193
names = append(names, name)
194194
}
@@ -259,9 +259,8 @@ func cloneValue(v any) any {
259259
}
260260
}
261261

262-
// ConventionNames returns the sorted list of built-in convention
263-
// names.
264-
func ConventionNames() []string {
262+
// Names returns the sorted list of built-in convention names.
263+
func Names() []string {
265264
names := make([]string, 0, len(conventions))
266265
for k := range conventions {
267266
names = append(names, k)

internal/rules/markdownflavor/conventions_test.go renamed to internal/convention/convention_test.go

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
package markdownflavor
1+
package convention
22

33
import (
44
"sort"
@@ -177,9 +177,9 @@ func TestLookup_UnknownListsUserAndBuiltin(t *testing.T) {
177177
assert.Contains(t, err.Error(), "portable", "error must list built-in name")
178178
}
179179

180-
func TestConventionNamesSorted(t *testing.T) {
181-
names := ConventionNames()
180+
func TestNamesSorted(t *testing.T) {
181+
names := Names()
182182
assert.True(t, sort.StringsAreSorted(names),
183-
"ConventionNames should return a sorted slice; got %v", names)
183+
"Names should return a sorted slice; got %v", names)
184184
assert.ElementsMatch(t, []string{"github", "plain", "portable"}, names)
185185
}

internal/convention/flavor.go

Lines changed: 101 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,101 @@
1+
// Package convention owns the convention and flavor data shapes
2+
// independent of any rule. A convention pairs a Markdown flavor with
3+
// a table of rule presets; the config loader consults this package
4+
// at load time so a top-level `convention:` selection becomes a base
5+
// layer beneath the user's own rule config. Rule packages (notably
6+
// internal/rules/markdownflavor) consume these data shapes — they do
7+
// not own them — which keeps internal/config from importing a rule.
8+
package convention
9+
10+
// Flavor identifies a target Markdown flavor.
11+
type Flavor int
12+
13+
// Flavor constants. The zero value is intentionally invalid so that
14+
// unparsed settings are caught.
15+
const (
16+
flavorInvalid Flavor = iota
17+
FlavorCommonMark
18+
FlavorGFM
19+
FlavorGoldmark
20+
// FlavorAny accepts every tracked feature. Useful when the
21+
// document is destined for an unknown or permissive renderer and
22+
// the user wants to disable flavor reporting without disabling
23+
// the rule.
24+
FlavorAny
25+
// FlavorPandoc is Pandoc's default markdown dialect. Accepts
26+
// GFM's four features plus footnotes, definition lists, heading
27+
// IDs, superscript, subscript, math block, and inline math;
28+
// rejects abbreviations (a non-default Pandoc extension).
29+
FlavorPandoc
30+
// FlavorPHPExtra is PHP Markdown Extra. Accepts tables,
31+
// footnotes, definition lists, heading IDs, and abbreviations;
32+
// rejects GFM's task lists, strikethrough, bare-URL autolinks,
33+
// and every math / sub/superscript feature.
34+
FlavorPHPExtra
35+
// FlavorMultiMarkdown extends PHP Markdown Extra with math
36+
// block and inline math. Like PHP Extra, rejects GFM task lists,
37+
// strikethrough, bare-URL autolinks, and sub/superscript.
38+
FlavorMultiMarkdown
39+
// FlavorMyST is the MyST flavor used by the Sphinx documentation
40+
// toolchain. Accepts tables, strikethrough, footnotes,
41+
// definition lists, heading IDs, math block, and inline math;
42+
// rejects GFM task lists, bare-URL autolinks, sub/superscript,
43+
// and abbreviations.
44+
FlavorMyST
45+
)
46+
47+
// IsValid reports whether f names a recognised flavor. The zero
48+
// value (reserved for "unparsed/unset") and any out-of-range integer
49+
// cast to Flavor both return false. Implemented in terms of String
50+
// so the two stay in lock-step: every recognised flavor has a name,
51+
// and adding a new constant only requires updating the switch in
52+
// String.
53+
func (f Flavor) IsValid() bool { return f.String() != "" }
54+
55+
// String returns the canonical lowercase name of the flavor.
56+
func (f Flavor) String() string {
57+
switch f {
58+
case FlavorCommonMark:
59+
return "commonmark"
60+
case FlavorGFM:
61+
return "gfm"
62+
case FlavorGoldmark:
63+
return "goldmark"
64+
case FlavorAny:
65+
return "any"
66+
case FlavorPandoc:
67+
return "pandoc"
68+
case FlavorPHPExtra:
69+
return "phpextra"
70+
case FlavorMultiMarkdown:
71+
return "multimarkdown"
72+
case FlavorMyST:
73+
return "myst"
74+
}
75+
return ""
76+
}
77+
78+
// ParseFlavor converts a config string into a Flavor. The match is
79+
// case-sensitive to reject typos like "GFM" that would otherwise
80+
// silently validate against the wrong flavor.
81+
func ParseFlavor(s string) (Flavor, bool) {
82+
switch s {
83+
case "commonmark":
84+
return FlavorCommonMark, true
85+
case "gfm":
86+
return FlavorGFM, true
87+
case "goldmark":
88+
return FlavorGoldmark, true
89+
case "any":
90+
return FlavorAny, true
91+
case "pandoc":
92+
return FlavorPandoc, true
93+
case "phpextra":
94+
return FlavorPHPExtra, true
95+
case "multimarkdown":
96+
return FlavorMultiMarkdown, true
97+
case "myst":
98+
return FlavorMyST, true
99+
}
100+
return 0, false
101+
}

0 commit comments

Comments
 (0)