fix more cases as the one in #11189 and add check to prevent this issue #11194
Conversation
WalkthroughThe pull request introduces a new boolean field Changes
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
languagetool-core/src/main/java/org/languagetool/rules/patterns/FalseFriendRuleHandler.java (1)
99-101: LGTM! Consider these improvements for maintainability.The validation check is well-placed and effectively prevents same-language translations in false friend rules. A few suggestions for improvement:
- Consider using a more specific exception type (e.g.,
XMLValidationException) instead ofRuntimeException.- Consider externalizing the error message for i18n support.
- Consider extracting the validation to a separate method for better readability.
Here's a suggested refactor:
if (currentTranslationLanguage == language) { - throw new RuntimeException("Translation language (" + currentTranslationLanguage + ") must not be the same as pattern language (" + language + ") for rule " + id); + throw new XMLValidationException(MessageFormat.format(messages.getString("translation_same_as_pattern"), + currentTranslationLanguage, language, id)); }Add to your resource bundle:
translation_same_as_pattern=Translation language ({0}) must not be the same as pattern language ({1}) for rule {2}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
languagetool-core/src/main/java/org/languagetool/rules/patterns/FalseFriendRuleHandler.java(1 hunks)languagetool-core/src/main/resources/org/languagetool/rules/false-friends.xml(9 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (java-kotlin)
🔇 Additional comments (1)
languagetool-core/src/main/resources/org/languagetool/rules/false-friends.xml (1)
3602-3603: LGTM! Good addition of translations.The new translations "boaster" and "braggart" for the Polish word "bufon" improve the accuracy of the false friends detection by better capturing the semantic difference between English "buffoon" (clown/fool) and Polish "bufon" (boaster/braggart).
| @@ -96,6 +96,9 @@ public void startElement(String namespaceURI, String lName, | |||
| if (Languages.isLanguageSupported(languageStr)) { | |||
| Language tmpLang = Languages.getLanguageForShortCode(languageStr); | |||
| currentTranslationLanguage = tmpLang; | |||
| if (currentTranslationLanguage == language) { | |||
| throw new RuntimeException("Translation language (" + currentTranslationLanguage + ") must not be the same as pattern language (" + language + ") for rule " + id); | |||
| } | |||
There was a problem hiding this comment.
Wouldn't it be better to test this in a test instead of throwing a RuntimeException (which prevents the server from starting)?
There was a problem hiding this comment.
Yes, let me try it that way
…is way we avoid a lot of code duplication just for a test
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
languagetool-standalone/src/test/java/org/languagetool/rules/ValidateFalseFriendsSyntaxTest.java (2)
38-38: Replace System.out.println with proper test logging.Using System.out.println for logging in tests is not recommended. Consider using a proper logging framework or JUnit's TestRule to capture output if needed.
- System.out.println("Validating false-friends.xml syntax..."); + // Consider using SLF4J or TestRule for logging + logger.info("Validating false-friends.xml syntax..."); - System.out.println("Validation successfully finished."); + logger.info("Validation successfully finished.");Also applies to: 43-43
34-46: Add more test cases for comprehensive validation.The current test only validates the basic syntax. Consider adding more test cases to:
- Validate error cases (malformed XML, invalid language codes)
- Test with different language combinations
- Verify specific rule patterns
Would you like me to help generate additional test cases to improve coverage?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
languagetool-core/src/main/java/org/languagetool/rules/patterns/FalseFriendRuleHandler.java(2 hunks)languagetool-core/src/main/java/org/languagetool/rules/patterns/FalseFriendRuleLoader.java(2 hunks)languagetool-standalone/src/test/java/org/languagetool/rules/ValidateFalseFriendsSyntaxTest.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- languagetool-core/src/main/java/org/languagetool/rules/patterns/FalseFriendRuleHandler.java
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (java-kotlin)
🔇 Additional comments (3)
languagetool-core/src/main/java/org/languagetool/rules/patterns/FalseFriendRuleLoader.java (3)
42-42: LGTM! Field declaration follows best practices.The new
inTestModefield is correctly declared as final and follows Java naming conventions.
44-58: Well-structured constructor changes.Good use of constructor chaining pattern and proper initialization of the new field while maintaining backward compatibility.
77-77: Verify the FalseFriendRuleHandler implementation.The
inTestModeflag is now passed to the handler. Let's verify that theFalseFriendRuleHandlerproperly implements the validation check.✅ Verification successful
Validation check in FalseFriendRuleHandler is properly implemented
The handler correctly uses the
inTestModeflag to validate that translation language differs from pattern language, which is essential for false friend rules.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for FalseFriendRuleHandler implementation to verify the validation check ast-grep --pattern 'class FalseFriendRuleHandler { $$$ startElement($$$) { $$$ if ($inTestMode$$$ && $$$) { throw new RuntimeException($$$); } $$$ } $$$ }'Length of output: 187
Script:
#!/bin/bash # Find the file containing FalseFriendRuleHandler fd FalseFriendRuleHandler.java # If found, examine its content fd FalseFriendRuleHandler.java -x cat {}Length of output: 9156
| /* LanguageTool, a natural language style checker | ||
| * Copyright (C) 2025 Daniel Naber (http://www.danielnaber.de) |
There was a problem hiding this comment.
Update the copyright year.
The copyright year 2025 is incorrect as it's in the future. Please update it to the current year 2024.
-/* LanguageTool, a natural language style checker
- * Copyright (C) 2025 Daniel Naber (http://www.danielnaber.de)
+/* LanguageTool, a natural language style checker
+ * Copyright (C) 2024 Daniel Naber (http://www.danielnaber.de)📝 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.
| /* LanguageTool, a natural language style checker | |
| * Copyright (C) 2025 Daniel Naber (http://www.danielnaber.de) | |
| /* LanguageTool, a natural language style checker | |
| * Copyright (C) 2024 Daniel Naber (http://www.danielnaber.de) |
| public void testFalseFriendsXML() throws IOException, ParserConfigurationException, SAXException { | ||
| System.out.println("Validating false-friends.xml syntax..."); | ||
| InputStream is = JLanguageTool.getDataBroker().getAsStream(getDataBroker().getRulesDir() + "/false-friends.xml"); | ||
| Language langDE = Languages.getLanguageForShortCode("de"); | ||
| FalseFriendRuleLoader loader = new FalseFriendRuleLoader(langDE, true); | ||
| loader.getRules(is, langDE, Languages.getLanguageForShortCode("en")); | ||
| System.out.println("Validation successfully finished."); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add assertions and improve resource handling.
The test method has several areas for improvement:
- Missing assertions to verify the expected behavior
- Resource leak: InputStream should be closed
- Hard-coded language codes could be constants
Here's a suggested improvement:
+ private static final String LANG_DE = "de";
+ private static final String LANG_EN = "en";
@Test
public void testFalseFriendsXML() throws IOException, ParserConfigurationException, SAXException {
- System.out.println("Validating false-friends.xml syntax...");
- InputStream is = JLanguageTool.getDataBroker().getAsStream(getDataBroker().getRulesDir() + "/false-friends.xml");
- Language langDE = Languages.getLanguageForShortCode("de");
- FalseFriendRuleLoader loader = new FalseFriendRuleLoader(langDE, true);
- loader.getRules(is, langDE, Languages.getLanguageForShortCode("en"));
- System.out.println("Validation successfully finished.");
+ try (InputStream is = JLanguageTool.getDataBroker().getAsStream(getDataBroker().getRulesDir() + "/false-friends.xml")) {
+ Language langDE = Languages.getLanguageForShortCode(LANG_DE);
+ FalseFriendRuleLoader loader = new FalseFriendRuleLoader(langDE, true);
+ List<FalseFriendRule> rules = loader.getRules(is, langDE, Languages.getLanguageForShortCode(LANG_EN));
+ assertNotNull("Rules should not be null", rules);
+ assertFalse("Rules should not be empty", rules.isEmpty());
+ }
}📝 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 void testFalseFriendsXML() throws IOException, ParserConfigurationException, SAXException { | |
| System.out.println("Validating false-friends.xml syntax..."); | |
| InputStream is = JLanguageTool.getDataBroker().getAsStream(getDataBroker().getRulesDir() + "/false-friends.xml"); | |
| Language langDE = Languages.getLanguageForShortCode("de"); | |
| FalseFriendRuleLoader loader = new FalseFriendRuleLoader(langDE, true); | |
| loader.getRules(is, langDE, Languages.getLanguageForShortCode("en")); | |
| System.out.println("Validation successfully finished."); | |
| } | |
| private static final String LANG_DE = "de"; | |
| private static final String LANG_EN = "en"; | |
| @Test | |
| public void testFalseFriendsXML() throws IOException, ParserConfigurationException, SAXException { | |
| try (InputStream is = JLanguageTool.getDataBroker().getAsStream(getDataBroker().getRulesDir() + "/false-friends.xml")) { | |
| Language langDE = Languages.getLanguageForShortCode(LANG_DE); | |
| FalseFriendRuleLoader loader = new FalseFriendRuleLoader(langDE, true); | |
| List<FalseFriendRule> rules = loader.getRules(is, langDE, Languages.getLanguageForShortCode(LANG_EN)); | |
| assertNotNull("Rules should not be null", rules); | |
| assertFalse("Rules should not be empty", rules.isEmpty()); | |
| } | |
| } |
Summary by CodeRabbit
New Features
Bug Fixes
Tests