feat: enhance validation command options to allow skipping and specifying validators#16
feat: enhance validation command options to allow skipping and specifying validators#16konradmichalik merged 4 commits intomainfrom
Conversation
WalkthroughThe changes update the validator selection mechanism in the translation validation command, replacing a single validator option with two new options: "only" and "skip" for specifying which validators to use or exclude. Supporting methods for validation and instantiation were refactored, and related documentation and tests were updated accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Command as ValidateTranslationCommand
participant Utility as ClassUtility
participant Logger
User->>Command: Run validate-translations with --only/--skip
Command->>Command: resolveValidators(input)
Command->>Utility: validateClassInput(interface, type, classNames)
Utility-->>Command: List of valid validator class names
loop For each validator
Command->>Utility: instantiate(interface, logger, type, className)
Utility-->>Command: Validator instance or null
end
Command->>Logger: Log errors if invalid classes
Command->>Command: Run validation with selected validators
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
✨ 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)
src/Command/ValidateTranslationCommand.php (1)
197-216: Consider optimizing the validation approach.The method instantiates classes only for validation purposes but discards the instances. Since
ClassUtility::validateClasscan validate without instantiation, consider using it directly to avoid unnecessary object creation.private function validateClassInput(string $interface, string $type, ?string $className = null): array { if (null === $className) { return []; } $classes = []; if (str_contains($className, ',')) { $classNames = explode(',', $className); foreach ($classNames as $name) { - ClassUtility::instantiate($interface, $this->logger, $type, $name); + $name = trim($name); + if (ClassUtility::validateClass($interface, $this->logger, $name)) { + $classes[] = $name; + } - $classes[] = $name; } } else { - ClassUtility::instantiate($interface, $this->logger, $type, $className); + if (ClassUtility::validateClass($interface, $this->logger, $className)) { + $classes[] = $className; + } - $classes[] = $className; } return $classes; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
README.md(1 hunks)src/Command/ValidateTranslationCommand.php(4 hunks)src/Utility/ClassUtility.php(1 hunks)tests/src/Command/ValidateTranslationCommandTest.php(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/Command/ValidateTranslationCommand.php (1)
src/Utility/ClassUtility.php (1)
instantiate(11-26)
🔇 Additional comments (7)
src/Utility/ClassUtility.php (1)
11-26: LGTM! Well-structured utility method.The implementation properly handles null inputs, validates classes against interfaces, and provides clear error messages. The parameterless constructor assumption is reasonable for the intended use case.
README.md (1)
42-42: Documentation accurately reflects the new validator options.The update correctly shows the new
-s|--skipand-o|--onlyoptions that replace the previous--validatoroption.tests/src/Command/ValidateTranslationCommandTest.php (2)
74-85: Test correctly updated to handle the new validation approach.The test now properly captures the exception thrown when attempting to instantiate an invalid validator class via the
--onlyoption.
245-245: Option correctly updated to use the new--skipflag.src/Command/ValidateTranslationCommand.php (3)
78-88: Well-designed validator selection options.The new
--onlyand--skipoptions provide flexible control over validator selection, improving upon the single validator limitation.
108-113: Clean refactoring using the utility method.Good use of the new
ClassUtility::instantiatemethod for file detector instantiation.
170-192: Logical and clear validator resolution.The precedence handling (
--onlytakes priority over--skip) is well-implemented.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/src/Utility/ClassUtilityTest.php(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: CGL
tests/src/Utility/ClassUtilityTest.php
[error] 69-69: Rector migration failed with exit code 2 during dry-run. Suggested changes involve method signature update for 'error' method callback to 'function (string|\Stringable $message)'.
🔇 Additional comments (2)
tests/src/Utility/ClassUtilityTest.php (2)
60-66: LGTM!The test correctly verifies that
instantiatereturns null when passed a null class name without logging errors.
95-103: LGTM!The test correctly verifies that
instantiatereturns a proper instance when passed a valid class implementing the required interface.
| public function testInstantiateWithClassNotImplementingInterface(): void | ||
| { | ||
| $loggedMessages = []; | ||
| $logger = $this->createMock(LoggerInterface::class); | ||
| $logger->method('error')->willReturnCallback(function ($message) use (&$loggedMessages) { | ||
| $loggedMessages[] = $message; | ||
| }); | ||
|
|
||
| $this->assertNull(ClassUtility::instantiate(DummyInterface::class, $logger, 'test', InvalidClass::class)); | ||
|
|
||
| $this->assertStringContainsString('The test class "MoveElevator\ComposerTranslationValidator\Tests\Utility\InvalidClass" must implement MoveElevator\ComposerTranslationValidator\Tests\Utility\DummyInterface', $loggedMessages[1]); | ||
| } |
There was a problem hiding this comment.
Fix unsafe array access and method signature issues.
This test has the same critical issues as the previous method:
- Pipeline failure: The callback signature needs updating.
- Unsafe array access: Line 92 accesses array index 1 without verification.
Apply this diff to fix the issues:
- $logger->method('error')->willReturnCallback(function ($message) use (&$loggedMessages) {
+ $logger->method('error')->willReturnCallback(function (string|\Stringable $message) use (&$loggedMessages) {
$loggedMessages[] = $message;
});
$this->assertNull(ClassUtility::instantiate(DummyInterface::class, $logger, 'test', InvalidClass::class));
+ $this->assertGreaterThanOrEqual(2, count($loggedMessages), 'Expected at least 2 error messages');
$this->assertStringContainsString('The test class "MoveElevator\ComposerTranslationValidator\Tests\Utility\InvalidClass" must implement MoveElevator\ComposerTranslationValidator\Tests\Utility\DummyInterface', $loggedMessages[1]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public function testInstantiateWithClassNotImplementingInterface(): void | |
| { | |
| $loggedMessages = []; | |
| $logger = $this->createMock(LoggerInterface::class); | |
| $logger->method('error')->willReturnCallback(function ($message) use (&$loggedMessages) { | |
| $loggedMessages[] = $message; | |
| }); | |
| $this->assertNull(ClassUtility::instantiate(DummyInterface::class, $logger, 'test', InvalidClass::class)); | |
| $this->assertStringContainsString('The test class "MoveElevator\ComposerTranslationValidator\Tests\Utility\InvalidClass" must implement MoveElevator\ComposerTranslationValidator\Tests\Utility\DummyInterface', $loggedMessages[1]); | |
| } | |
| public function testInstantiateWithClassNotImplementingInterface(): void | |
| { | |
| $loggedMessages = []; | |
| $logger = $this->createMock(LoggerInterface::class); | |
| $logger->method('error')->willReturnCallback(function (string|\Stringable $message) use (&$loggedMessages) { | |
| $loggedMessages[] = $message; | |
| }); | |
| $this->assertNull(ClassUtility::instantiate(DummyInterface::class, $logger, 'test', InvalidClass::class)); | |
| $this->assertGreaterThanOrEqual(2, count($loggedMessages), 'Expected at least 2 error messages'); | |
| $this->assertStringContainsString( | |
| 'The test class "MoveElevator\ComposerTranslationValidator\Tests\Utility\InvalidClass" must implement MoveElevator\ComposerTranslationValidator\Tests\Utility\DummyInterface', | |
| $loggedMessages[1] | |
| ); | |
| } |
🤖 Prompt for AI Agents
In tests/src/Utility/ClassUtilityTest.php around lines 82 to 93, the test method
has an unsafe array access on $loggedMessages[1] without checking if that index
exists, and the callback signature for the logger mock's error method is
incorrect. To fix this, update the callback to match the expected signature of
the error method, and add a check to ensure the $loggedMessages array has the
required index before accessing it, or adjust the assertion to safely verify the
logged message.
| public function testInstantiateWithNonExistentClass(): void | ||
| { | ||
| $loggedMessages = []; | ||
| $logger = $this->createMock(LoggerInterface::class); | ||
| $logger->method('error')->willReturnCallback(function ($message) use (&$loggedMessages) { | ||
| $loggedMessages[] = $message; | ||
| }); | ||
|
|
||
| ClassUtility::instantiate(DummyInterface::class, $logger, 'test', 'NonExistentClass'); | ||
|
|
||
| $this->assertStringContainsString('The class "NonExistentClass" does not exist.', $loggedMessages[0]); | ||
| $this->assertStringContainsString('The test class "NonExistentClass" must implement MoveElevator\ComposerTranslationValidator\Tests\Utility\DummyInterface', $loggedMessages[1]); | ||
| } |
There was a problem hiding this comment.
Fix unsafe array access and method signature issues.
The test has several critical issues:
- Pipeline failure: The callback signature needs to match
function (string|\Stringable $message)based on the Rector migration error. - Unsafe array access: Lines 78-79 access array indices without verifying the array has those elements.
Apply this diff to fix the issues:
- $logger->method('error')->willReturnCallback(function ($message) use (&$loggedMessages) {
+ $logger->method('error')->willReturnCallback(function (string|\Stringable $message) use (&$loggedMessages) {
$loggedMessages[] = $message;
});
ClassUtility::instantiate(DummyInterface::class, $logger, 'test', 'NonExistentClass');
- $this->assertStringContainsString('The class "NonExistentClass" does not exist.', $loggedMessages[0]);
- $this->assertStringContainsString('The test class "NonExistentClass" must implement MoveElevator\ComposerTranslationValidator\Tests\Utility\DummyInterface', $loggedMessages[1]);
+ $this->assertCount(2, $loggedMessages, 'Expected exactly 2 error messages');
+ $this->assertStringContainsString('The class "NonExistentClass" does not exist.', $loggedMessages[0]);
+ $this->assertStringContainsString('The test class "NonExistentClass" must implement MoveElevator\ComposerTranslationValidator\Tests\Utility\DummyInterface', $loggedMessages[1]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public function testInstantiateWithNonExistentClass(): void | |
| { | |
| $loggedMessages = []; | |
| $logger = $this->createMock(LoggerInterface::class); | |
| $logger->method('error')->willReturnCallback(function ($message) use (&$loggedMessages) { | |
| $loggedMessages[] = $message; | |
| }); | |
| ClassUtility::instantiate(DummyInterface::class, $logger, 'test', 'NonExistentClass'); | |
| $this->assertStringContainsString('The class "NonExistentClass" does not exist.', $loggedMessages[0]); | |
| $this->assertStringContainsString('The test class "NonExistentClass" must implement MoveElevator\ComposerTranslationValidator\Tests\Utility\DummyInterface', $loggedMessages[1]); | |
| } | |
| public function testInstantiateWithNonExistentClass(): void | |
| { | |
| $loggedMessages = []; | |
| $logger = $this->createMock(LoggerInterface::class); | |
| $logger->method('error')->willReturnCallback(function (string|\Stringable $message) use (&$loggedMessages) { | |
| $loggedMessages[] = $message; | |
| }); | |
| ClassUtility::instantiate(DummyInterface::class, $logger, 'test', 'NonExistentClass'); | |
| $this->assertCount(2, $loggedMessages, 'Expected exactly 2 error messages'); | |
| $this->assertStringContainsString( | |
| 'The class "NonExistentClass" does not exist.', | |
| $loggedMessages[0] | |
| ); | |
| $this->assertStringContainsString( | |
| 'The test class "NonExistentClass" must implement MoveElevator\ComposerTranslationValidator\Tests\Utility\DummyInterface', | |
| $loggedMessages[1] | |
| ); | |
| } |
🧰 Tools
🪛 GitHub Actions: CGL
[error] 69-69: Rector migration failed with exit code 2 during dry-run. Suggested changes involve method signature update for 'error' method callback to 'function (string|\Stringable $message)'.
🤖 Prompt for AI Agents
In tests/src/Utility/ClassUtilityTest.php around lines 68 to 80, the error
callback's parameter type should be updated to accept string or Stringable to
match the expected signature, and before accessing $loggedMessages[0] and
$loggedMessages[1], add checks to ensure these indices exist to prevent unsafe
array access errors. Modify the callback function parameter accordingly and add
assertions or conditional checks to verify the array elements exist before
asserting their contents.
Summary by CodeRabbit
New Features
--onlyand--skipoptions.Documentation
Tests