Fixed Issue #11710 - Capitalization of false positive in URLs - #11740
Fixed Issue #11710 - Capitalization of false positive in URLs#11740dhruvgupta77 wants to merge 6 commits into
Conversation
WalkthroughAdded an antipattern to prevent the lowercase-after-period capitalization rule from firing on URLs and tightened the URL token regex; added a new JUnit test class with six tests to verify URLs are not flagged. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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.
Actionable comments posted: 2
🧹 Nitpick comments (2)
languagetool-language-modules/en/src/test/java/org/languagetool/rules/en/GithubUrlCapitalizationTest.java (2)
12-12: Consider a more general class name.The class name
GithubUrlCapitalizationTestis specific to GitHub, but the underlying grammar fix applies to URLs in general (http, https, www patterns). Consider renaming toUrlCapitalizationTestor similar to reflect the broader scope of the fix.
14-23: Test logic is correct; consider expanding coverage.The test correctly verifies that the GitHub URL doesn't trigger capitalization rules. However, the grammar changes reportedly handle URLs more broadly (http, https, www patterns).
Consider adding test cases for:
- Other URL schemes:
http://example.com/Path,www.example.com/Path- URLs at different positions in text
- Negative case: Verify capitalization rules still work for non-URL text (e.g., "visit example.com" should still flag issues if appropriate)
🔎 Example additional test cases
@Test public void testHttpUrlNotFlagged() throws Exception { JLanguageTool langTool = new JLanguageTool(AmericanEnglish.getInstance()); String text = "Visit http://example.com/SomePath for more info."; List<RuleMatch> matches = langTool.check(text); assertTrue(matches.stream().noneMatch( m -> m.getRule().getId().contains("UPPERCASE") || m.getRule().getId().contains("CAPITALIZATION"))); } @Test public void testWwwUrlNotFlagged() throws Exception { JLanguageTool langTool = new JLanguageTool(AmericanEnglish.getInstance()); String text = "Check www.example.com/CamelCase today."; List<RuleMatch> matches = langTool.check(text); assertTrue(matches.stream().noneMatch( m -> m.getRule().getId().contains("UPPERCASE") || m.getRule().getId().contains("CAPITALIZATION"))); }
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
languagetool-language-modules/en/src/main/resources/org/languagetool/rules/en/grammar.xmllanguagetool-language-modules/en/src/test/java/org/languagetool/rules/en/GithubUrlCapitalizationTest.java
🧰 Additional context used
🧠 Learnings (7)
📚 Learning: 2025-12-04T09:58:23.800Z
Learnt from: AnnaRusalkina
Repo: languagetool-org/languagetool PR: 11688
File: languagetool-language-modules/en/src/main/resources/org/languagetool/rules/en/grammar.xml:134216-134216
Timestamp: 2025-12-04T09:58:23.800Z
Learning: In the file `languagetool-language-modules/en/src/main/resources/org/languagetool/rules/en/grammar.xml`, the comment "Grammatical mistakes are widespread in everyday writting." contains an intentional typo ("writting" instead of "writing") that serves as an example of the type of mistake the antipattern is designed to handle.
Applied to files:
languagetool-language-modules/en/src/main/resources/org/languagetool/rules/en/grammar.xml
📚 Learning: 2025-10-10T13:40:54.056Z
Learnt from: AnnaRusalkina
Repo: languagetool-org/languagetool PR: 11564
File: languagetool-language-modules/en/src/main/resources/org/languagetool/rules/en/grammar.xml:139951-139956
Timestamp: 2025-10-10T13:40:54.056Z
Learning: In LanguageTool XML rules, hyphenated words (e.g., "COVID-19") are tokenized as single tokens, so a single `<token>` pattern with a regex can match them in one go.
Applied to files:
languagetool-language-modules/en/src/main/resources/org/languagetool/rules/en/grammar.xml
📚 Learning: 2025-10-10T12:48:33.301Z
Learnt from: AnnaRusalkina
Repo: languagetool-org/languagetool PR: 11564
File: languagetool-language-modules/en/src/main/resources/org/languagetool/rules/en/grammar.xml:139958-139966
Timestamp: 2025-10-10T12:48:33.301Z
Learning: In LanguageTool XML grammar rules (grammar.xml files), tokens without the `case_sensitive="yes"` attribute are case-insensitive by default. For example, `<token>pre</token>` will match "pre", "Pre", "PRE", etc. without needing regex patterns like "[Pp]re".
Applied to files:
languagetool-language-modules/en/src/main/resources/org/languagetool/rules/en/grammar.xml
📚 Learning: 2025-12-29T14:12:34.878Z
Learnt from: inesakochur
Repo: languagetool-org/languagetool PR: 11739
File: languagetool-language-modules/pt/src/main/resources/org/languagetool/rules/pt/pt-PT/grammar.xml:2850-2874
Timestamp: 2025-12-29T14:12:34.878Z
Learning: In LanguageTool XML rules, suggestions do not need to be manually capitalized when a rule fires at SENT_START. LanguageTool automatically capitalizes suggestions that are applied at the beginning of a sentence, so lowercase suggestions like "tu" and "eu" will be correctly capitalized to "Tu" and "Eu" when replacing text at sentence start.
Applied to files:
languagetool-language-modules/en/src/main/resources/org/languagetool/rules/en/grammar.xml
📚 Learning: 2025-10-15T12:25:46.254Z
Learnt from: AnnaRusalkina
Repo: languagetool-org/languagetool PR: 11574
File: languagetool-language-modules/en/src/main/resources/org/languagetool/rules/en/grammar.xml:109839-109843
Timestamp: 2025-10-15T12:25:46.254Z
Learning: In LanguageTool XML rule files, the `skip` attribute with a negative value (e.g., `skip="-1"`) means "skip forward any number of tokens until the next pattern token is found", allowing patterns to match across arbitrarily long spans. This is documented at https://dev.languagetool.org/development-overview
Applied to files:
languagetool-language-modules/en/src/main/resources/org/languagetool/rules/en/grammar.xml
📚 Learning: 2025-07-26T07:31:09.449Z
Learnt from: marcoagpinto
Repo: languagetool-org/languagetool PR: 11458
File: languagetool-language-modules/pt/src/main/resources/org/languagetool/rules/pt/style.xml:3689-3693
Timestamp: 2025-07-26T07:31:09.449Z
Learning: In LanguageTool antipatterns, the `scope='next'` attribute is bugged and removes valid entries, so workarounds using empty `<token>` with `<exception>` tags are sometimes necessary to achieve the desired matching logic, even though this approach appears counterintuitive.
Applied to files:
languagetool-language-modules/en/src/main/resources/org/languagetool/rules/en/grammar.xml
📚 Learning: 2025-10-08T06:41:55.119Z
Learnt from: marcoagpinto
Repo: languagetool-org/languagetool PR: 11557
File: languagetool-language-modules/pt/src/main/resources/org/languagetool/resource/pt/disambiguation.xml:4068-4078
Timestamp: 2025-10-08T06:41:55.119Z
Learning: In Portuguese disambiguation rules, when a pattern targets tokens with multiple verb readings (e.g., VMIP3S0 and VMM02S0), including an exception like `<exception postag_regexp='yes' postag='AQ.+'//>` on subsequent participle tokens is necessary to prevent false positives, even though it reduces the number of matches. The rule can still fire successfully for cases where the participle doesn't have an adjective reading, as confirmed by testing showing ~4683 matches across ~950k sentences.
Applied to files:
languagetool-language-modules/en/src/main/resources/org/languagetool/rules/en/grammar.xml
🧬 Code graph analysis (1)
languagetool-language-modules/en/src/test/java/org/languagetool/rules/en/GithubUrlCapitalizationTest.java (1)
languagetool-core/src/main/java/org/languagetool/JLanguageTool.java (1)
JLanguageTool(73-2295)
🔇 Additional comments (1)
languagetool-language-modules/en/src/test/java/org/languagetool/rules/en/GithubUrlCapitalizationTest.java (1)
1-11: LGTM!Package declaration and imports are appropriate for this test class.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
languagetool-language-modules/en/src/main/resources/org/languagetool/rules/en/grammar.xml (1)
2127-2130: Critical XML syntax error at line 100995 prevents rule from loading.Line 100995 contains a malformed
+character prefix before the token element:+ <token regexp="yes">(https?://|www\.)[^\s]+</token>. This breaks XML parsing for the entire "need" verb rule.Additionally, the test coverage in GithubUrlCapitalizationTest.java is insufficient—it only validates a simple GitHub URL with capital letters, not complex URLs with query parameters, fragments, or other special characters that could expose tokenization issues.
The antipattern implementation at lines 2127-2130 is syntactically correct, but both locations need to be fixed: remove the
+prefix on line 100995 and expand test coverage to include URLs with special characters (e.g.,https://example.com/path?query=value#anchor).
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
languagetool-language-modules/en/src/main/resources/org/languagetool/rules/en/grammar.xml
🧰 Additional context used
🧠 Learnings (7)
📚 Learning: 2025-12-04T09:58:23.800Z
Learnt from: AnnaRusalkina
Repo: languagetool-org/languagetool PR: 11688
File: languagetool-language-modules/en/src/main/resources/org/languagetool/rules/en/grammar.xml:134216-134216
Timestamp: 2025-12-04T09:58:23.800Z
Learning: In the file `languagetool-language-modules/en/src/main/resources/org/languagetool/rules/en/grammar.xml`, the comment "Grammatical mistakes are widespread in everyday writting." contains an intentional typo ("writting" instead of "writing") that serves as an example of the type of mistake the antipattern is designed to handle.
Applied to files:
languagetool-language-modules/en/src/main/resources/org/languagetool/rules/en/grammar.xml
📚 Learning: 2025-10-10T13:40:54.056Z
Learnt from: AnnaRusalkina
Repo: languagetool-org/languagetool PR: 11564
File: languagetool-language-modules/en/src/main/resources/org/languagetool/rules/en/grammar.xml:139951-139956
Timestamp: 2025-10-10T13:40:54.056Z
Learning: In LanguageTool XML rules, hyphenated words (e.g., "COVID-19") are tokenized as single tokens, so a single `<token>` pattern with a regex can match them in one go.
Applied to files:
languagetool-language-modules/en/src/main/resources/org/languagetool/rules/en/grammar.xml
📚 Learning: 2025-10-10T12:48:33.301Z
Learnt from: AnnaRusalkina
Repo: languagetool-org/languagetool PR: 11564
File: languagetool-language-modules/en/src/main/resources/org/languagetool/rules/en/grammar.xml:139958-139966
Timestamp: 2025-10-10T12:48:33.301Z
Learning: In LanguageTool XML grammar rules (grammar.xml files), tokens without the `case_sensitive="yes"` attribute are case-insensitive by default. For example, `<token>pre</token>` will match "pre", "Pre", "PRE", etc. without needing regex patterns like "[Pp]re".
Applied to files:
languagetool-language-modules/en/src/main/resources/org/languagetool/rules/en/grammar.xml
📚 Learning: 2025-10-15T12:25:46.254Z
Learnt from: AnnaRusalkina
Repo: languagetool-org/languagetool PR: 11574
File: languagetool-language-modules/en/src/main/resources/org/languagetool/rules/en/grammar.xml:109839-109843
Timestamp: 2025-10-15T12:25:46.254Z
Learning: In LanguageTool XML rule files, the `skip` attribute with a negative value (e.g., `skip="-1"`) means "skip forward any number of tokens until the next pattern token is found", allowing patterns to match across arbitrarily long spans. This is documented at https://dev.languagetool.org/development-overview
Applied to files:
languagetool-language-modules/en/src/main/resources/org/languagetool/rules/en/grammar.xml
📚 Learning: 2025-12-29T14:12:34.878Z
Learnt from: inesakochur
Repo: languagetool-org/languagetool PR: 11739
File: languagetool-language-modules/pt/src/main/resources/org/languagetool/rules/pt/pt-PT/grammar.xml:2850-2874
Timestamp: 2025-12-29T14:12:34.878Z
Learning: In LanguageTool XML rules, suggestions do not need to be manually capitalized when a rule fires at SENT_START. LanguageTool automatically capitalizes suggestions that are applied at the beginning of a sentence, so lowercase suggestions like "tu" and "eu" will be correctly capitalized to "Tu" and "Eu" when replacing text at sentence start.
Applied to files:
languagetool-language-modules/en/src/main/resources/org/languagetool/rules/en/grammar.xml
📚 Learning: 2025-10-08T06:41:55.119Z
Learnt from: marcoagpinto
Repo: languagetool-org/languagetool PR: 11557
File: languagetool-language-modules/pt/src/main/resources/org/languagetool/resource/pt/disambiguation.xml:4068-4078
Timestamp: 2025-10-08T06:41:55.119Z
Learning: In Portuguese disambiguation rules, when a pattern targets tokens with multiple verb readings (e.g., VMIP3S0 and VMM02S0), including an exception like `<exception postag_regexp='yes' postag='AQ.+'//>` on subsequent participle tokens is necessary to prevent false positives, even though it reduces the number of matches. The rule can still fire successfully for cases where the participle doesn't have an adjective reading, as confirmed by testing showing ~4683 matches across ~950k sentences.
Applied to files:
languagetool-language-modules/en/src/main/resources/org/languagetool/rules/en/grammar.xml
📚 Learning: 2025-07-26T07:31:09.449Z
Learnt from: marcoagpinto
Repo: languagetool-org/languagetool PR: 11458
File: languagetool-language-modules/pt/src/main/resources/org/languagetool/rules/pt/style.xml:3689-3693
Timestamp: 2025-07-26T07:31:09.449Z
Learning: In LanguageTool antipatterns, the `scope='next'` attribute is bugged and removes valid entries, so workarounds using empty `<token>` with `<exception>` tags are sometimes necessary to achieve the desired matching logic, even though this approach appears counterintuitive.
Applied to files:
languagetool-language-modules/en/src/main/resources/org/languagetool/rules/en/grammar.xml
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
languagetool-language-modules/en/src/main/resources/org/languagetool/rules/en/grammar.xml (1)
2127-2130: Antipattern implementation looks good; consider edge case with trailing punctuation.The antipattern correctly prevents capitalization rules from triggering on URLs. The regex properly matches
https://,http://, andwww.prefixed URLs.One minor consideration: The pattern
[^\s]+greedily matches all non-whitespace characters, which will include trailing punctuation. For example,https://github.com.(URL at end of sentence) will match including the period. This is likely acceptable for an antipattern that's preventing false positives, but if you encounter edge cases where this causes issues, you might consider a more restrictive pattern like[^\s.,;!?]+to exclude common sentence-ending punctuation.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
languagetool-language-modules/en/src/main/resources/org/languagetool/rules/en/grammar.xml
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: AnnaRusalkina
Repo: languagetool-org/languagetool PR: 11688
File: languagetool-language-modules/en/src/main/resources/org/languagetool/rules/en/grammar.xml:134216-134216
Timestamp: 2025-12-04T09:58:23.800Z
Learning: In the file `languagetool-language-modules/en/src/main/resources/org/languagetool/rules/en/grammar.xml`, the comment "Grammatical mistakes are widespread in everyday writting." contains an intentional typo ("writting" instead of "writing") that serves as an example of the type of mistake the antipattern is designed to handle.
Learnt from: AnnaRusalkina
Repo: languagetool-org/languagetool PR: 11564
File: languagetool-language-modules/en/src/main/resources/org/languagetool/rules/en/grammar.xml:139958-139966
Timestamp: 2025-10-10T12:48:33.301Z
Learning: In LanguageTool XML grammar rules (grammar.xml files), tokens without the `case_sensitive="yes"` attribute are case-insensitive by default. For example, `<token>pre</token>` will match "pre", "Pre", "PRE", etc. without needing regex patterns like "[Pp]re".
Learnt from: AnnaRusalkina
Repo: languagetool-org/languagetool PR: 11564
File: languagetool-language-modules/en/src/main/resources/org/languagetool/rules/en/grammar.xml:139951-139956
Timestamp: 2025-10-10T13:40:54.056Z
Learning: In LanguageTool XML rules, hyphenated words (e.g., "COVID-19") are tokenized as single tokens, so a single `<token>` pattern with a regex can match them in one go.
📚 Learning: 2025-12-04T09:58:23.800Z
Learnt from: AnnaRusalkina
Repo: languagetool-org/languagetool PR: 11688
File: languagetool-language-modules/en/src/main/resources/org/languagetool/rules/en/grammar.xml:134216-134216
Timestamp: 2025-12-04T09:58:23.800Z
Learning: In the file `languagetool-language-modules/en/src/main/resources/org/languagetool/rules/en/grammar.xml`, the comment "Grammatical mistakes are widespread in everyday writting." contains an intentional typo ("writting" instead of "writing") that serves as an example of the type of mistake the antipattern is designed to handle.
Applied to files:
languagetool-language-modules/en/src/main/resources/org/languagetool/rules/en/grammar.xml
📚 Learning: 2025-10-10T12:48:33.301Z
Learnt from: AnnaRusalkina
Repo: languagetool-org/languagetool PR: 11564
File: languagetool-language-modules/en/src/main/resources/org/languagetool/rules/en/grammar.xml:139958-139966
Timestamp: 2025-10-10T12:48:33.301Z
Learning: In LanguageTool XML grammar rules (grammar.xml files), tokens without the `case_sensitive="yes"` attribute are case-insensitive by default. For example, `<token>pre</token>` will match "pre", "Pre", "PRE", etc. without needing regex patterns like "[Pp]re".
Applied to files:
languagetool-language-modules/en/src/main/resources/org/languagetool/rules/en/grammar.xml
📚 Learning: 2025-10-10T13:40:54.056Z
Learnt from: AnnaRusalkina
Repo: languagetool-org/languagetool PR: 11564
File: languagetool-language-modules/en/src/main/resources/org/languagetool/rules/en/grammar.xml:139951-139956
Timestamp: 2025-10-10T13:40:54.056Z
Learning: In LanguageTool XML rules, hyphenated words (e.g., "COVID-19") are tokenized as single tokens, so a single `<token>` pattern with a regex can match them in one go.
Applied to files:
languagetool-language-modules/en/src/main/resources/org/languagetool/rules/en/grammar.xml
📚 Learning: 2025-10-08T06:41:55.119Z
Learnt from: marcoagpinto
Repo: languagetool-org/languagetool PR: 11557
File: languagetool-language-modules/pt/src/main/resources/org/languagetool/resource/pt/disambiguation.xml:4068-4078
Timestamp: 2025-10-08T06:41:55.119Z
Learning: In Portuguese disambiguation rules, when a pattern targets tokens with multiple verb readings (e.g., VMIP3S0 and VMM02S0), including an exception like `<exception postag_regexp='yes' postag='AQ.+'//>` on subsequent participle tokens is necessary to prevent false positives, even though it reduces the number of matches. The rule can still fire successfully for cases where the participle doesn't have an adjective reading, as confirmed by testing showing ~4683 matches across ~950k sentences.
Applied to files:
languagetool-language-modules/en/src/main/resources/org/languagetool/rules/en/grammar.xml
📚 Learning: 2025-07-26T07:31:09.449Z
Learnt from: marcoagpinto
Repo: languagetool-org/languagetool PR: 11458
File: languagetool-language-modules/pt/src/main/resources/org/languagetool/rules/pt/style.xml:3689-3693
Timestamp: 2025-07-26T07:31:09.449Z
Learning: In LanguageTool antipatterns, the `scope='next'` attribute is bugged and removes valid entries, so workarounds using empty `<token>` with `<exception>` tags are sometimes necessary to achieve the desired matching logic, even though this approach appears counterintuitive.
Applied to files:
languagetool-language-modules/en/src/main/resources/org/languagetool/rules/en/grammar.xml
📚 Learning: 2025-10-15T12:25:46.254Z
Learnt from: AnnaRusalkina
Repo: languagetool-org/languagetool PR: 11574
File: languagetool-language-modules/en/src/main/resources/org/languagetool/rules/en/grammar.xml:109839-109843
Timestamp: 2025-10-15T12:25:46.254Z
Learning: In LanguageTool XML rule files, the `skip` attribute with a negative value (e.g., `skip="-1"`) means "skip forward any number of tokens until the next pattern token is found", allowing patterns to match across arbitrarily long spans. This is documented at https://dev.languagetool.org/development-overview
Applied to files:
languagetool-language-modules/en/src/main/resources/org/languagetool/rules/en/grammar.xml
📚 Learning: 2025-10-15T12:22:12.875Z
Learnt from: AnnaRusalkina
Repo: languagetool-org/languagetool PR: 11574
File: languagetool-language-modules/en/src/main/resources/org/languagetool/rules/en/grammar.xml:139215-139218
Timestamp: 2025-10-15T12:22:12.875Z
Learning: In the English grammar rules XML file, "NN Group" refers to Nielsen Norman Group, a proper noun/company name, and should be matched literally with `<token>NN</token>` rather than as a POS tag.
Applied to files:
languagetool-language-modules/en/src/main/resources/org/languagetool/rules/en/grammar.xml
🔇 Additional comments (1)
languagetool-language-modules/en/src/main/resources/org/languagetool/rules/en/grammar.xml (1)
100995-100995: URL pattern update looks correct.The token pattern correctly matches HTTP/HTTPS and www-prefixed URLs, consistent with the antipattern added above. The past issues regarding the protocol separator and invalid XML syntax have been successfully resolved.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
languagetool-language-modules/en/src/test/java/org/languagetool/rules/en/UrlCapitalizationTest.java (1)
17-55: Consider adding edge case coverage.While the current tests cover the main URL patterns well, you could optionally add tests for:
- URLs with query parameters:
https://example.com?param=value- URLs with fragments/anchors:
https://example.com#section- URLs with ports:
http://localhost:8080/pathThese edge cases would further strengthen confidence that the antipattern correctly handles all URL variations.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
languagetool-language-modules/en/src/test/java/org/languagetool/rules/en/UrlCapitalizationTest.java
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: AnnaRusalkina
Repo: languagetool-org/languagetool PR: 11564
File: languagetool-language-modules/en/src/main/resources/org/languagetool/rules/en/grammar.xml:139958-139966
Timestamp: 2025-10-10T12:48:33.301Z
Learning: In LanguageTool XML grammar rules (grammar.xml files), tokens without the `case_sensitive="yes"` attribute are case-insensitive by default. For example, `<token>pre</token>` will match "pre", "Pre", "PRE", etc. without needing regex patterns like "[Pp]re".
📚 Learning: 2025-12-29T14:12:34.878Z
Learnt from: inesakochur
Repo: languagetool-org/languagetool PR: 11739
File: languagetool-language-modules/pt/src/main/resources/org/languagetool/rules/pt/pt-PT/grammar.xml:2850-2874
Timestamp: 2025-12-29T14:12:34.878Z
Learning: In LanguageTool XML rules, suggestions do not need to be manually capitalized when a rule fires at SENT_START. LanguageTool automatically capitalizes suggestions that are applied at the beginning of a sentence, so lowercase suggestions like "tu" and "eu" will be correctly capitalized to "Tu" and "Eu" when replacing text at sentence start.
Applied to files:
languagetool-language-modules/en/src/test/java/org/languagetool/rules/en/UrlCapitalizationTest.java
🧬 Code graph analysis (1)
languagetool-language-modules/en/src/test/java/org/languagetool/rules/en/UrlCapitalizationTest.java (1)
languagetool-core/src/main/java/org/languagetool/rules/RuleMatch.java (1)
RuleMatch(43-739)
🔇 Additional comments (2)
languagetool-language-modules/en/src/test/java/org/languagetool/rules/en/UrlCapitalizationTest.java (2)
1-15: LGTM! Clean test setup.The package structure, imports, and class initialization follow LanguageTool testing conventions. The reusable
langToolinstance is appropriate for stateless grammar checking.
17-55: LGTM! Comprehensive URL test coverage.The five test methods effectively verify that various URL patterns (HTTPS, HTTP, www, at sentence start, and in middle) are not flagged by grammar rules. The consistent pattern and descriptive names make the tests clear and maintainable.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
languagetool-language-modules/en/src/test/java/org/languagetool/rules/en/UrlCapitalizationTest.java (2)
13-15: Consider test isolation with instance per test.The shared
langToolinstance is a common pattern for performance, but creating a fresh instance in a@Beforemethod would ensure complete test isolation and eliminate any potential state leakage between tests.🔎 Optional refactor for test isolation
public class UrlCapitalizationTest { - private final JLanguageTool langTool = new JLanguageTool(AmericanEnglish.getInstance()); + private JLanguageTool langTool; + + @Before + public void setUp() { + langTool = new JLanguageTool(AmericanEnglish.getInstance()); + }Note: You'll need to add the import:
import org.junit.Before;
17-53: Optional: Consider additional URL edge cases.The current test coverage is solid for the core issue. If you want to be even more thorough, consider testing:
- URLs with paths and query parameters:
https://example.com/path?query=value- URLs with mixed-case domains:
https://Example.COM- Other protocols like
ftp://ormailto:However, these are likely out of scope for the specific issue being addressed, so this is purely optional.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
languagetool-language-modules/en/src/main/resources/org/languagetool/rules/en/grammar.xmllanguagetool-language-modules/en/src/test/java/org/languagetool/rules/en/UrlCapitalizationTest.java
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: AnnaRusalkina
Repo: languagetool-org/languagetool PR: 11564
File: languagetool-language-modules/en/src/main/resources/org/languagetool/rules/en/grammar.xml:139958-139966
Timestamp: 2025-10-10T12:48:33.301Z
Learning: In LanguageTool XML grammar rules (grammar.xml files), tokens without the `case_sensitive="yes"` attribute are case-insensitive by default. For example, `<token>pre</token>` will match "pre", "Pre", "PRE", etc. without needing regex patterns like "[Pp]re".
📚 Learning: 2025-12-04T09:58:23.800Z
Learnt from: AnnaRusalkina
Repo: languagetool-org/languagetool PR: 11688
File: languagetool-language-modules/en/src/main/resources/org/languagetool/rules/en/grammar.xml:134216-134216
Timestamp: 2025-12-04T09:58:23.800Z
Learning: In the file `languagetool-language-modules/en/src/main/resources/org/languagetool/rules/en/grammar.xml`, the comment "Grammatical mistakes are widespread in everyday writting." contains an intentional typo ("writting" instead of "writing") that serves as an example of the type of mistake the antipattern is designed to handle.
Applied to files:
languagetool-language-modules/en/src/main/resources/org/languagetool/rules/en/grammar.xml
📚 Learning: 2025-10-10T12:48:33.301Z
Learnt from: AnnaRusalkina
Repo: languagetool-org/languagetool PR: 11564
File: languagetool-language-modules/en/src/main/resources/org/languagetool/rules/en/grammar.xml:139958-139966
Timestamp: 2025-10-10T12:48:33.301Z
Learning: In LanguageTool XML grammar rules (grammar.xml files), tokens without the `case_sensitive="yes"` attribute are case-insensitive by default. For example, `<token>pre</token>` will match "pre", "Pre", "PRE", etc. without needing regex patterns like "[Pp]re".
Applied to files:
languagetool-language-modules/en/src/main/resources/org/languagetool/rules/en/grammar.xml
📚 Learning: 2025-10-10T13:40:54.056Z
Learnt from: AnnaRusalkina
Repo: languagetool-org/languagetool PR: 11564
File: languagetool-language-modules/en/src/main/resources/org/languagetool/rules/en/grammar.xml:139951-139956
Timestamp: 2025-10-10T13:40:54.056Z
Learning: In LanguageTool XML rules, hyphenated words (e.g., "COVID-19") are tokenized as single tokens, so a single `<token>` pattern with a regex can match them in one go.
Applied to files:
languagetool-language-modules/en/src/main/resources/org/languagetool/rules/en/grammar.xml
📚 Learning: 2025-10-08T06:41:55.119Z
Learnt from: marcoagpinto
Repo: languagetool-org/languagetool PR: 11557
File: languagetool-language-modules/pt/src/main/resources/org/languagetool/resource/pt/disambiguation.xml:4068-4078
Timestamp: 2025-10-08T06:41:55.119Z
Learning: In Portuguese disambiguation rules, when a pattern targets tokens with multiple verb readings (e.g., VMIP3S0 and VMM02S0), including an exception like `<exception postag_regexp='yes' postag='AQ.+'//>` on subsequent participle tokens is necessary to prevent false positives, even though it reduces the number of matches. The rule can still fire successfully for cases where the participle doesn't have an adjective reading, as confirmed by testing showing ~4683 matches across ~950k sentences.
Applied to files:
languagetool-language-modules/en/src/main/resources/org/languagetool/rules/en/grammar.xml
📚 Learning: 2025-07-26T07:31:09.449Z
Learnt from: marcoagpinto
Repo: languagetool-org/languagetool PR: 11458
File: languagetool-language-modules/pt/src/main/resources/org/languagetool/rules/pt/style.xml:3689-3693
Timestamp: 2025-07-26T07:31:09.449Z
Learning: In LanguageTool antipatterns, the `scope='next'` attribute is bugged and removes valid entries, so workarounds using empty `<token>` with `<exception>` tags are sometimes necessary to achieve the desired matching logic, even though this approach appears counterintuitive.
Applied to files:
languagetool-language-modules/en/src/main/resources/org/languagetool/rules/en/grammar.xml
📚 Learning: 2025-10-15T12:25:46.254Z
Learnt from: AnnaRusalkina
Repo: languagetool-org/languagetool PR: 11574
File: languagetool-language-modules/en/src/main/resources/org/languagetool/rules/en/grammar.xml:109839-109843
Timestamp: 2025-10-15T12:25:46.254Z
Learning: In LanguageTool XML rule files, the `skip` attribute with a negative value (e.g., `skip="-1"`) means "skip forward any number of tokens until the next pattern token is found", allowing patterns to match across arbitrarily long spans. This is documented at https://dev.languagetool.org/development-overview
Applied to files:
languagetool-language-modules/en/src/main/resources/org/languagetool/rules/en/grammar.xml
📚 Learning: 2025-12-29T14:12:34.878Z
Learnt from: inesakochur
Repo: languagetool-org/languagetool PR: 11739
File: languagetool-language-modules/pt/src/main/resources/org/languagetool/rules/pt/pt-PT/grammar.xml:2850-2874
Timestamp: 2025-12-29T14:12:34.878Z
Learning: In LanguageTool XML rules, suggestions do not need to be manually capitalized when a rule fires at SENT_START. LanguageTool automatically capitalizes suggestions that are applied at the beginning of a sentence, so lowercase suggestions like "tu" and "eu" will be correctly capitalized to "Tu" and "Eu" when replacing text at sentence start.
Applied to files:
languagetool-language-modules/en/src/test/java/org/languagetool/rules/en/UrlCapitalizationTest.java
🧬 Code graph analysis (1)
languagetool-language-modules/en/src/test/java/org/languagetool/rules/en/UrlCapitalizationTest.java (1)
languagetool-core/src/main/java/org/languagetool/rules/RuleMatch.java (1)
RuleMatch(43-739)
🔇 Additional comments (6)
languagetool-language-modules/en/src/test/java/org/languagetool/rules/en/UrlCapitalizationTest.java (6)
17-21: LGTM: HTTPS URL test is well-structured.This test correctly verifies that a lowercase HTTPS URL within a properly capitalized sentence does not trigger false positive capitalization errors.
23-27: LGTM: HTTP URL test provides good coverage.Testing both secure (HTTPS) and non-secure (HTTP) protocols ensures comprehensive coverage of URL schemes.
29-33: LGTM: WWW prefix test covers protocol-less URLs.Testing
www.prefix without a protocol is important, as this is a common way to write URLs in documentation.
35-39: Excellent: Critical edge case for sentence-initial URLs.This test covers the most important scenario where a URL starts a sentence with a lowercase letter. This is likely the primary use case that Issue #11710 addresses, as sentence-start capitalization rules would typically flag this as an error.
41-45: LGTM: Trailing punctuation edge case is covered.This test ensures that the URL token matching correctly handles punctuation boundaries, preventing URLs followed by commas (or other punctuation) from being incorrectly parsed.
47-53: LGTM: Control test is now correct.This test properly verifies that capitalization checking still works on non-URL text. The assertion
assertTrue(matches > 0)is correct—the text "the united states is large." should trigger at least one capitalization error (for "the" at sentence start and/or "united states" as a proper noun).Note: This addresses the previous review feedback about the inverted assertion.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
languagetool-language-modules/en/src/main/resources/org/languagetool/rules/en/grammar.xml (1)
2127-2130: Verify trailing punctuation behavior with\S+pattern.The pattern
\S+matches all non-whitespace characters, which includes trailing punctuation. For example,https://example.com.would match with the period included. Depending on how LanguageTool tokenizes URLs followed by sentence-ending punctuation, this could either:
- Work correctly if "URL + period" is tokenized as a single token
- Miss cases if the period is tokenized separately
Since the test suite includes "URLs with trailing punctuation," this may already be validated, but it's worth confirming the antipattern fires correctly for inputs like:
"Visit https://example.com. Then...""See www.example.org, then..."Additionally, the pattern doesn't cover bare domains without protocol or
www.prefix (e.g.,github.comorexample.org). If these are commonly used in the corpus and should also skip the capitalization rule, consider extending the pattern as a future enhancement.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
languagetool-language-modules/en/src/main/resources/org/languagetool/rules/en/grammar.xml
🧰 Additional context used
🧠 Learnings (6)
📚 Learning: 2025-12-04T09:58:23.800Z
Learnt from: AnnaRusalkina
Repo: languagetool-org/languagetool PR: 11688
File: languagetool-language-modules/en/src/main/resources/org/languagetool/rules/en/grammar.xml:134216-134216
Timestamp: 2025-12-04T09:58:23.800Z
Learning: In the file `languagetool-language-modules/en/src/main/resources/org/languagetool/rules/en/grammar.xml`, the comment "Grammatical mistakes are widespread in everyday writting." contains an intentional typo ("writting" instead of "writing") that serves as an example of the type of mistake the antipattern is designed to handle.
Applied to files:
languagetool-language-modules/en/src/main/resources/org/languagetool/rules/en/grammar.xml
📚 Learning: 2025-10-10T12:48:33.301Z
Learnt from: AnnaRusalkina
Repo: languagetool-org/languagetool PR: 11564
File: languagetool-language-modules/en/src/main/resources/org/languagetool/rules/en/grammar.xml:139958-139966
Timestamp: 2025-10-10T12:48:33.301Z
Learning: In LanguageTool XML grammar rules (grammar.xml files), tokens without the `case_sensitive="yes"` attribute are case-insensitive by default. For example, `<token>pre</token>` will match "pre", "Pre", "PRE", etc. without needing regex patterns like "[Pp]re".
Applied to files:
languagetool-language-modules/en/src/main/resources/org/languagetool/rules/en/grammar.xml
📚 Learning: 2025-10-10T13:40:54.056Z
Learnt from: AnnaRusalkina
Repo: languagetool-org/languagetool PR: 11564
File: languagetool-language-modules/en/src/main/resources/org/languagetool/rules/en/grammar.xml:139951-139956
Timestamp: 2025-10-10T13:40:54.056Z
Learning: In LanguageTool XML rules, hyphenated words (e.g., "COVID-19") are tokenized as single tokens, so a single `<token>` pattern with a regex can match them in one go.
Applied to files:
languagetool-language-modules/en/src/main/resources/org/languagetool/rules/en/grammar.xml
📚 Learning: 2025-10-08T06:41:55.119Z
Learnt from: marcoagpinto
Repo: languagetool-org/languagetool PR: 11557
File: languagetool-language-modules/pt/src/main/resources/org/languagetool/resource/pt/disambiguation.xml:4068-4078
Timestamp: 2025-10-08T06:41:55.119Z
Learning: In Portuguese disambiguation rules, when a pattern targets tokens with multiple verb readings (e.g., VMIP3S0 and VMM02S0), including an exception like `<exception postag_regexp='yes' postag='AQ.+'//>` on subsequent participle tokens is necessary to prevent false positives, even though it reduces the number of matches. The rule can still fire successfully for cases where the participle doesn't have an adjective reading, as confirmed by testing showing ~4683 matches across ~950k sentences.
Applied to files:
languagetool-language-modules/en/src/main/resources/org/languagetool/rules/en/grammar.xml
📚 Learning: 2025-07-26T07:31:09.449Z
Learnt from: marcoagpinto
Repo: languagetool-org/languagetool PR: 11458
File: languagetool-language-modules/pt/src/main/resources/org/languagetool/rules/pt/style.xml:3689-3693
Timestamp: 2025-07-26T07:31:09.449Z
Learning: In LanguageTool antipatterns, the `scope='next'` attribute is bugged and removes valid entries, so workarounds using empty `<token>` with `<exception>` tags are sometimes necessary to achieve the desired matching logic, even though this approach appears counterintuitive.
Applied to files:
languagetool-language-modules/en/src/main/resources/org/languagetool/rules/en/grammar.xml
📚 Learning: 2025-10-15T12:25:46.254Z
Learnt from: AnnaRusalkina
Repo: languagetool-org/languagetool PR: 11574
File: languagetool-language-modules/en/src/main/resources/org/languagetool/rules/en/grammar.xml:109839-109843
Timestamp: 2025-10-15T12:25:46.254Z
Learning: In LanguageTool XML rule files, the `skip` attribute with a negative value (e.g., `skip="-1"`) means "skip forward any number of tokens until the next pattern token is found", allowing patterns to match across arbitrarily long spans. This is documented at https://dev.languagetool.org/development-overview
Applied to files:
languagetool-language-modules/en/src/main/resources/org/languagetool/rules/en/grammar.xml
🔇 Additional comments (1)
languagetool-language-modules/en/src/main/resources/org/languagetool/rules/en/grammar.xml (1)
100995-100995: Same trailing punctuation concern applies here.This token uses the identical pattern
(https?://|www\.)\S+as the antipattern in lines 2127-2130. The same trailing punctuation consideration applies: verify that URLs followed by sentence-ending punctuation are handled correctly in this rule's context.
|
By making a contribution to this project, I certify that: (a) The contribution was created in whole or in part by me and I have the right to submit it under the open source license indicated in the file; or (b) The contribution is based upon previous work that, to the best of my knowledge, is covered under an appropriate open source license and I have the right under that license to submit that work with modifications, whether created in whole or in part by me, under the same open source license (unless I am permitted to submit under a different license), as indicated in the file; or (c) The contribution was provided directly to me by some other person who certified (a), (b) or (c) and I have not modified it. (d) I understand and agree that this project and the contribution are public and that a record of the contribution (including all personal information I submit with it, including my sign-off) is maintained indefinitely and may be redistributed consistent with this project or the open source license(s) involved. Signed-off-by: Dhruv Gupta dhruvgupta77@gmail.com |
|
Hi @jaumeortola, This is my first contribution. Can you please review this PR? Thanks, |
|
Hi @dhruvgupta77 |
Fixed Issue #11710
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.