fix: Skip files without metrics instead of adding empty arrays#30
Conversation
Resolves #23 Changes: - Modified parseFile() to return null for files without metrics (was empty array) - Updated parseClover() to filter out null values when building files array - Added comprehensive tests to verify files are skipped entirely - Test coverage remains at 100% - All code formatting passes Laravel Pint Impact: - Files without metrics are now skipped completely - Files array no longer contains empty elements - More accurate file counts in coverage reporting - Cleaner data structure for downstream consumers
WalkthroughThis PR applies consistent code style improvements across the codebase: converting parameterless constructor instantiations to short syntax (removing parentheses), adjusting string concatenation formatting by removing unnecessary spaces, and adding two new branded names. It also fixes the CoverageReporter to skip files without metrics rather than including empty arrays in the results. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
app/Stages/TechnicalGate.php (1)
13-13: Revert to standard PHPDoc formatting.The extra spaces around the type and parameter name are non-standard. PHPDoc conventions typically use single spaces:
@param array<CheckInterface> $checks.🔎 Suggested formatting fix
- * @param array<CheckInterface> $checks + * @param array<CheckInterface> $checkstests/Feature/TechnicalGateTest.php (1)
11-22: Consider the value of this formatting change.While expanding anonymous class definitions to multi-line format improves readability and consistency with standard class formatting, it creates significant diff noise (100+ lines) for a purely stylistic change with no functional benefit. For future PRs, consider whether such extensive formatting changes should be separated from functional changes to keep reviews focused.
Also applies to: 32-43, 54-77, 89-120, 129-140
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (28)
app/Branding.php(1 hunks)app/Checks/CheckInterface.php(1 hunks)app/Checks/PestSyntaxValidator.php(2 hunks)app/Checks/SecurityScanner.php(1 hunks)app/Checks/TestRunner.php(2 hunks)app/Commands/CertifyCommand.php(3 hunks)app/Commands/SecurityCommand.php(1 hunks)app/Commands/SyntaxCommand.php(1 hunks)app/GitHub/ChecksClient.php(2 hunks)app/GitHub/CommentsClient.php(2 hunks)app/Services/CoverageReporter.php(3 hunks)app/Services/PestOutputParser.php(1 hunks)app/Stages/TechnicalGate.php(1 hunks)app/Verdict.php(1 hunks)tests/Feature/RunCommandTest.php(1 hunks)tests/Feature/TechnicalGateTest.php(5 hunks)tests/Unit/Checks/PestSyntaxValidatorTest.php(3 hunks)tests/Unit/Checks/SecurityScannerTest.php(1 hunks)tests/Unit/Checks/TestRunnerTest.php(18 hunks)tests/Unit/Commands/CertifyCommandTest.php(1 hunks)tests/Unit/Commands/SecurityCommandTest.php(1 hunks)tests/Unit/Commands/SyntaxCommandTest.php(1 hunks)tests/Unit/Commands/TestsCommandTest.php(1 hunks)tests/Unit/GitHub/ChecksClientTest.php(1 hunks)tests/Unit/GitHub/CommentsClientTest.php(2 hunks)tests/Unit/Services/CoverageReporterTest.php(9 hunks)tests/Unit/Services/PestOutputParserTest.php(11 hunks)tests/Unit/Services/SymfonyProcessRunnerTest.php(5 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-12-17T20:12:44.062Z
Learnt from: CR
Repo: synapse-sentinel/gate PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-17T20:12:44.062Z
Learning: Applies to **/*.test.{php,ts,tsx,js,jsx} : All tests MUST use `describe()`/`it()` blocks instead of `test()` functions
Applied to files:
tests/Feature/RunCommandTest.phptests/Unit/Checks/SecurityScannerTest.phpapp/Checks/PestSyntaxValidator.phptests/Unit/Services/PestOutputParserTest.phptests/Unit/Checks/PestSyntaxValidatorTest.php
📚 Learning: 2025-12-17T20:12:44.062Z
Learnt from: CR
Repo: synapse-sentinel/gate PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-17T20:12:44.062Z
Learning: Applies to **/*.test.php : 100% code coverage required for all tests (enforced with `vendor/bin/pest --coverage --min=100`)
Applied to files:
app/Checks/PestSyntaxValidator.phptests/Unit/Services/PestOutputParserTest.phptests/Unit/Checks/TestRunnerTest.phptests/Unit/Checks/PestSyntaxValidatorTest.phpapp/Checks/TestRunner.phptests/Unit/Services/CoverageReporterTest.php
📚 Learning: 2025-12-17T20:12:44.062Z
Learnt from: CR
Repo: synapse-sentinel/gate PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-17T20:12:44.062Z
Learning: All quality checks must return a Verdict value object with states: APPROVED, REJECTED, or ESCALATE
Applied to files:
app/Verdict.php
🧬 Code graph analysis (17)
tests/Unit/Commands/SyntaxCommandTest.php (1)
app/Commands/SyntaxCommand.php (1)
SyntaxCommand(17-79)
tests/Feature/RunCommandTest.php (4)
app/Commands/TestsCommand.php (1)
TestsCommand(18-92)app/Commands/SecurityCommand.php (1)
SecurityCommand(17-75)app/Commands/SyntaxCommand.php (1)
SyntaxCommand(17-79)app/Commands/CertifyCommand.php (1)
CertifyCommand(21-217)
tests/Unit/Checks/SecurityScannerTest.php (2)
app/Checks/SecurityScanner.php (2)
SecurityScanner(10-50)name(16-19)app/Checks/CheckInterface.php (1)
name(9-9)
tests/Unit/Commands/CertifyCommandTest.php (1)
app/Commands/CertifyCommand.php (1)
CertifyCommand(21-217)
app/Checks/SecurityScanner.php (1)
app/Services/SymfonyProcessRunner.php (1)
SymfonyProcessRunner(11-24)
app/Checks/PestSyntaxValidator.php (1)
app/Checks/CheckResult.php (2)
CheckResult(7-24)pass(15-18)
tests/Unit/Checks/TestRunnerTest.php (2)
app/Services/PestOutputParser.php (1)
PestOutputParser(7-84)app/Checks/TestRunner.php (2)
run(41-62)TestRunner(11-132)
app/Commands/SyntaxCommand.php (1)
app/Checks/PestSyntaxValidator.php (1)
PestSyntaxValidator(9-47)
tests/Unit/Commands/SecurityCommandTest.php (1)
app/Commands/SecurityCommand.php (1)
SecurityCommand(17-75)
tests/Feature/TechnicalGateTest.php (4)
app/Stages/TechnicalGate.php (3)
TechnicalGate(10-37)run(19-36)__construct(15-17)app/Checks/CheckInterface.php (2)
name(9-9)run(11-11)app/Checks/CheckResult.php (3)
CheckResult(7-24)fail(20-23)__construct(9-13)app/Verdict.php (1)
__construct(12-16)
tests/Unit/Checks/PestSyntaxValidatorTest.php (2)
app/Checks/PestSyntaxValidator.php (3)
PestSyntaxValidator(9-47)name(11-14)run(16-46)app/Checks/CheckInterface.php (2)
name(9-9)run(11-11)
tests/Unit/Commands/TestsCommandTest.php (1)
app/Commands/TestsCommand.php (1)
TestsCommand(18-92)
app/Checks/TestRunner.php (2)
app/Services/PestOutputParser.php (1)
PestOutputParser(7-84)app/Services/SymfonyProcessRunner.php (1)
SymfonyProcessRunner(11-24)
app/Commands/SecurityCommand.php (1)
app/Checks/SecurityScanner.php (1)
SecurityScanner(10-50)
tests/Unit/Services/CoverageReporterTest.php (1)
app/Services/CoverageReporter.php (2)
CoverageReporter(9-148)parseClover(23-53)
tests/Unit/Services/SymfonyProcessRunnerTest.php (1)
app/Services/SymfonyProcessRunner.php (1)
SymfonyProcessRunner(11-24)
app/Commands/CertifyCommand.php (3)
app/Checks/SecurityScanner.php (1)
SecurityScanner(10-50)app/Checks/PestSyntaxValidator.php (1)
PestSyntaxValidator(9-47)app/Verdict.php (2)
failures(46-49)status(33-36)
🔇 Additional comments (30)
app/Verdict.php (2)
10-10: PHPDoc formatting aligned with Laravel Pint standards.The spacing adjustment in the @param annotation is consistent with automated PHP formatting.
12-16: Constructor formatting improved; no functional impact.The closing brace placement is now consistent with Laravel Pint formatting standards, and the constructor remains properly encapsulated as a private method on the readonly value object.
app/GitHub/ChecksClient.php (1)
130-158: LGTM - Formatting improvements.The added blank lines after the warning echo statements improve readability by visually separating the diagnostic output from the return statements. This aligns with the Laravel Pint formatting applied across the PR.
tests/Unit/GitHub/ChecksClientTest.php (1)
6-11: LGTM - Import organization.The imports are correctly organized with
RequestExceptionexplicitly imported from the Guzzle exceptions namespace. BothRequestExceptionandResponseare used throughout the test file, confirming these imports are necessary.app/Services/CoverageReporter.php (3)
42-45: LGTM! Core fix correctly implemented.The null-filtering logic properly excludes files without metrics from the results array, achieving the PR objective. The explicit null check is type-safe and clear.
55-55: LGTM! Return type correctly updated.The nullable return type (
?array) accurately reflects the new behavior wherenullis returned when a file lacks metrics.
59-59: LGTM! Correctly signals absence of metrics.Returning
nullinstead of an empty array provides a clear semantic distinction between "file has no metrics" (null) versus "file has metrics but no data" (empty arrays within the structure).tests/Unit/Services/CoverageReporterTest.php (2)
73-96: LGTM! Test correctly validates skipping behavior.The test properly verifies that files without a
<metrics>element are excluded from the results, confirming the core fix.
98-129: LGTM! Comprehensive test for mixed scenarios.The test effectively validates that files with metrics are correctly included while files without metrics are filtered out, even when interleaved. The assertions confirm both count accuracy and correct data extraction.
app/GitHub/CommentsClient.php (1)
72-72: LGTM - Formatting improvements.The added blank lines improve readability without any functional changes.
Also applies to: 110-110
tests/Unit/GitHub/CommentsClientTest.php (1)
66-66: LGTM - Consistent formatting.String concatenation formatting tightened without functional changes.
Also applies to: 77-77
tests/Unit/Commands/TestsCommandTest.php (1)
16-16: LGTM - Modern PHP syntax.Short-form instantiation is valid for parameterless constructors and aligns with the codebase modernization in this PR.
tests/Unit/Services/SymfonyProcessRunnerTest.php (1)
11-11: LGTM - Consistent short-form instantiation.All instances correctly use short-form syntax for the parameterless constructor.
Also applies to: 21-21, 30-30, 39-39, 48-48
app/Branding.php (1)
11-15: LGTM - New branding constants added.The new SECURITY and SYNTAX constants follow the existing pattern and align with the
checkName()mappings below.app/Checks/CheckInterface.php (1)
10-11: All CheckInterface implementations have been successfully updated with the run() method. Verification confirms:
- SecurityScanner ✓
- PestSyntaxValidator ✓
- TestRunner ✓
- Anonymous test classes in TechnicalGateTest ✓
No implementations are missing the required method.
app/Commands/SyntaxCommand.php (1)
1-79: Inconsistency: File unrelated to stated PR objectives.This file contains only a stylistic change (line 46: removing parentheses from zero-argument constructor instantiation). However, the PR objectives state this PR fixes Clover XML parsing to skip files without metrics—none of the provided files relate to CoverageReporter or Clover XML parsing.
The constructor syntax change itself is valid and harmless.
tests/Unit/Commands/CertifyCommandTest.php (1)
1-358: Inconsistency: Test file unrelated to stated PR objectives.This test file contains only a stylistic change (line 17: short-form constructor instantiation). The PR objectives describe fixing Clover XML parsing for files without metrics, but this file tests CertifyCommand functionality.
The style change is valid and has no behavioral impact.
tests/Unit/Services/PestOutputParserTest.php (1)
1-223: Inconsistency: Test file unrelated to stated PR objectives.This file contains only stylistic changes across multiple lines (10, 16, 22, 30, 36, 42, 48, 56, 71, 89, 102, 115, 126, 132, 145, 151, 157, 165, 180, 192, 204, 217), all converting
new PestOutputParser()tonew PestOutputParser.The PR objectives describe fixing Clover XML parsing to skip files without metrics, but this file tests PestOutputParser functionality unrelated to that goal.
All style changes are valid and have no behavioral impact.
tests/Feature/RunCommandTest.php (1)
1-82: Inconsistency: Feature test unrelated to stated PR objectives.This feature test file contains only stylistic changes across lines 13, 18, 26, 33, 38, 45, 50, 57, 62, 70, 75, converting parameterless constructor calls to short form for various command classes.
The PR objectives describe fixing Clover XML parsing to skip files without metrics. This file tests command signatures and options, which is unrelated to that objective.
All style changes are valid and have no behavioral impact.
tests/Unit/Commands/SyntaxCommandTest.php (1)
1-91: Inconsistency: Test file unrelated to stated PR objectives.This test file contains only a stylistic change (line 16: short-form constructor instantiation). The PR objectives describe fixing Clover XML parsing for files without metrics, but this file tests SyntaxCommand functionality.
The style change is valid and has no behavioral impact.
tests/Unit/Commands/SecurityCommandTest.php (1)
1-92: Inconsistency: Test file unrelated to stated PR objectives.This test file contains only a stylistic change (line 16: short-form constructor instantiation). The PR objectives describe fixing Clover XML parsing for files without metrics, but this file tests SecurityCommand functionality.
The style change is valid and has no behavioral impact.
app/Commands/SecurityCommand.php (1)
1-75: Inconsistency: File unrelated to stated PR objectives.This file contains only a stylistic change (line 46: removing parentheses from zero-argument constructor instantiation). The PR objectives state this PR fixes Clover XML parsing to skip files without metrics—this file handles security audit command logic.
The constructor syntax change is valid and harmless.
tests/Unit/Checks/SecurityScannerTest.php (1)
1-135: Inconsistency: Test file unrelated to stated PR objectives.This test file contains only stylistic changes (lines 11, 16: short-form constructor instantiation). The PR objectives describe fixing Clover XML parsing for files without metrics, but this file tests SecurityScanner functionality.
The style changes are valid and have no behavioral impact.
tests/Unit/Checks/TestRunnerTest.php (1)
32-32: LGTM: Consistent style improvements with no behavioral changes.The shift from
new PestOutputParser()tonew PestOutputParseris valid PHP 8+ syntax for parameterless constructors. All string concatenation updates consistently remove spaces around the.operator. These changes are purely stylistic and don't affect test behavior.Also applies to: 60-60, 82-82, 114-114, 139-139, 157-157, 185-185, 216-216, 238-238, 266-266, 308-308, 348-348, 390-390, 431-431
app/Services/PestOutputParser.php (1)
39-39: LGTM: Formatting-only change.Tightening the concatenation is consistent with the codebase-wide style update and doesn't alter the method's output.
tests/Unit/Checks/PestSyntaxValidatorTest.php (1)
9-9: LGTM: Consistent constructor and concatenation style updates.The parameterless constructor syntax and tightened concatenation align with the project-wide style improvements. No behavioral changes to the tests.
Also applies to: 14-14, 19-19, 44-44, 58-58
app/Checks/PestSyntaxValidator.php (1)
18-18: LGTM: Style-only updates.The concatenation tightening and
new Findersyntax are consistent with the broader style improvements. No functional changes.Also applies to: 24-24, 34-34
app/Checks/SecurityScanner.php (1)
13-13: LGTM: Valid constructor syntax update.The short-form instantiation
new SymfonyProcessRunneris valid PHP 8+ syntax and consistent with style changes across the codebase.app/Commands/CertifyCommand.php (1)
64-65: LGTM: Consistent style improvements.The short-form constructor instantiation and tightened concatenation are applied consistently with no behavioral changes to the command logic.
Also applies to: 103-103, 209-209
app/Checks/TestRunner.php (1)
21-22: LGTM: Style-only constructor and concatenation updates.The short-form instantiation for default parameters and tightened concatenation are consistent with the project-wide style improvements. No functional changes.
Also applies to: 43-43
1 participant
Summary
Fixes #23 - Files without metrics are now skipped entirely instead of being added as empty arrays to the files list.
Changes
parseFile()to return?array(nullable) instead ofarraynullwhen file has no metrics element (previously returned[])parseClover()to filter out null values when building files arrayImpact
formatMarkdown()was already handling this correctlyTesting
Test Plan
./vendor/bin/pest --coverage --min=100./vendor/bin/pintSummary by CodeRabbit
Release Notes
New Features
Bug Fixes
Style
✏️ Tip: You can customize this high-level summary in your review settings.