Issue 11165 -- turn off space rule for fr-CA #11175
Conversation
…ultDisabledRulesForVariant() is now also used for Java rules (#11165)
WalkthroughThe pull request introduces changes to the LanguageTool core and language modules, focusing on rule filtering and language-specific configurations. The primary modification is in the Changes
Possibly related PRs
Suggested reviewers
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 (
|
| Solarzubau/S | ||
| Repowering/S #eng | ||
| Inbetriebnahmezahl | ||
| Inbetriebnahmezahlen |
There was a problem hiding this comment.
doesn't belong here, of course...
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
languagetool-language-modules/es/src/main/java/org/languagetool/language/SpanishVoseo.java (1)
19-19: Clean up commented country codesThe line contains commented-out country codes (PA, UY, CR). If these countries aren't currently supported, the comments should be removed. If they will be supported in the future, consider:
- Adding a TODO comment explaining the plan
- Creating a tracking issue for adding support for these countries
- return new String[] { "AR" }; //, "PA" , "UY", "CR" + return new String[] { "AR" };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
languagetool-core/src/main/java/org/languagetool/JLanguageTool.java(1 hunks)languagetool-core/src/test/java/org/languagetool/rules/DemoRuleTest.java(1 hunks)languagetool-language-modules/ca/src/main/java/org/languagetool/language/BalearicCatalan.java(0 hunks)languagetool-language-modules/ca/src/main/java/org/languagetool/language/ValencianCatalan.java(0 hunks)languagetool-language-modules/de/src/main/resources/org/languagetool/resource/de/hunspell/spelling.txt(1 hunks)languagetool-language-modules/es/src/main/java/org/languagetool/language/SpanishVoseo.java(2 hunks)languagetool-language-modules/fr/src/main/java/org/languagetool/language/CanadianFrench.java(2 hunks)
💤 Files with no reviewable changes (2)
- languagetool-language-modules/ca/src/main/java/org/languagetool/language/BalearicCatalan.java
- languagetool-language-modules/ca/src/main/java/org/languagetool/language/ValencianCatalan.java
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (java-kotlin)
🔇 Additional comments (9)
languagetool-language-modules/fr/src/main/java/org/languagetool/language/CanadianFrench.java (1)
21-21: LGTM! The changes correctly implement the space rule disabling for fr-CA.The implementation:
- Properly switches from
Collections.singletonList()toArrays.asList()as we now have multiple rules- Correctly adds the "FRENCH_WHITESPACE" rule to the disabled rules list
Also applies to: 49-51
languagetool-core/src/test/java/org/languagetool/rules/DemoRuleTest.java (2)
39-47: LGTM! Well-structured test cases that verify rule disabling functionality.The test cases effectively verify:
- Basic rule matching (line 42-43)
- Rule disabling behavior (line 44-46)
78-83: LGTM! Good test coverage for disabled rules.The
DemoLang2class properly demonstrates how to disable rules by adding them to the default disabled rules list.languagetool-core/src/main/java/org/languagetool/JLanguageTool.java (1)
478-480: LGTM! Efficient implementation of rule filtering for language variants.The implementation:
- Correctly checks for disabled rules using
getDefaultDisabledRulesForVariant()- Efficiently filters the rules using Stream API
- Maintains the existing behavior for languages without disabled rules
languagetool-language-modules/de/src/main/resources/org/languagetool/resource/de/hunspell/spelling.txt (2)
65837-65841: LGTM! New entries follow German compound word formation rulesThe additions are well-formed:
- Compound words follow correct German word formation rules
- Plural forms are properly indicated with
/Ssuffix where applicable- Terms are relevant to construction and energy sectors
65839-65839: Verify the common usage of the English loanword "Repowering"The term is correctly marked with
#engcomment. However, let's verify its widespread usage in German technical documentation.✅ Verification successful
The "Repowering/S" entry is correctly formatted and valid
While there's limited usage in the codebase, this is an established technical term in the German energy sector, particularly in wind energy contexts. The entry is properly marked as an English loanword (#eng) and correctly includes the plural form suffix (/S).
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for "Repowering" usage in German documentation rg -i "repowering" . --type=txt --glob="**/de/**"Length of output: 173
Script:
#!/bin/bash # Broader search for Repowering and its compounds rg -i "repowering" . --glob="!*.class" --glob="!*.jar" # Search for compound words with Repowering rg -i "[a-z]*repowering[a-z]*|repowering[a-z]*|[a-z]*repowering" . --glob="!*.class" --glob="!*.jar" # Search specifically in documentation files rg -i "repowering" . --glob="*.md" --glob="*.txt" --glob="*.html" --glob="*.xml" --glob="*.properties"Length of output: 632
languagetool-language-modules/es/src/main/java/org/languagetool/language/SpanishVoseo.java (3)
24-26: 🛠️ Refactor suggestionReturn an immutable list to prevent modifications
The method returns a mutable list which could be modified by callers. Since this is a configuration method, it's safer to return an immutable list.
- return rules; + return Collections.unmodifiableList(rules);Likely invalid or redundant comment.
8-10: Verify the purpose of the boolean parameter in super constructorThe constructor calls
super(true), but the purpose of this boolean parameter isn't immediately clear. Consider adding a comment explaining its significance or using a more descriptive constant.Let's check the parent class implementation:
✅ Verification successful
The boolean parameter in super constructor is a framework implementation detail
The
super(true)call is using a protected constructor in the parentSpanishclass that's specifically designed as a "fake" constructor for subclasses. The boolean parameter has no functional purpose - it's just a marker to differentiate between public and protected constructors, preventing direct instantiation except through the language tool framework.Consider adding a comment explaining this implementation detail:
public SpanishVoseo() { // Protected constructor for language variants, boolean parameter is a framework requirement super(true); }🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Find the Spanish.java parent class implementation ast-grep --pattern 'class Spanish { $$$ Spanish(boolean $_) { $$$ } $$$ }'Length of output: 88
Script:
#!/bin/bash # Find Spanish.java file fd Spanish.java # Once found, let's examine its content fd Spanish.java --exec cat {}Length of output: 14741
Line range hint
1-28: Verify if these Spanish language changes are intendedThe PR's objective is to turn off the space rule for fr-CA, but this file contains changes to Spanish language support. Please confirm if these changes are intentional or if they were accidentally included in this PR.
Let's check for related changes in fr-CA files:
| } | ||
|
|
||
| @Override | ||
| public List<String> getDefaultDisabledRulesForVariant() { | ||
| List<String> rules = Collections.singletonList("VOSEO"); | ||
| return Collections.unmodifiableList(rules); | ||
| return rules; |
There was a problem hiding this comment.
Just do
return Collections.singletonList("VOSEO");
| @@ -46,7 +46,7 @@ public String[] getCountries() { | |||
|
|
|||
| @Override | |||
| public List<String> getDefaultDisabledRulesForVariant() { | |||
| List<String> rules = Collections.singletonList("DOUBLER_UNE_CLASSE"); | |||
| List<String> rules = Arrays.asList("DOUBLER_UNE_CLASSE", "FRENCH_WHITESPACE"); | |||
There was a problem hiding this comment.
just do
return Arrays.asList("DOUBLER_UNE_CLASSE", "FRENCH_WHITESPACE");
|
@SteVio89 Thanks for the review. Just after submitting the PR, I noticed we have this case covered already in the front-end, so I'll close this. |
See #11165
Summary by CodeRabbit
Release Notes
New Features
Improvements
Dictionary Updates
Testing