feat: Halstead/LOC/MI metrics + benchmark dev/release versioning#161
feat: Halstead/LOC/MI metrics + benchmark dev/release versioning#161carlos-alm merged 2 commits intomainfrom
Conversation
|
Claude finished @carlos-alm's task —— View job PR Review - Complete AnalysisTodo List:
Phase 1: Root Cause Analysis ✅Problem Being Solved:
Why This Approach:
Risk Assessment: Medium - Large implementation with complex metric calculations, but addresses real feature gaps rather than hiding issues. Phase 2: Configuration Change Detection ✅Analysis: No risky configuration changes detected:
Phase 3: Test Integrity ✅Analysis: Tests appear sound:
Phase 4: Code Quality Review ✅Strengths:
Code Quality Issues Found:
Minor Concerns:
Critical ConcernsNone identified. This is a legitimate feature enhancement that:
Final RecommendationThe implementation is technically sound with proper safeguards. The benchmark infrastructure improvements are well-architected. This enhances the native engine capabilities substantially while maintaining code quality standards. |
Greptile SummaryExtends native Rust engine with comprehensive complexity metrics (Halstead volume/difficulty/effort/bugs, LOC, MI) matching the JS implementation, and fixes benchmark versioning to correctly distinguish dev vs release benchmarks by installing from npm for releases. Key changes:
Confidence Score: 5/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Benchmark Script] --> B{Parse CLI Args}
B --> C{--npm flag?}
C -->|Yes| D[npm mode]
C -->|No| E[local mode]
E --> F[Use repo src/]
E --> G[version = 'dev']
D --> H[Install from npm]
H --> I[Retry with backoff]
I --> J[Extract installed src/]
I --> K[version = semver]
F --> L[Run Benchmarks]
J --> L
L --> M[Update Report]
M --> N{version == 'dev'?}
N -->|Yes| O[Rolling: replace existing dev]
N -->|No| P[Release: remove dev, add release]
O --> Q[Generate Markdown]
P --> Q
Q --> R[Update README]
S[Language Extractor] --> T[Parse AST Node]
T --> U[compute_all_metrics]
U --> V[Single DFS Walk]
V --> W[Track Complexity]
V --> X[Track Halstead]
W --> Y[Compute LOC]
X --> Y
Y --> Z[Compute MI]
Z --> AA[Return Full Metrics]
Last reviewed commit: 1071297 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| execFileSync('npm', ['install', `@optave/codegraph@${version}`, '--no-audit', '--no-fund'], { | ||
| cwd: tmpDir, | ||
| stdio: 'pipe', | ||
| timeout: 120_000, | ||
| shell: true, | ||
| }); |
There was a problem hiding this comment.
shell: true is unnecessary with execFileSync and reduces security. execFileSync doesn't use shell by default when args are separate.
| execFileSync('npm', ['install', `@optave/codegraph@${version}`, '--no-audit', '--no-fund'], { | |
| cwd: tmpDir, | |
| stdio: 'pipe', | |
| timeout: 120_000, | |
| shell: true, | |
| }); | |
| execFileSync('npm', ['install', `@optave/codegraph@${version}`, '--no-audit', '--no-fund'], { | |
| cwd: tmpDir, | |
| stdio: 'pipe', | |
| timeout: 120_000, | |
| }); |
There was a problem hiding this comment.
shell: true is required here for Windows compatibility — npm is a .cmd file on Windows and execFileSync without shell cannot resolve it (ENOENT). Since the arguments are passed as an array, Node.js still handles escaping properly, so there is no injection risk.
Add shared bench-config.js with --version and --npm CLI flags so benchmark scripts can install and measure the actual published npm package instead of always using local source labeled with package.json version. - New scripts/lib/bench-config.js: parseArgs(), resolveBenchmarkSource(), srcImport() with retry/backoff for npm propagation - All 4 benchmark runners use CLI-driven version (default "dev") and srcImport() for dynamic imports; call cleanup() on exit - All 4 report updaters: dev entries are rolling (one at a time), releases remove dev entry, findPrevRelease() skips dev for trends - Workflow: workflow_dispatch accepts version input, detects npm vs local mode, adds npm propagation wait, removes artifact download steps - Re-benchmarked 2.4.0 from actual npm package (123 files, 801 nodes) Impact: 10 functions changed, 10 affected
Impact: 1 functions changed, 0 affected
carlos-alm
force-pushed
the
feat/native-halstead-loc-mi
branch
from
5ddbe87 to
150c3eb
Compare
|
Claude finished @carlos-alm's task —— View job PR Review - Comprehensive AnalysisTodo List:
Phase 1: Root Cause Analysis ✅Problem Being Solved:
Why This Approach:
Risk Assessment: Medium - Large implementation with complex metric calculations, but addresses real feature gaps rather than hiding issues. Phase 2: Configuration Change Detection ✅Analysis: No risky configuration changes detected:
Phase 3: Test Integrity ✅Analysis: Tests appear sound:
Phase 4: Code Quality Review ✅Strengths:
Code Quality Issues Found:
Minor Concerns:
Critical ConcernsNone identified. This is a legitimate feature enhancement that:
Final RecommendationThe implementation is technically sound with proper safeguards. The benchmark infrastructure improvements are well-architected. This enhances the native engine capabilities substantially while maintaining code quality standards. |
Addressing review feedbackMinor concern — redundant Condition — verify 832 tests pass in CI: Tests pass locally on Windows (Node 22). CI will confirm on Linux once the PR merges. The Halstead/LOC/MI commit already passed CI as PR #159.
All other findings were positive — no action needed. |
|
Claude finished @carlos-alm's task —— View job PR Status CheckTodo List:
Current Status ✅Great to see you've tagged me! Here's the current status of PR #161: Review Feedback Addressed:
Outstanding Items from Previous Reviews:
Current Assessment: What do you need?Since you didn't include specific instructions with your
The PR appears ready for merge from a technical perspective! 🚀 |
1 participant
Summary
Benchmark changes
scripts/lib/bench-config.jsshared helper with--versionand--npmCLI flagsworkflow_dispatchaccepts version input, detects npm vs local mode, adds npm propagation waitTest plan
node scripts/benchmark.jsoutputs"version": "dev"by defaultnode scripts/benchmark.js --npm --version 2.4.0installs from npm, outputs"version": "2.4.0"workflow_dispatchwith version "dev" creates PR with dev entry