fix: improve two-two-way-mismatch validation#2
Conversation
WalkthroughThe updates broaden terminology in documentation, enhance translation validation to detect mismatches in both directions, and refine output formatting for missing translations. The XLIFF parser now supports retrieving either source or target content by key. Test XLIFF files are updated to reflect new key values and structure. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ValidateCommand
participant XliffParser
User->>ValidateCommand: Run translation validation
ValidateCommand->>XliffParser: getContentByKey(key, "source")
ValidateCommand->>XliffParser: getContentByKey(key, "target")
ValidateCommand->>ValidateCommand: Compare keys in source and target
alt Missing or extra keys found
ValidateCommand->>User: Display table with missing/extra keys (yellow dash for empty)
else No mismatches
ValidateCommand->>User: Output "No translation mismatches found."
end
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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
docs/console.jpgis excluded by!**/*.jpg
📒 Files selected for processing (7)
README.md(1 hunks)src/Command/ValidateTranslationCommand.php(4 hunks)src/Parser/XliffParser.php(1 hunks)tests/Build/app/Language/XLIFF-fail/de.locallang.xlf(1 hunks)tests/Build/app/Language/XLIFF-fail/de.locallang_be.xlf(1 hunks)tests/Build/app/Language/XLIFF-fail/locallang.xlf(1 hunks)tests/Build/app/Language/XLIFF-fail/locallang_be.xlf(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/Parser/XliffParser.php (1)
src/Parser/ParserInterface.php (1)
getContentByKey(21-21)
src/Command/ValidateTranslationCommand.php (2)
src/Parser/XliffParser.php (4)
getFileDirectory(66-69)getFileName(61-64)extractKeys(32-40)getContentByKey(42-51)src/Parser/ParserInterface.php (2)
extractKeys(19-19)getContentByKey(21-21)
🔇 Additional comments (14)
src/Parser/XliffParser.php (1)
46-46: LGTM: Dynamic attribute access implementation.The change from hardcoded
$unit->sourceto dynamic$unit->{$attribute}correctly implements the flexibility to retrieve either source or target content. The logic properly handles empty strings by returningnull.tests/Build/app/Language/XLIFF-fail/locallang.xlf (1)
7-7: LGTM: Test data updated for enhanced validation scenarios.The source text updates from simple "English1/English2" to more descriptive "English Key #1/English Key #2" format align well with the enhanced validation logic and provide clearer test scenarios.
Also applies to: 10-10
tests/Build/app/Language/XLIFF-fail/locallang_be.xlf (1)
7-7: LGTM: Consistent test data updates across XLIFF files.The source text updates maintain consistency with other test files in the suite, using the same "English Key #N" format for improved test clarity.
Also applies to: 10-10
tests/Build/app/Language/XLIFF-fail/de.locallang_be.xlf (1)
8-8: LGTM: Target text updates complete the test data consistency.The target text updates to "German Key #N" format complement the source text updates in other test files, creating a comprehensive test suite for the enhanced two-way validation functionality.
Also applies to: 12-12
README.md (3)
3-3: LGTM: Documentation terminology appropriately generalized.The change from "language source and target files" to "language files" correctly reflects the enhanced bidirectional validation scope implemented in the codebase.
7-7: LGTM: Feature description updated consistently.The terminology change aligns with the broader validation capabilities now supported by the enhanced validation logic.
10-10: LGTM: More specific XLIFF format documentation.Adding the explicit file extensions (
*.xlf,*.xliff) provides clearer guidance for users about supported file types.tests/Build/app/Language/XLIFF-fail/de.locallang.xlf (2)
8-8: LGTM: Test data updated with more descriptive content.The target text change to "German Key #1" makes the test data more descriptive and easier to understand.
10-13: LGTM: Test scenario added for bidirectional validation.The new translation unit with
id="key3"and empty source but non-empty target creates an ideal test case for the enhanced bidirectional validation logic that detects keys present in target files but missing from source files.src/Command/ValidateTranslationCommand.php (5)
136-136: LGTM: Type annotations improve code clarity.The inline type annotations for
$sourceand$targetvariables asParserInterfaceinstances enhance code readability and provide better IDE support.Also applies to: 149-149
152-152: LGTM: Excellent implementation of bidirectional validation.This is the core improvement that implements true two-way mismatch detection by combining:
- Keys missing in target from source:
array_diff($sourceKeys, $target->extractKeys())- Keys present in target but missing from source:
array_diff($target->extractKeys(), $sourceKeys)This addresses the PR objective of "two-way-mismatch validation" and ensures comprehensive validation coverage.
154-154: LGTM: More idiomatic PHP condition check.Using
!empty($missingKeys)is more idiomatic and slightly more efficient thancount($missingKeys) > 0for checking non-empty arrays.
163-163: LGTM: Enhanced table formatting with proper content retrieval.Two excellent improvements:
- Correct content retrieval: Using
$target->getContentByKey($key, 'target')correctly retrieves the target content instead of the source content from target files- Better UX: The yellow dash placeholder
<fg=yellow>–</>provides clear visual indication for missing translation valuesThis aligns perfectly with the enhanced
XliffParser::getContentByKey()method that now accepts an optional$attributeparameter.
199-199: LGTM: Success message aligned with broader terminology.The updated message "No translation mismatches found" is consistent with the generalized terminology used in the README and reflects the comprehensive bidirectional validation scope.
| } | ||
|
|
||
| public function getContentByKey(string $key): ?string | ||
| public function getContentByKey(string $key, string $attribute = 'source'): ?string |
There was a problem hiding this comment.
Critical: Interface signature mismatch detected.
The method signature has been changed to include an optional $attribute parameter, but the ParserInterface still declares the method with only one parameter. This violates the interface contract and breaks the Liskov Substitution Principle.
Update the interface to match the new signature:
// In src/Parser/ParserInterface.php
-public function getContentByKey(string $key): ?string;
+public function getContentByKey(string $key, string $attribute = 'source'): ?string;📝 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 getContentByKey(string $key, string $attribute = 'source'): ?string | |
| // File: src/Parser/ParserInterface.php | |
| -public function getContentByKey(string $key): ?string; | |
| +public function getContentByKey(string $key, string $attribute = 'source'): ?string; |
🤖 Prompt for AI Agents
In src/Parser/XliffParser.php at line 42, the method getContentByKey now
includes an optional $attribute parameter, but the ParserInterface still
declares this method with only one parameter. To fix this, update the
ParserInterface method signature for getContentByKey to include the optional
$attribute parameter with the same default value, ensuring the interface
contract matches the implementation and adheres to the Liskov Substitution
Principle.
Summary by CodeRabbit
Bug Fixes
New Features
.xlfand.xliffextensions.Documentation
Tests