-
-
Notifications
You must be signed in to change notification settings - Fork 318
fix: improve number placeholder regex in PluralTranslationUtil #3331
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
fix: improve number placeholder regex in PluralTranslationUtil #3331
Conversation
…s accidentally translate it
WalkthroughUpdates Tolgee XML tag handling in plural translation utilities by replacing the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
backend/data/src/main/kotlin/io/tolgee/service/machineTranslation/PluralTranslationUtil.kt (1)
69-69: Minor optimization: Non-greedy quantifier is redundant.The non-greedy quantifier
*?in[^\">]*?is redundant since it's immediately followed by the fixed delimiter">. The greedy*would have the same behavior and potentially perform slightly better. However, this is a very minor optimization and the current code is correct and readable.If you'd like to apply the micro-optimization:
- val TOLGEE_TAG_REGEX = "<x tolgee-id=\"[^\">]*?\">.*?</x>".toRegex() + val TOLGEE_TAG_REGEX = "<x tolgee-id=\"[^\">]*\">.*?</x>".toRegex()Note: The
.*?should remain non-greedy to avoid matching multiple tags in one match.backend/data/src/test/kotlin/io/tolgee/unit/util/PluralTranslationUtilTest.kt (1)
32-36: LGTM! Good test coverage for the regex replacement.This test effectively validates that the regex correctly strips x-tags from translated text, and using a different attribute value (
numero-tolleranza) is excellent for verifying the regex flexibility.Consider adding test cases for edge cases if you want more comprehensive coverage:
@Test fun `strips x tags with various content`() { // Empty content "<x tolgee-id=\"test\"></x> text".replace(PluralTranslationUtil.TOLGEE_TAG_REGEX, "#") .assert.isEqualTo("# text") // Whitespace in content "<x tolgee-id=\"test\"> 5 </x> items".replace(PluralTranslationUtil.TOLGEE_TAG_REGEX, "#") .assert.isEqualTo("# items") // Multiple tags "<x tolgee-id=\"a\">1</x> and <x tolgee-id=\"b\">2</x>".replace(PluralTranslationUtil.TOLGEE_TAG_REGEX, "#") .assert.isEqualTo("# and #") }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/data/src/main/kotlin/io/tolgee/service/machineTranslation/PluralTranslationUtil.kt(1 hunks)backend/data/src/test/kotlin/io/tolgee/unit/util/PluralTranslationUtilTest.kt(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
backend/**/*.kt
📄 CodeRabbit inference engine (AGENTS.md)
backend/**/*.kt: After modifying JPA entities, run./gradlew diffChangeLogto generate Liquibase changelog entries (add--no-daemonflag if docker command not found)
Run./gradlew ktlintFormatbefore committing code
Files:
backend/data/src/test/kotlin/io/tolgee/unit/util/PluralTranslationUtilTest.ktbackend/data/src/main/kotlin/io/tolgee/service/machineTranslation/PluralTranslationUtil.kt
backend/**/*Test.kt
📄 CodeRabbit inference engine (AGENTS.md)
backend/**/*Test.kt: Use TestData classes for test setup with TestDataService for saving and cleaning test data in unit tests
Use.andAssertThatJsonfor testing API JSON responses in tests
Files:
backend/data/src/test/kotlin/io/tolgee/unit/util/PluralTranslationUtilTest.kt
**/*.{ts,tsx,js,jsx,kt,kts}
⚙️ CodeRabbit configuration file
As part of review, please check if the file follows 'The Stepdown Rule': The most important, high-level concepts should be at the top (like a newspaper headline and opening paragraph). Details should increase as you read downward. Functions should be ordered so that a caller appears before the functions it calls. When working with JavaScript components, we allow the main component to live at the bottom of the file as an exception to the rule.
Files:
backend/data/src/test/kotlin/io/tolgee/unit/util/PluralTranslationUtilTest.ktbackend/data/src/main/kotlin/io/tolgee/service/machineTranslation/PluralTranslationUtil.kt
🧠 Learnings (1)
📓 Common learnings
Learnt from: stepan662
Repo: tolgee/tolgee-platform PR: 3182
File: webapp/src/component/layout/TopBanner/useAnnouncement.tsx:124-130
Timestamp: 2025-07-28T12:08:29.462Z
Learning: In the Tolgee platform project, JSON translation files are not manually edited. Translations are stored in the Tolgee platform itself and availability is checked at build time. The JSON files are generated artifacts, not the source of truth.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Frontend static check 🪲
- GitHub Check: Build backend 🏗️
- GitHub Check: Build frontend 🏗️
🔇 Additional comments (2)
backend/data/src/test/kotlin/io/tolgee/unit/util/PluralTranslationUtilTest.kt (1)
21-27: LGTM! Test assertions correctly reflect the implementation changes.The updated test expectations consistently use the new
tolgee-idattribute across all plural forms, properly validating the behavior ofgetPreparedSourceStrings.backend/data/src/main/kotlin/io/tolgee/service/machineTranslation/PluralTranslationUtil.kt (1)
67-69: LGTM! The attribute name change fromidtotolgee-idis safe and no old pattern references remain in the codebase.The change from a dynamically constructed regex to an explicit pattern improves code clarity and maintainability. The attribute rename makes it more specific and avoids potential conflicts with standard HTML
idattributes. Verification confirms that no other code references the oldid="tolgee-number"pattern in Kotlin files or test resources.
#3330
Summary by CodeRabbit
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.