feat: modernize KeyNamingConventionValidator with enum-based constants#48
Conversation
|
""" WalkthroughA new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Validator as KeyNamingConventionValidator
participant Enum as KeyNamingConvention
User->>Validator: setConvention("snake_case")
Validator->>Enum: fromString("snake_case")
Enum-->>Validator: KeyNamingConvention::SNAKE_CASE
User->>Validator: validateKeyFormat("my_key")
Validator->>Enum: matches("my_key")
Enum-->>Validator: true/false
Validator-->>User: Validation result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
""" ✨ Finishing Touches
🧪 Generate unit tests
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: 0
🧹 Nitpick comments (1)
src/Enum/KeyNamingConvention.php (1)
65-65: Consider splitting this line for better readability.The line exceeds typical length limits. Consider reformatting for improved readability.
- return self::tryFrom($convention) ?? throw new InvalidArgumentException(sprintf('Unknown convention "%s". Available conventions: %s', $convention, implode(', ', self::getAvailableConventions()))); + return self::tryFrom($convention) + ?? throw new InvalidArgumentException( + sprintf( + 'Unknown convention "%s". Available conventions: %s', + $convention, + implode(', ', self::getAvailableConventions()) + ) + );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Enum/KeyNamingConvention.php(1 hunks)src/Validator/KeyNamingConventionValidator.php(8 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/Enum/KeyNamingConvention.php (1)
src/Validator/KeyNamingConventionValidator.php (1)
getAvailableConventions(345-356)
🔇 Additional comments (10)
src/Enum/KeyNamingConvention.php (4)
28-34: Well-structured enum design!The string-backed enum with clearly named cases provides excellent type safety and matches each convention with its descriptive value.
73-76: Clean implementation using modern PHP features!Excellent use of array_map with arrow functions for a concise implementation.
81-84: Correct pattern matching implementation!Good use of strict comparison with preg_match return value.
39-39: Fix SNAKE_CASE pattern to allow single-character keys.The current pattern
/^[a-z]([a-z0-9]|_[a-z0-9])*$/won't match single-character keys like"a"or"x". This is inconsistent with the KEBAB_CASE pattern which explicitly handles this case.Apply this fix to allow single-character snake_case keys:
- self::SNAKE_CASE => '/^[a-z]([a-z0-9]|_[a-z0-9])*$/', + self::SNAKE_CASE => '/^[a-z]([a-z0-9]|_[a-z0-9])*$|^[a-z]$/',Alternatively, simplify the pattern:
- self::SNAKE_CASE => '/^[a-z]([a-z0-9]|_[a-z0-9])*$/', + self::SNAKE_CASE => '/^[a-z][a-z0-9_]*$/',Likely an incorrect or invalid review comment.
src/Validator/KeyNamingConventionValidator.php (6)
28-28: Excellent type safety improvement!Using the enum type for the convention property ensures compile-time type safety and prevents invalid convention values.
Also applies to: 38-38
119-122: Clean delegation to enum validation!The validation logic is now properly encapsulated in the enum, simplifying this method.
162-169: Good use of enum's matches() method!Clean implementation that properly delegates validation to the enum.
259-268: Well-implemented conversion method!The toDotNotation method correctly handles all source formats and follows the same pattern as other conversion methods.
345-356: Excellent refactoring to use enum as single source of truth!The dynamic generation from enum cases eliminates code duplication and ensures consistency.
477-481: Clean integration with enum iteration!Good use of enum cases() method and the matches() validation.
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (1)
tests/src/Validator/EncodingValidatorTest.php (1)
298-391: Consider consolidating duplicate test functionality.Some of the new test methods duplicate functionality already covered by existing tests:
testEmptyFileContentReturnsNoIssuesoverlaps withtestEmptyFiletestFileReadErrorHandlingduplicatestestFileReadErrorConsider removing duplicates or clearly differentiating the test scenarios to avoid redundancy.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/src/Validator/EncodingValidatorTest.php(1 hunks)tests/src/Validator/KeyNamingConventionValidatorTest.php(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: CGL
tests/src/Validator/EncodingValidatorTest.php
[error] 303-303: EditorConfig: Trailing whitespaces found.
[error] 319-319: EditorConfig: Trailing whitespaces found.
[error] 346-346: EditorConfig: Trailing whitespaces found.
[error] 353-353: EditorConfig: Trailing whitespaces found.
[error] 372-372: EditorConfig: Trailing whitespaces found.
[error] 377-377: EditorConfig: Trailing whitespaces found.
tests/src/Validator/KeyNamingConventionValidatorTest.php
[error] 606-606: EditorConfig: Trailing whitespaces found.
[error] 618-618: EditorConfig: Trailing whitespaces found.
[error] 624-624: EditorConfig: Trailing whitespaces found.
🔇 Additional comments (2)
tests/src/Validator/KeyNamingConventionValidatorTest.php (2)
578-593: LGTM! Good validation of enum-based convention handling.The test correctly validates that the
expected_conventionis now an enum instance instead of a string, which aligns with the modernization objectives.
629-682: LGTM! Excellent coverage of edge cases.The new test methods provide comprehensive coverage of:
- Mixed conventions with dot-separated segments
- Unknown pattern detection
- Enum-based issue message formatting
These tests ensure the enum-based refactoring works correctly across various scenarios.
| public function testUnicodeNormalizationWithoutIntlExtension(): void | ||
| { | ||
| // Test the fallback when Normalizer class doesn't exist | ||
| $filePath = $this->testFilesPath.'/unicode-test.yaml'; | ||
| file_put_contents($filePath, "key: café"); // Contains normalized Unicode | ||
|
|
||
| $parser = new YamlParser($filePath); | ||
|
|
||
| // Test by calling the method through reflection since it's private | ||
| $reflector = new \ReflectionClass(EncodingValidator::class); | ||
| $method = $reflector->getMethod('hasUnicodeNormalizationIssues'); | ||
| $method->setAccessible(true); | ||
|
|
||
| $validator = new EncodingValidator(); | ||
|
|
||
| if (!class_exists('Normalizer')) { | ||
| $result = $method->invokeArgs($validator, [file_get_contents($filePath)]); | ||
| $this->assertFalse($result); | ||
| } else { | ||
| // If Normalizer exists, test that it works normally | ||
| $issues = $validator->processFile($parser); | ||
| // File with normal Unicode should not have normalization issues | ||
| $this->assertArrayNotHasKey('unicode_normalization', $issues); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix trailing whitespace issues and improve test reliability.
There are trailing whitespaces on lines 346 and 353 that need to be removed. Additionally, using reflection to test private methods is generally discouraged as it breaks encapsulation.
Apply this diff to fix the trailing whitespace:
- $parser = new YamlParser($filePath);
+ $parser = new YamlParser($filePath);- $validator = new EncodingValidator();
+ $validator = new EncodingValidator();Consider testing the public interface instead of using reflection to access private methods. This makes tests more maintainable and less brittle.
📝 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 testUnicodeNormalizationWithoutIntlExtension(): void | |
| { | |
| // Test the fallback when Normalizer class doesn't exist | |
| $filePath = $this->testFilesPath.'/unicode-test.yaml'; | |
| file_put_contents($filePath, "key: café"); // Contains normalized Unicode | |
| $parser = new YamlParser($filePath); | |
| // Test by calling the method through reflection since it's private | |
| $reflector = new \ReflectionClass(EncodingValidator::class); | |
| $method = $reflector->getMethod('hasUnicodeNormalizationIssues'); | |
| $method->setAccessible(true); | |
| $validator = new EncodingValidator(); | |
| if (!class_exists('Normalizer')) { | |
| $result = $method->invokeArgs($validator, [file_get_contents($filePath)]); | |
| $this->assertFalse($result); | |
| } else { | |
| // If Normalizer exists, test that it works normally | |
| $issues = $validator->processFile($parser); | |
| // File with normal Unicode should not have normalization issues | |
| $this->assertArrayNotHasKey('unicode_normalization', $issues); | |
| } | |
| } | |
| public function testUnicodeNormalizationWithoutIntlExtension(): void | |
| { | |
| // Test the fallback when Normalizer class doesn't exist | |
| $filePath = $this->testFilesPath.'/unicode-test.yaml'; | |
| file_put_contents($filePath, "key: café"); // Contains normalized Unicode | |
| $parser = new YamlParser($filePath); | |
| // Test by calling the method through reflection since it's private | |
| $reflector = new \ReflectionClass(EncodingValidator::class); | |
| $method = $reflector->getMethod('hasUnicodeNormalizationIssues'); | |
| $method->setAccessible(true); | |
| $validator = new EncodingValidator(); | |
| if (!class_exists('Normalizer')) { | |
| $result = $method->invokeArgs($validator, [file_get_contents($filePath)]); | |
| $this->assertFalse($result); | |
| } else { | |
| // If Normalizer exists, test that it works normally | |
| $issues = $validator->processFile($parser); | |
| // File with normal Unicode should not have normalization issues | |
| $this->assertArrayNotHasKey('unicode_normalization', $issues); | |
| } | |
| } |
🧰 Tools
🪛 GitHub Actions: CGL
[error] 346-346: EditorConfig: Trailing whitespaces found.
[error] 353-353: EditorConfig: Trailing whitespaces found.
🤖 Prompt for AI Agents
In tests/src/Validator/EncodingValidatorTest.php around lines 339 to 363, remove
the trailing whitespaces on lines 346 and 353 to clean up the code. Replace the
reflection-based call to the private method 'hasUnicodeNormalizationIssues' by
testing the public interface of EncodingValidator instead, such as using the
'processFile' method, to improve test reliability and maintain encapsulation.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/src/Validator/EncodingValidatorTest.php (1)
340-364: Consider testing public interface instead of using reflection.While the test logic is sound, using reflection to test private methods breaks encapsulation and makes tests more brittle. The test correctly handles both scenarios (with and without Normalizer class), but could be improved by testing through the public interface.
Consider refactoring to test the public
processFilemethod instead:- // Test by calling the method through reflection since it's private - $reflector = new ReflectionClass(EncodingValidator::class); - $method = $reflector->getMethod('hasUnicodeNormalizationIssues'); - $method->setAccessible(true); - - $validator = new EncodingValidator(); - - if (!class_exists('Normalizer')) { - $result = $method->invokeArgs($validator, [file_get_contents($filePath)]); - $this->assertFalse($result); - } else { - // If Normalizer exists, test that it works normally - $issues = $validator->processFile($parser); - // File with normal Unicode should not have normalization issues - $this->assertArrayNotHasKey('unicode_normalization', $issues); - } + $validator = new EncodingValidator(); + $issues = $validator->processFile($parser); + + if (!class_exists('Normalizer')) { + // When Normalizer is not available, no normalization issues should be detected + $this->assertArrayNotHasKey('unicode_normalization', $issues); + } else { + // File with normal Unicode should not have normalization issues + $this->assertArrayNotHasKey('unicode_normalization', $issues); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/src/Validator/EncodingValidatorTest.php(2 hunks)tests/src/Validator/KeyNamingConventionValidatorTest.php(1 hunks)
🔇 Additional comments (11)
tests/src/Validator/EncodingValidatorTest.php (4)
35-35: LGTM: ReflectionClass import added for testing.The addition of the ReflectionClass import is appropriate for the new test methods that need to access private methods.
299-313: LGTM: Empty file test is well-implemented.The test correctly validates that empty file content returns no validation issues, which is appropriate behavior for the encoding validator.
315-338: LGTM: File read error handling test is comprehensive.The test properly simulates file read errors and verifies that the logger is called with the expected error message. The use of a mock parser and file deletion to simulate the error condition is appropriate.
366-392: LGTM: Unicode normalization test is thorough.The test correctly creates content with decomposed Unicode characters and validates that normalization issues are detected when the content is not in NFC form. The conditional assertion based on actual normalization differences is appropriate.
tests/src/Validator/KeyNamingConventionValidatorTest.php (7)
566-576: LGTM: Test updated to include dot.notation convention.The test correctly validates that the new
dot.notationconvention is available and has an appropriate description. The enum-based approach is properly reflected in the test assertions.
578-593: LGTM: Dot notation validation test is well-implemented.The test correctly validates keys against the dot.notation convention and properly asserts that the
expected_conventionis now an enum instance rather than a string, reflecting the modernization to enum-based constants.
595-612: LGTM: Dot notation conversion test covers multiple input formats.The test comprehensively validates conversion suggestions from camelCase, snake_case, and kebab-case to dot.notation format, ensuring the enum-based suggestion logic works correctly.
614-627: LGTM: Null convention test validates expected behavior.The test correctly validates that when no convention is set, the validator returns no issues for a single key, which is the expected behavior for consistency checking.
629-642: LGTM: Mixed convention detection test is appropriate.The test validates that mixed conventions in dot-separated keys are properly detected, ensuring the enum-based detection logic works correctly with complex key patterns.
644-657: LGTM: Unknown pattern detection test is comprehensive.The test validates detection of mixed conventions when invalid characters are present alongside valid keys, ensuring robust pattern matching with the enum-based approach.
659-682: LGTM: Enum-based issue formatting test is solid.The test validates that issue message formatting works correctly with the enum-based convention system, ensuring backward compatibility in the output format.
Summary by CodeRabbit
New Features
Refactor
Tests