Skip to content
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-PT] Improved antipattern in rule ID:POSSESSIVE_WITHOUT_ARTICLE #9376

Merged
merged 6 commits into from
Sep 25, 2023

Conversation

marcoagpinto
Copy link
Member

Heya, Susana and Pedro,

This enhancement fixes false positives that have been around for a very long time, and I should have done it before.

But it is better late, than never.

❤️ ❤️ ❤️ ❤️ ❤️

@marcoagpinto
Copy link
Member Author

And another antipattern for other rule (id:PARA-POR_TER_PARTICIPIO-PASSADO)

Copy link
Collaborator

@p-goulart p-goulart left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR includes disparate changes to unrelated rules, IMO it should be split into multiple PRs.


We've finished our (Git documentation)[https://dev.languagetool.org/pt/git] for Portuguese-language contributors, do have a look, it might help guide you.

@@ -1398,6 +1398,12 @@ Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA.

<rulegroup id='GENERAL_GENDER_AGREEMENT_ERRORS' name="Concordância: Geral">
<url>https://pt.wikibooks.org/wiki/Portugu%C3%AAs/Concord%C3%A2ncia/Concord%C3%A2ncia_nominal</url>
<antipattern>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This antipattern technically allows for:

o que você acha dos outros regras?
quero dar mais consistência aos outros regras

Both of which are incorrect, so I don't think we should be introducing this.


The key word here is the verb. If you have a verb that governs a certain preposition (such as 'impor', in your example), then we should allow these constructions. I'd suggest adding an extra token accounting for this verb.

@@ -8426,6 +8432,15 @@ Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA.
<example>as bagagens não convencionais agora desaparecerão</example>
</antipattern>

<antipattern>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The key blocker of agreement here is the verb in the first-person singular, not the whole construction. I'd rethink this.

<token postag='DP1.+' postag_regexp='yes'/>
<token regexp='yes'>car[ao]s?|estimad[ao]s?|excelentes?|excelentíssim[ao]s?|querid[ao]s?</token>
<token regexp='yes'>adoráv(el|eis)|amantes?|amig[ao]s?|car[ao]s?|clientes?|colaborador(es)?|colegas?|companheir[ao]s?|empregad[ao]s?|estimad[ao]s?|excelentes?|excelentíssim[ao]s?|filh[ao]s?|funcionári[ao]s?|inimig[ao]s?|irmã(s|o|os)?|mães?|médic[ao]s?|pais?|prim[ao]s?|professor(res)?|querid[ao]s?</token>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick 🥜] to avoid this type of thing, why not just a list of words with a POS tag filter for nouns/adjectives, and inflected="yes"?

@marcoagpinto
Copy link
Member Author

@p-goulart

Sure, thanks for the feedback.

I will fix everything at night, and tomorrow morning you will see the new code.

@marcoagpinto
Copy link
Member Author

@p-goulart @susanaboatto

Regarding the other rule “ir no ginásio” which I referred last week, it is too complex, and it won't be ready for this release of LanguageTool in a few days.

@p-goulart
Copy link
Collaborator

I will fix everything at night, and tomorrow morning you will see the new code.

Sure, there's no rush. Just try to keep unrelated changes on different branches (and therefore PRs), because we could already have merged a couple of your changes! 😢

@marcoagpinto
Copy link
Member Author

Sure, there's no rush. Just try to keep unrelated changes on different branches (and therefore PRs), because we could already have merged a couple of your changes! 😢

Ahhhh… sorry… I thought I should only do that for rules and for APs add all in the same branch.

We are always learning, I will do better.

@p-goulart
Copy link
Collaborator

Aha, I understand your confusion. No, it's generally a better idea to keep everything separate. No worries, though, these APs should be easy enough to fix.

@marcoagpinto
Copy link
Member Author

@p-goulart

Heya,

I believe I have resolved the issues.

Please have a look when you have the time.

Regarding the “as unhas não pude arranjar” I simply deleted the AP as I am too stressed to rethink the logic of it.

I simply made the changes you requested and did a TESTRULES pt and TESTRULES pt-PT to check if they broke anything.

From now on, I will create a branch for each rule and for each AP, this will make reviewing easier.

Thanks!

@p-goulart
Copy link
Collaborator

Regarding the “as unhas não pude arranjar” I simply deleted the AP as I am too stressed to rethink the logic of it.

My larger point here is that it is the 1SG inflection of the verb that makes that construction allowable. The fronting of the direct object is only incidental to that construction being incorrect. You can delete that rule, but we plan to do some larger work with agreement rules in the near future and I am pretty sure that we will greatly reduce the number of instances of 1SG verbs tagged.


From now on, I will create a branch for each rule and for each AP, this will make reviewing easier.

I think you can keep antipatterns of the same rule within the same PR, no need to split them. Unless, of course, they are each massive and there's a high probability one of them will need more work.

@marcoagpinto marcoagpinto merged commit 63d09c2 into master Sep 25, 2023
2 checks passed
@marcoagpinto marcoagpinto deleted the lt_marcoagpinto_20230923_1214 branch September 25, 2023 17:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants