refactor: implement object-oriented validation architecture with unified rendering system#18
Conversation
WalkthroughThis update introduces a major refactor of the translation validation system. It adds new classes to encapsulate file sets, issues, validation results, and output rendering. The validation execution is centralized in a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ValidateTranslationCommand
participant ValidationRun
participant Validator(s)
participant ValidationResult
participant Output
participant Renderer
User->>ValidateTranslationCommand: execute()
ValidateTranslationCommand->>ValidationRun: createFileSetsFromArray()
ValidateTranslationCommand->>ValidationRun: executeFor(fileSets, validators)
ValidationRun->>Validator(s): validate(files)
Validator(s)->>ValidationRun: return issues
ValidationRun->>ValidationResult: aggregate results
ValidateTranslationCommand->>Output: new Output(validationResult)
Output->>Renderer: render(validationResult)
Renderer->>User: display results
Possibly related PRs
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/src/Validator/AbstractValidatorTest.php (1)
293-318: Remove unused variable to address static analysis warning.The test logic is sound but the
$resultvariable is assigned but never used.Apply this diff to remove the unused variable:
// Call validate - this should reset state $files = ['/path/to/some_file.xlf']; $parserClass = TestParser::class; -$result = $validator->validate($files, $parserClass); +$validator->validate($files, $parserClass);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
src/Command/ValidateTranslationCommand.php(3 hunks)src/FileDetector/FileSet.php(1 hunks)src/Result/Issue.php(1 hunks)src/Result/Output.php(2 hunks)src/Result/ValidationResult.php(1 hunks)src/Result/ValidationResultCliRenderer.php(1 hunks)src/Result/ValidationResultJsonRenderer.php(1 hunks)src/Result/ValidationRun.php(1 hunks)src/Validator/AbstractValidator.php(4 hunks)src/Validator/DuplicateValuesValidator.php(3 hunks)src/Validator/MismatchValidator.php(4 hunks)src/Validator/ValidatorInterface.php(2 hunks)tests/src/FileDetector/FileSetTest.php(1 hunks)tests/src/Result/IssueTest.php(1 hunks)tests/src/Result/OutputTest.php(4 hunks)tests/src/Result/ValidationResultCliRendererTest.php(1 hunks)tests/src/Result/ValidationResultJsonRendererTest.php(1 hunks)tests/src/Result/ValidationResultTest.php(1 hunks)tests/src/Result/ValidationRunTest.php(1 hunks)tests/src/Validator/AbstractValidatorTest.php(4 hunks)tests/src/Validator/DuplicateValuesValidatorTest.php(2 hunks)tests/src/Validator/MismatchValidatorTest.php(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (5)
src/Validator/DuplicateValuesValidator.php (4)
src/Result/Issue.php (1)
Issue(7-55)src/Validator/AbstractValidator.php (2)
addIssue(109-112)resetState(118-121)src/Validator/ValidatorInterface.php (1)
addIssue(48-48)src/Validator/MismatchValidator.php (1)
resetState(130-134)
src/Command/ValidateTranslationCommand.php (1)
src/Result/ValidationRun.php (3)
ValidationRun(12-67)createFileSetsFromArray(53-66)executeFor(23-46)
tests/src/Validator/MismatchValidatorTest.php (4)
src/Result/Issue.php (1)
toArray(46-54)src/Validator/MismatchValidator.php (1)
MismatchValidator(16-135)src/Validator/AbstractValidator.php (1)
hasIssues(96-99)src/Validator/ValidatorInterface.php (1)
hasIssues(41-41)
tests/src/Validator/DuplicateValuesValidatorTest.php (4)
src/Result/Issue.php (1)
toArray(46-54)src/Validator/DuplicateValuesValidator.php (1)
DuplicateValuesValidator(17-126)src/Validator/AbstractValidator.php (1)
hasIssues(96-99)src/Validator/ValidatorInterface.php (1)
hasIssues(41-41)
src/Result/ValidationResult.php (4)
src/Result/Issue.php (2)
__construct(12-18)toArray(46-54)src/FileDetector/FileSet.php (3)
__construct(12-18)getPath(25-28)getSetKey(30-33)src/Validator/AbstractValidator.php (3)
__construct(17-19)hasIssues(96-99)getIssues(104-107)src/Validator/ValidatorInterface.php (2)
hasIssues(41-41)getIssues(46-46)
🪛 PHPMD (2.15.0)
tests/src/Result/ValidationRunTest.php
194-194: Avoid unused parameters such as '$files'. (Unused Code Rules)
(UnusedFormalParameter)
194-194: Avoid unused parameters such as '$parserClass'. (Unused Code Rules)
(UnusedFormalParameter)
202-202: Avoid unused parameters such as '$file'. (Unused Code Rules)
(UnusedFormalParameter)
207-207: Avoid unused parameters such as '$input'. (Unused Code Rules)
(UnusedFormalParameter)
207-207: Avoid unused parameters such as '$output'. (Unused Code Rules)
(UnusedFormalParameter)
207-207: Avoid unused parameters such as '$issueSets'. (Unused Code Rules)
(UnusedFormalParameter)
236-236: Avoid unused parameters such as '$issue'. (Unused Code Rules)
(UnusedFormalParameter)
252-252: Avoid unused parameters such as '$files'. (Unused Code Rules)
(UnusedFormalParameter)
252-252: Avoid unused parameters such as '$parserClass'. (Unused Code Rules)
(UnusedFormalParameter)
260-260: Avoid unused parameters such as '$file'. (Unused Code Rules)
(UnusedFormalParameter)
265-265: Avoid unused parameters such as '$input'. (Unused Code Rules)
(UnusedFormalParameter)
265-265: Avoid unused parameters such as '$output'. (Unused Code Rules)
(UnusedFormalParameter)
265-265: Avoid unused parameters such as '$issueSets'. (Unused Code Rules)
(UnusedFormalParameter)
294-294: Avoid unused parameters such as '$issue'. (Unused Code Rules)
(UnusedFormalParameter)
tests/src/Result/ValidationResultTest.php
226-226: Avoid unused parameters such as '$issue'. (Unused Code Rules)
(UnusedFormalParameter)
230-230: Avoid unused parameters such as '$files'. (Unused Code Rules)
(UnusedFormalParameter)
230-230: Avoid unused parameters such as '$parserClass'. (Unused Code Rules)
(UnusedFormalParameter)
235-235: Avoid unused parameters such as '$file'. (Unused Code Rules)
(UnusedFormalParameter)
240-240: Avoid unused parameters such as '$input'. (Unused Code Rules)
(UnusedFormalParameter)
240-240: Avoid unused parameters such as '$output'. (Unused Code Rules)
(UnusedFormalParameter)
240-240: Avoid unused parameters such as '$issueSets'. (Unused Code Rules)
(UnusedFormalParameter)
349-349: Avoid unused parameters such as '$issue'. (Unused Code Rules)
(UnusedFormalParameter)
353-353: Avoid unused parameters such as '$files'. (Unused Code Rules)
(UnusedFormalParameter)
353-353: Avoid unused parameters such as '$parserClass'. (Unused Code Rules)
(UnusedFormalParameter)
358-358: Avoid unused parameters such as '$file'. (Unused Code Rules)
(UnusedFormalParameter)
363-363: Avoid unused parameters such as '$input'. (Unused Code Rules)
(UnusedFormalParameter)
363-363: Avoid unused parameters such as '$output'. (Unused Code Rules)
(UnusedFormalParameter)
363-363: Avoid unused parameters such as '$issueSets'. (Unused Code Rules)
(UnusedFormalParameter)
tests/src/Validator/AbstractValidatorTest.php
311-311: Avoid unused local variables such as '$result'. (Unused Code Rules)
(UnusedLocalVariable)
🔇 Additional comments (52)
src/FileDetector/FileSet.php (1)
1-42: LGTM! Clean data class implementation.The
FileSetclass is well-structured with proper immutability using readonly properties, clear type annotations, and simple getters. This design aligns perfectly with the object-oriented refactoring goals.tests/src/Result/OutputTest.php (3)
34-42: Test correctly updated for ValidationResult architecture.The test properly creates a
ValidationResultinstance and passes it to theOutputconstructor, aligning with the new object-oriented validation result handling.
52-60: Test correctly updated for ValidationResult architecture.The JSON format test properly uses the new
ValidationResultobject structure.
75-83: Test correctly updated for ValidationResult architecture.The failure test case properly uses the new
ValidationResultobject withResultType::ERROR.src/Validator/DuplicateValuesValidator.php (3)
10-10: Correctly imported Issue class.The import is properly added to support the new object-oriented issue handling.
56-61: Proper Issue object construction.The Issue object is correctly instantiated with the file name, duplicates data, empty parser string (appropriate for validator-level issues), and validator type. This aligns with the new object-oriented architecture.
116-120: Correct resetState() implementation.The method properly calls the parent's resetState() and resets the validator's internal state (
valuesArray). This follows the established pattern from other validators and ensures clean state between validation runs.src/Validator/ValidatorInterface.php (2)
8-8: Correctly imported Issue class.The import is properly added to support the new Issue-based method signatures.
41-48: Well-designed interface extension for issue management.The three new methods provide a clean API for issue management:
hasIssues()for checking if issues existgetIssues()for retrieving Issue objectsaddIssue()for adding new issuesThe method signatures are clear and follow good interface design principles.
tests/src/Validator/DuplicateValuesValidatorTest.php (3)
134-135: Correctly updated expected data structure.The test properly includes the new
parserandtypefields that are part of the Issue object structure, aligning with the object-oriented refactoring.Also applies to: 142-143
147-147: Proper assertion for Issue objects.The assertion correctly uses
array_mapwithtoArray()to compare the Issue objects, which is the appropriate way to test the new object-oriented structure.
174-206: Comprehensive test for resetState() method.The test thoroughly verifies that
resetState()properly clears both the validator's internalvaluesArrayand the inherited issues. The use of reflection to test private methods and properties is appropriate for ensuring proper state management.tests/src/FileDetector/FileSetTest.php (1)
1-94: Excellent comprehensive test coverage for FileSet class.The test suite thoroughly covers all aspects of the FileSet class including:
- Constructor validation with all parameters
- Individual getter method testing
- Edge cases with empty strings and arrays
- Real-world data scenarios
- Proper PHPUnit structure and naming conventions
The tests provide good confidence in the FileSet implementation and follow testing best practices.
tests/src/Result/IssueTest.php (1)
1-112: Comprehensive test coverage for the Issue class.The test suite excellently covers all aspects of the Issue class:
- Constructor validation with all parameters
- Individual getter method testing
toArray()method with various complexity levels (empty, simple, complex nested structures)- Proper assertions and test structure
The tests for
toArray()are particularly well-designed, covering edge cases and complex data structures that will likely be encountered in real-world usage.tests/src/Validator/MismatchValidatorTest.php (3)
89-104: Proper adaptation to new Issue class structure.The expected issues structure has been correctly updated to match the new Issue class format, including the nested
'issues'key and additional metadata fields ('file','parser','type').
126-126: Correct assertion adaptation for Issue objects.The assertion correctly maps Issue objects to their array representation using
toArray(), which is the appropriate way to test the new Issue-based structure.
156-183: Valuable addition: resetState test for proper state management.This new test ensures that the
resetState()method properly clears both the validator-specific$keyArrayand the inherited issues collection. This is crucial for validators that might be reused across multiple validation runs.The test correctly uses reflection to access protected members and verify internal state, which is appropriate for testing state management functionality.
src/Result/Issue.php (1)
1-55: Well-designed Issue class with proper encapsulation.The Issue class demonstrates excellent object-oriented design:
- Immutability: Uses readonly properties to ensure data integrity
- Type safety: Proper type declarations and docblocks for array types
- Encapsulation: Private properties with public getter methods
- Interoperability:
toArray()method for legacy compatibility and serialization- Clear structure: Well-organized with logical method ordering
The class effectively replaces the previous ad-hoc array structures with a proper value object, improving type safety and maintainability throughout the validation system.
tests/src/Result/ValidationResultJsonRendererTest.php (1)
1-207: Comprehensive and well-structured JSON renderer test suite.This test suite excellently covers all aspects of the ValidationResultJsonRenderer:
Coverage highlights:
- Success scenarios: No issues, proper exit codes and JSON structure
- Error scenarios: With issues, proper error reporting and exit codes
- Mode variations: Dry run, strict mode, warning handling
- Output validation: JSON format validation, proper indentation, valid JSON structure
- Legacy compatibility: Ensures output structure matches expected format
Implementation quality:
- Proper use of PHPUnit features (setUp, mocking, assertions)
- BufferedOutput for clean output capture
- Helper methods for maintainable test code
- Comprehensive assertions covering both functional and structural aspects
The tests provide strong confidence in the JSON renderer's behavior across all expected usage scenarios.
src/Command/ValidateTranslationCommand.php (3)
12-12: Import addition aligns with the architectural refactoring.The addition of the
ValidationRunimport supports the new object-oriented validation architecture.
130-133: Excellent refactoring that eliminates complex nested loops.The replacement of the previous 4-level nested loop with this clean object-oriented approach significantly improves maintainability and code clarity:
ValidationRun::createFileSetsFromArray()converts raw arrays to structuredFileSetobjectsValidationRun::executeFor()centralizes validation logic and result aggregation- The
ValidationResultobject encapsulates all validation outcomesThis change aligns perfectly with the PR objectives of implementing a modern object-oriented validation architecture.
148-148: Constructor parameter change aligns with the unified result system.Passing the
ValidationResultobject instead of separate parameters simplifies the constructor call and supports the new unified rendering system.src/Result/ValidationResultJsonRenderer.php (3)
9-16: Well-structured constructor with proper dependency injection.The constructor follows good design patterns with readonly properties and clear parameter defaults for the dry-run and strict mode flags.
18-33: Robust JSON rendering with proper error handling.The implementation includes several good practices:
- Uses
ValidationResult::getOverallResult()to calculate exit codes consistently- Includes comprehensive JSON output with status, message, and issues
- Uses proper JSON encoding flags (
JSON_THROW_ON_ERROR,JSON_PRETTY_PRINT,JSON_UNESCAPED_UNICODE)- Returns the calculated exit code for command execution
51-55: Maintains backward compatibility through legacy array format.The use of
ValidationResult::toLegacyArray()ensures that existing JSON consumers continue to work with the same data structure, which is crucial for maintaining backward compatibility during this architectural refactoring.src/Result/Output.php (2)
21-21: Constructor simplification supports the unified result system.The change from separate
ResultTypeandarray $issuesparameters to a singleValidationResultobject encapsulates all validation data in one cohesive structure, improving the API design.
37-50: Clean renderer selection with match expression.The match expression provides a clear, type-safe way to instantiate the appropriate renderer based on the format. The direct instantiation of
ValidationResultCliRendererandValidationResultJsonRenderereliminates the need for a factory pattern, simplifying the codebase while maintaining the same functionality.src/Result/ValidationResult.php (3)
10-21: Well-designed constructor with proper type safety.The constructor properly types all parameters and uses readonly properties for immutability. The optional
$validatorFileSetPairsparameter with default empty array allows for backward compatibility while supporting the new enhanced functionality.
23-32: Efficient issue detection implementation.The
hasIssues()method uses early return optimization, checking each validator'shasIssues()method and returning immediately when issues are found, which is more efficient than collecting all issues first.
66-112: Comprehensive backward compatibility with dual-mode legacy array conversion.The
toLegacyArray()method excellently handles both scenarios:
- New mode (lines 70-95): Uses validator-fileSet pairs to provide accurate path and set key information, organizing issues by validator class, path, and set key
- Fallback mode (lines 97-108): Maintains backward compatibility by using empty strings for path and set key when validator-fileSet pairs are not available
The nested array structure preserves the existing JSON format while leveraging the new
Issue::toArray()method for consistent issue representation.tests/src/Result/ValidationResultCliRendererTest.php (5)
19-33: Excellent test setup with proper mocking infrastructure.The test setup uses
BufferedOutputfor capturing output and mockedInputInterfacefor controlled testing, providing a solid foundation for testing the CLI renderer behavior.
35-43: Clean test for success scenario.The test properly verifies both the exit code (0) and the expected success message, covering the basic happy path scenario.
45-66: Comprehensive test for issues scenario with proper mock setup.The test creates a realistic scenario with:
- Mock validator with issues
- Proper
Issueobject with test dataFileSetwith test path and filesValidationResultwith validator-fileSet pairsThe assertions verify both exit code and presence of expected output elements.
172-197: Excellent test for multiple validators scenario.This test verifies the renderer's ability to handle multiple file sets for the same validator, ensuring that both paths are included in the output. This is important for validating the grouping logic in the CLI renderer.
199-206: Well-structured mock validator helper method.The helper method provides consistent mock setup with proper method stubs for
renderIssueSets()andexplain(), ensuring all tests use the same mock behavior.src/Result/ValidationRun.php (2)
23-46: Good state isolation with fresh validator instancesCreating fresh validator instances for each file set (line 32) ensures proper state isolation between validation runs. This prevents issues where validators might accumulate state across different file sets.
34-41: Efficient filtering of validators with issuesThe logic correctly filters out validators that don't produce any results, keeping only those with actual issues. This reduces memory usage and simplifies downstream processing.
tests/src/Result/ValidationRunTest.php (1)
14-180: Comprehensive test coverageThe test suite thoroughly covers various scenarios including edge cases (empty inputs), single and multiple validators/file sets, and the static factory method. Good job on the test design!
src/Validator/AbstractValidator.php (2)
31-33: Essential state reset for validator reusabilityThe
resetState()call ensures validators start with a clean state for each validation run. This is crucial for the architecture where validator instances may be reused.
62-73: Improved type safety with Issue objectsThe refactoring to use
Issueobjects internally while maintaining backward compatibility throughtoArray()mapping is well done. This provides better type safety and structure while preserving existing interfaces.src/Validator/MismatchValidator.php (1)
86-91: Good backward compatibility handlingThe dual format support ensures smooth transition from the old array format to the new Issue-based structure. This prevents breaking changes during the refactoring.
src/Result/ValidationResultCliRenderer.php (1)
14-137: Well-designed CLI renderer with good separation of concernsThe renderer effectively organizes output by validator and path, provides appropriate styling, and maintains backward compatibility. The use of
PathUtility::normalizeFolderPathensures consistent path display across different environments.tests/src/Result/ValidationResultTest.php (5)
170-213: LGTM! Well-structured anonymous class implementation.The anonymous class implementation provides a clean way to test different validator behaviors without creating separate test classes. The empty method stubs are appropriate for testing purposes.
215-258: LGTM! Consistent anonymous class pattern.The second anonymous class follows the same pattern as the first, providing controlled test scenarios for validation result aggregation.
125-141: LGTM! Comprehensive test coverage for legacy array conversion.The test properly validates the toLegacyArray method behavior when validator-file set pairs are not provided, ensuring the correct nested array structure is maintained.
143-165: LGTM! Validates file set integration in legacy array.The test confirms that validator-file set pairs are properly integrated into the legacy array structure, maintaining backward compatibility.
291-399: LGTM! Thorough test of mixed validator scenarios.The test properly validates that only validators with issues are included in the legacy array, which is the expected behavior for maintaining backward compatibility.
tests/src/Validator/AbstractValidatorTest.php (5)
29-32: LGTM! Enhanced test scenario for resetState functionality.The additional condition provides a specific test case for validating the resetState functionality, which is crucial for ensuring validators start with clean state.
63-68: LGTM! Updated to use Issue objects.The refactoring to use the new Issue class aligns with the architectural improvements, providing better encapsulation of validation issues.
191-197: LGTM! Updated assertion format.The test assertion correctly validates the expected array structure after the Issue object refactoring.
199-268: LGTM! Comprehensive issue management API tests.The new test methods provide thorough coverage of the issue management functionality, including state validation, collection operations, and proper encapsulation.
270-291: LGTM! Validates state reset functionality.The test properly validates that resetState clears all issues, which is essential for validator reusability.
| $this->addIssue(new Issue( | ||
| '', | ||
| $result, | ||
| '', | ||
| 'MismatchValidator' | ||
| )); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider providing meaningful values for Issue properties
The Issue object is created with empty strings for filename and parserClass. Since this validator works across multiple files, consider using a more descriptive approach, such as:
- Using a combined filename like "multiple_files" or listing the affected files
- Specifying the parser class from the processed files
🤖 Prompt for AI Agents
In src/Validator/MismatchValidator.php around lines 65 to 70, the Issue object
is created with empty strings for filename and parserClass, which reduces
clarity. Update the code to provide meaningful values by setting filename to a
descriptive string like "multiple_files" or a list of affected files, and set
parserClass to the relevant parser class name used in the validation process to
improve issue traceability.
Summary
Major architectural refactoring that transforms the translation validator from legacy array-based processing to a modern object-oriented architecture. This change eliminates nested loops, improves maintainability, and provides unified rendering for both CLI and JSON output formats.
Key Architectural Changes
ValidateTranslationCommand.php:132→ Clean 3-line validation approachValidationResult,ValidationRun,Issue, andFileSetclassesValidationResultCliRendererandValidationResultJsonRendererhasIssues(),getIssues(), andaddIssue()methodsNew Classes
ValidationResult- Core result aggregation with validator instancesValidationRun- Orchestrates validation execution across file setsIssue- Encapsulates individual validation issuesFileSet- Represents grouped translation filesValidationResultCliRenderer- Object-oriented CLI output renderingValidationResultJsonRenderer- Object-oriented JSON output renderingBenefits
Test Coverage
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Tests