Skip to content

refactor: consolidate CFG rules with defaults factory + validation#284

Merged
carlos-alm merged 1 commit intomainfrom
fix/cfg-rule-consolidation
Mar 3, 2026
Merged

refactor: consolidate CFG rules with defaults factory + validation#284
carlos-alm merged 1 commit intomainfrom
fix/cfg-rule-consolidation

Conversation

@carlos-alm
Copy link
Contributor

Summary

  • Add CFG_DEFAULTS + makeCfgRules(overrides) factory in src/cfg.js that validates unknown keys and required fields (functionNodes, forNodes), catching typos and missing fields at startup
  • Rewrite all 8 language rule objects to only specify non-default fields, cutting ~120 lines of repetitive null assignments
  • Derive CFG_LANG_IDS from CFG_RULES.keys() instead of a manually duplicated set

Addresses feedback from PR #283.

Test plan

  • All 67 existing CFG tests pass unchanged
  • 5 new makeCfgRules validation tests (unknown key, missing/empty functionNodes, non-Set forNodes, defaults filled in)
  • npm run lint clean (pre-existing warning in unrelated file only)

@greptile-apps
Copy link

greptile-apps bot commented Mar 3, 2026

Greptile Summary

Consolidated CFG rule definitions using a factory pattern with defaults and validation. Added CFG_DEFAULTS object and makeCfgRules(overrides) factory that validates unknown keys, missing/empty functionNodes, and non-Set forNodes types. Rewrote all 8 language configurations to only specify non-default fields, eliminating ~126 lines of repetitive null assignments. Derived CFG_LANG_IDS from CFG_RULES.keys() instead of maintaining a duplicate list.

Key improvements:

  • Startup validation catches typos and missing required fields immediately
  • Reduced code duplication across language configs
  • Eliminated manual maintenance of CFG_LANG_IDS
  • Fixed C# tree-sitter node names (for_each_statementforeach_statement) and standardized language ID (c_sharpcsharp)

All 67 existing CFG tests pass, plus 5 new validation tests.

Confidence Score: 5/5

  • This PR is safe to merge with minimal risk
  • Well-structured refactoring with comprehensive test coverage (67 existing + 5 new validation tests). The factory pattern improves maintainability and catches configuration errors at startup. All language configs correctly preserve functionality while eliminating duplication. C# fixes address real tree-sitter node naming issues.
  • No files require special attention

Important Files Changed

Filename Overview
src/cfg.js Added factory pattern with defaults and validation, consolidated 8 language configs by removing ~126 lines of repetitive null assignments
src/complexity.js Fixed C# node name typo (for_each_statement → foreach_statement), standardized language ID (c_sharp → csharp), corrected else handling pattern
tests/unit/cfg.test.js Added 5 validation tests for makeCfgRules factory (unknown keys, missing/empty functionNodes, invalid forNodes types)
tests/unit/complexity.test.js Updated test assertions to use standardized csharp language ID instead of c_sharp

Last reviewed commit: 338197a

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

4 files reviewed, no comments

Edit Code Review Agent Settings | Greptile

Replace 8 verbose language rule objects (each ~30 lines with explicit
null fields) with a makeCfgRules(overrides) factory that merges onto
CFG_DEFAULTS. Each language now only specifies non-default fields,
cutting ~120 lines. The factory validates unknown keys (catches typos)
and requires functionNodes (non-empty Set) and forNodes (Set).

Derive CFG_LANG_IDS from CFG_RULES.keys() instead of duplicating.

Impact: 1 functions changed, 0 affected
@carlos-alm carlos-alm force-pushed the fix/cfg-rule-consolidation branch from 338197a to 9f8b547 Compare March 3, 2026 06:58
@claude
Copy link

claude bot commented Mar 3, 2026

Claude finished @carlos-alm's task —— View job


PR Review Complete ✅

PR #284: "refactor: consolidate CFG rules with defaults factory + validation"

Root Cause Analysis

Problem Being Solved: Excessive code duplication in CFG language rule definitions. Each of the 8 language configs (~30 lines each) had repetitive null assignments for unused fields, totaling ~240 lines where only ~120 were meaningful.

Why This Approach: Introduces a factory pattern with CFG_DEFAULTS and makeCfgRules(overrides) that:

  1. Eliminates repetitive null assignments by providing sensible defaults
  2. Adds startup validation to catch configuration typos and missing required fields
  3. Auto-derives CFG_LANG_IDS from the rule map keys instead of manual duplication

Risk Assessment: LOW RISK - This is a pure refactor that consolidates existing logic without changing behavior.

Configuration Change Detection

NO HIGH-RISK CHANGES DETECTED

  • No linting rules disabled or relaxed
  • No test timeout increases or coverage threshold reductions
  • No build configuration modifications
  • Pure code consolidation refactor with validation improvements

Test Integrity Verification

TESTS STRENGTHENED, NOT WEAKENED

  • All existing 67 CFG tests remain unchanged and continue to pass
  • 5 NEW VALIDATION TESTS ADDED for makeCfgRules() factory
  • No test expectations lowered or assertions weakened
  • New tests provide comprehensive validation coverage

Code Quality Review

HIGH QUALITY IMPLEMENTATION

Positive Aspects:

  • Excellent validation: Factory catches typos (unknown key), missing required fields, and type errors at startup
  • Clean abstraction: CFG_DEFAULTS eliminates ~126 lines of repetitive null assignments
  • Type safety: Validates functionNodes is non-empty Set and forNodes is Set
  • DRY principle: Auto-derives CFG_LANG_IDS from CFG_RULES.keys() instead of manual duplication
  • Maintainability: Each language config is now ~15 lines vs ~30 (50% reduction)
  • Comprehensive testing: New validation tests cover all error conditions and success cases

Implementation Quality:

  • Factory function at src/cfg.js:53-67 is well-structured and defensive
  • Error messages are clear and actionable
  • Object spread syntax { ...CFG_DEFAULTS, ...overrides } is clean and readable
  • All 8 language configs successfully consolidated without behavior changes

Security & Performance:

  • No security concerns identified
  • Performance impact negligible (one-time factory calls at module initialization)
  • No changes to runtime CFG building algorithms

Critical Concerns

None identified. This is a well-executed refactoring that improves code quality without introducing risks.

Final Recommendation

  • Rating: ⭐⭐⭐⭐⭐ (5/5)
  • Action: APPROVE
  • Reasoning: Exemplary refactoring that eliminates code duplication, adds valuable validation, improves maintainability, and includes comprehensive test coverage. All quality gates passed with no red flags detected. This addresses real technical debt while making future CFG rule maintenance significantly easier.

@carlos-alm carlos-alm merged commit c76de21 into main Mar 3, 2026
23 of 25 checks passed
@carlos-alm carlos-alm deleted the fix/cfg-rule-consolidation branch March 3, 2026 07:08
@github-actions github-actions bot locked and limited conversation to collaborators Mar 3, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant