feat(generate): make --use-templates-var variadic

[crhntr]

resolves: #67

[claude]

Code Review: feat(generate): make --use-templates-var variadic

Summary

This PR successfully implements support for multiple template variables via the --use-templates-variable flag, allowing users to work with multiple template collections (e.g., admin and public templates). The implementation is well-structured with comprehensive test coverage.

---

✅ Strengths

1. Excellent Test Coverage: The PR includes 4 new test files covering various scenarios:

   • reference_multiple_templates_variables.txt - Happy path
   • err_duplicate_route_different_variables.txt - Error handling for duplicates
   • err_template_in_shared_file_unused_by_one_variable.txt - Edge case with shared files
   • reference_cross_package_embed_fs.txt - Cross-package usage

2. Backward Compatibility: The deprecated flag handling in fixTemplateVariables() is well done, ensuring existing users won't experience breaking changes.

3. Pattern Normalization: The addition of Pattern(), HTTPMethod(), Host(), and Path() methods with proper normalization (trimming, case conversion) is a significant improvement that prevents subtle bugs.

4. Duplicate Detection: Moving duplicate pattern detection to CheckForDuplicatePatterns() function provides better error messages, especially when duplicates span multiple files.

---

🔍 Issues & Concerns

1. Breaking API Change (internal/muxt/definition.go:109)

-func (def Definition) Pattern() string { return def.pattern }
+func (def Definition) RawPattern() string { return def.pattern }
+func (def Definition) Pattern() string { /* normalized version */ }

The Pattern() method now returns a normalized pattern instead of the raw value. This changes behavior for existing callers. Consider:

• Is this intentional?
• Could this break existing code that depends on the raw pattern format?

Recommendation: Document this breaking change or ensure all callers are updated to use RawPattern() where needed.

---

2. Potential Inefficiency in Pattern Normalization (internal/muxt/definition.go:112-126)

The Pattern() method allocates a strings.Builder and performs string operations on every call. This could be expensive if called frequently in loops.

Recommendation: Consider caching the normalized pattern:

type Definition struct {
    pattern           string // raw pattern
    normalizedPattern string // cached normalized pattern
    // ... other fields
}

Pre-compute during newDefinition() to avoid repeated allocations.

---

3. Empty Host Handling (internal/muxt/definition.go:120-122)

if h := def.Host(); h != "" {
    sb.WriteString(h)
}
sb.WriteString(def.Path())

When a host is present, there's no separator between host and path. For pattern "GET example.com/", this produces "GET example.com/" correctly, but the logic seems fragile.

Question: Should there be explicit validation that paths start with / when a host is present?

---

4. Inconsistent Error Messages (internal/cli/commands.go:704, 720)

return fmt.Errorf("deprecated flag %s not permitted along with %s", ...)
return fmt.Errorf("duplicate template variable: %s", tv)

vs.

return fmt.Errorf("variable %s%s", tv, errIdentSuffix)  // line 815

Recommendation: Standardize error message format for consistency.

---

5. Missing Validation (internal/cli/commands.go:713)

findDuplicateVariables() checks for duplicate variables, but there's no validation that variables are non-empty strings.

Recommendation: Add validation:

for _, tv := range in {
    if tv == "" {
        return fmt.Errorf("template variable cannot be empty")
    }
    if _, ok := seen[tv]; ok {
        return fmt.Errorf("duplicate template variable: %s", tv)
    }
    seen[tv] = struct{}{}
}

---

6. Code Duplication in Analysis Functions

The changes to internal/analysis/check.go, template_callers.go, and template_calls.go follow nearly identical patterns - wrapping existing logic in a for _, tv := range config.TemplatesVariables loop.

Recommendation: Consider extracting a higher-order function to reduce duplication:

func forEachTemplateVariable(
    config Config,
    wd string,
    pkg *packages.Package,
    fn func(tv string, ts *template.Template, ...) error,
) error {
    for _, tv := range config.TemplatesVariables {
        ts, fm, err := asteval.Templates(wd, tv, pkg)
        if err != nil {
            return err
        }
        if err := fn(tv, ts, fm); err != nil {
            return err
        }
    }
    return nil
}

---

7. Regex Pattern Cleanup (internal/muxt/definition.go:235)

-templateNameMux = regexp.MustCompile(`^(?P<pattern>(((?P<METHOD>[A-Z]+)\s+)?)...)`)
+templateNameMux = regexp.MustCompile(`^(?P<pattern>((?P<METHOD>[A-Z]+)\s+)?...)`)

The removal of the extra grouping ((...)) is good, but the change isn't mentioned in the PR description.

Question: Was this intentional? Does it affect capture groups?

---

8. Comment Typo (docs/links_test.go:354)

-// Pattern matches markdown links: [text](path)
+// RawPattern matches markdown links: [text](path)

This seems like an unintentional rename. The variable is still linkPattern, so the comment should remain Pattern.

Recommendation: Revert this change or update the variable name to rawLinkPattern.

---

🔒 Security Considerations

No significant security concerns identified. The changes:

• Don't introduce command injection risks
• Properly validate identifiers using token.IsIdentifier()
• Don't expose sensitive information in error messages

---

🎯 Performance Considerations

1. Pattern Normalization: As mentioned, repeated Pattern() calls could be expensive. Caching would help.

2. Map Allocations: CheckForDuplicatePatterns() allocates a new map each time. For large template sets, this could add up. Pre-allocating with capacity would help:

patterns := make(map[string]Definition, len(templates))

3. Loop Overhead: The per-variable loops in analysis functions mean AST traversal happens multiple times. This is necessary for correctness but could be slow with many variables.

---

📝 Documentation & Testing

Strengths:

• Test files follow the project's excellent txtar convention
• Tests cover error cases thoroughly
• Cross-package test demonstrates real-world usage

Suggestions:

• Add a comment explaining why NeedImports was added to packages.Load (internal/asteval/package.go:738)
• Document the normalization behavior in the Pattern() method
• Add a test for multiple variables with overlapping but non-duplicate patterns

---

🎓 Best Practices Alignment

The PR follows the project's TDD workflow well:

• Tests were added/updated
• Generator logic was modified
• Deterministic output is maintained

However, per CLAUDE.md, consider whether the regex change deserves its own test case.

---

✅ Recommendations Summary

High Priority:

1. Clarify/document the Pattern() behavior change (breaking change?)
2. Add empty string validation for template variables
3. Fix the unintentional comment change in docs/links_test.go

Medium Priority:
4. Cache normalized patterns to avoid repeated allocations
5. Pre-allocate maps with capacity hints
6. Standardize error message formats
7. Consider extracting common loop patterns in analysis code

Low Priority:
8. Add validation for host/path separator logic
9. Document the NeedImports addition

---

🎉 Conclusion

This is a solid implementation that maintains backward compatibility while adding useful functionality. The test coverage is exemplary. With the minor issues addressed, this PR will be production-ready.

Overall Assessment: ✅ Approve with minor revisions suggested

---

Review conducted following CLAUDE.md guidelines with focus on TDD workflow, deterministic generation, and type safety.