-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
[pt] Joined subrules into just one rule, ID:QUE_VEM_PROXIMA #11202
Conversation
WalkthroughThe pull request modifies a Portuguese language style rule in the LanguageTool project. The change simplifies a rule group containing two separate rules into a single, more broadly applicable rule. The modification broadens the rule's scope by adjusting postag matching criteria, allowing it to handle a wider range of noun grammatical categories while maintaining the core functionality of simplifying phrases that include "que vem". Changes
Possibly related PRs
Suggested reviewers
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 (
|
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)
languagetool-language-modules/pt/src/main/resources/org/languagetool/rules/pt/style.xml (2)
4290-4298
: Add comments explaining the pattern matching logic.While the pattern is well-structured, adding XML comments to explain the postag patterns (e.g.,
SPS00:DA.+
,N.+|AQ.+
) would improve maintainability.<pattern> + <!-- Match preposition or article --> <token postag='SPS00:DA.+' postag_regexp='yes'/> <marker> + <!-- Match noun or adjective followed by "que vem" --> <token postag='N.+|AQ.+' postag_regexp='yes'/> <token>que</token> <token regexp='no'>vem</token> </marker> + <!-- Ensure proper phrase termination --> <token postag='V.+|_PUNCT|CC' postag_regexp='yes'/> </pattern>
4301-4302
: Consider adding more diverse examples.While the current examples cover basic gender agreement, consider adding examples that demonstrate:
- Plural forms
- Different prepositions/articles
- Various terminating tokens (verb/punctuation/conjunction)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
languagetool-language-modules/pt/src/main/resources/org/languagetool/rules/pt/style.xml
(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (java-kotlin)
🔇 Additional comments (3)
languagetool-language-modules/pt/src/main/resources/org/languagetool/rules/pt/style.xml (3)
4287-4287
: Verify if the rule should be disabled by default.The rule is currently set to
default='temp_off'
. Consider if this is intentional, as it means the simplification suggestions won't be active by default.
4299-4300
: LGTM! Well-implemented suggestion logic.The suggestion mechanism correctly:
- Preserves grammatical agreement using postag replacement
- Maintains the simplified form while keeping the meaning
4287-4303
: Successfully implements the PR objectives.The changes effectively:
- Combine the previous two rules into a single, more maintainable rule
- Preserve the original functionality while broadening the scope
- Maintain the same transformations as before
Joined the two rules from the rulegroup into just one rule, and the results are identical.
Summary by CodeRabbit