[ar] Fix some bugs and add new rules #6785

sohaibafifi · 2022-06-14T18:12:50Z

Fix sohaibafifi#19 and add many new rules:

ArabicTransVerbDirectToIndirectRule
ArabicTransVerbIndirectToDirectRule
ArabicTransVerbIndirectToIndirectRule
ArabicInflectedOneWordReplaceRule

TODO:

Cleanup commented code
Update Copyright headers
Remove duplicated code

sohaibafifi · 2022-06-20T07:49:53Z

Hello @danielnaber,

Can you please review this PR before the next version freeze?

Thank you

danielnaber · 2022-06-20T08:35:31Z

...anguage-modules/ar/src/main/java/org/languagetool/rules/ar/AdjectiveToExclamationFilter.java

+    try {
+      adjTokenIndex = Integer.valueOf(arguments.get("adj_pos")) - 1;
+    } catch (NumberFormatException e) {
+      e.printStackTrace();


We shouldn't print stacktraces like this - if this is an internal bug, the exception should be thrown. Otherwise, it should be logged with the logging framework we use elsewhere.

danielnaber · 2022-06-20T08:35:43Z

...anguage-modules/ar/src/main/java/org/languagetool/rules/ar/AdjectiveToExclamationFilter.java

+    // generate suggestion
+    List<String> suggestionList = prepareSuggestions(compList, noun);
+    for (String sug : suggestionList) {
+


danielnaber · 2022-06-20T08:35:53Z

...anguage-modules/ar/src/main/java/org/languagetool/rules/ar/AdjectiveToExclamationFilter.java

+
+  /* prepare suggesiyton for a list of comparative */
+  protected static List<String> prepareSuggestions(List<String> compList, String noun) {
+


empty lines

danielnaber · 2022-06-20T08:36:16Z

...anguage-modules/ar/src/main/java/org/languagetool/rules/ar/AdjectiveToExclamationFilter.java

+
+  /* test if the word is an isolated pronoun */
+  private static boolean isPronoun(String word) {
+    if (word == null)


Please use {..} even for single line if statements

danielnaber · 2022-06-20T08:36:42Z

...anguage-modules/ar/src/main/java/org/languagetool/rules/ar/AdjectiveToExclamationFilter.java

+    return isolatedToAttachedPronoun.getOrDefault(word, "");
+  }
+
+


danielnaber · 2022-06-20T08:38:23Z

...uage-modules/ar/src/main/java/org/languagetool/rules/ar/ArabicPunctuationWhitespaceRule.java

+    return "ARABIC_COMMA_PARENTHESIS_WHITESPACE2";
+  }
+
+  //@Override


why is this commented out?

danielnaber · 2022-06-20T08:39:47Z

...-modules/ar/src/main/java/org/languagetool/rules/ar/ArabicTransVerbDirectToIndirectRule.java

+import static java.lang.Math.min;
+
+/**
+ * TODO : Sohaib - check the duplicated code


should this be done before merging the PR?

Actually, many codes come from the English part. I was thinking of a way to reduce such duplications.

Yes, please clean up the duplication before we merge the PR.

danielnaber · 2022-06-20T08:40:09Z

...-modules/ar/src/main/java/org/languagetool/rules/ar/ArabicTransVerbDirectToIndirectRule.java

+  }
+
+  private boolean isCandidateVerb(AnalyzedToken mytoken) {
+


danielnaber · 2022-06-20T08:40:21Z

...-modules/ar/src/main/java/org/languagetool/rules/ar/ArabicTransVerbDirectToIndirectRule.java

+
+  private boolean isCandidateVerb(AnalyzedToken mytoken) {
+
+    if (getSuggestedPreposition(mytoken) != null)


danielnaber · 2022-06-20T08:41:14Z

...agetool-language-modules/ar/src/main/java/org/languagetool/rules/ar/ArabicWordinessRule.java

+      try {
+        adjTokenIndex = Integer.valueOf(arguments.get("adj_pos")) - 1;
+      } catch (NumberFormatException e) {
+        e.printStackTrace();


log or throw

danielnaber · 2022-06-20T08:45:33Z

@sohaibafifi I've started a review, but I see a lot of TODO and FIXME. Are you sure the code is ready to be merged? How have you tested it (other than having unit tests)? Please also make sure the code has no random empty lines etc and complies at https://dev.languagetool.org/code-style. In the future, it would be better to have several smaller PRs, as reviewing such a massive change set is a lot of work.

danielnaber

Please make sure all the formal issues (CamelCase instead of under_scores, indent, {} for single-line if-statements etc.) are cleaned up first

danielnaber · 2022-06-21T10:20:38Z

...anguage-modules/ar/src/main/java/org/languagetool/rules/ar/AdjectiveToExclamationFilter.java

+    try {
+      adjTokenIndex = Integer.valueOf(arguments.get("adj_pos")) - 1;
+    } catch (NumberFormatException e) {
+        throw new RuntimeException("Error parsing adj_pos from : " + arguments.get("adj_pos"));


too much indent

danielnaber · 2022-06-21T10:21:19Z

...ge-modules/ar/src/main/java/org/languagetool/rules/ar/ArabicInflectedOneWordReplaceRule.java

+      for (AnalyzedToken wordTok : token.getReadings()) {
+        // test if the first token is a to replace word
+        boolean is_candidate_word = isCandidateWord(wordTok);
+        if (is_candidate_word) {


Please use CamelCase

danielnaber · 2022-06-21T10:21:42Z

...ge-modules/ar/src/main/java/org/languagetool/rules/ar/ArabicInflectedOneWordReplaceRule.java

+          if (propositionsWithMessage != null) {
+            propositions = Arrays.asList(propositionsWithMessage.getSuggestion().split("\\|"));
+            sug_msg = propositionsWithMessage.getMessage();
+            sug_msg = sug_msg != null ? sug_msg : "";


danielnaber

Sorry, this won't make it into LT 5.8 anymore, but we can merge it after the release once the remaining issues get cleaned up. Please make sure there's no duplication, each class should have a test (or have its test updated), and there should be no random empty lines that are not useful to structure the code (e.g. an empty line before } is rarely useful).

danielnaber · 2022-06-22T06:26:05Z

...anguage-modules/ar/src/main/java/org/languagetool/rules/ar/AdjectiveToExclamationFilter.java

+    try {
+      adjTokenIndex = Integer.valueOf(arguments.get("adj_pos")) - 1;
+    } catch (NumberFormatException e) {
+      throw new RuntimeException("Error parsing adj_pos from : " + arguments.get("adj_pos"));


Please use e as the second argument here, so no information is lost.

danielnaber · 2022-06-22T06:26:44Z

...anguage-modules/ar/src/main/java/org/languagetool/rules/ar/AdjectiveToExclamationFilter.java

+    return newMatch;
+  }
+
+  /* prepare suggesiyton for a list of comparative */


typo "suggesiyton"

danielnaber · 2022-06-22T06:27:38Z

...ge-modules/ar/src/main/java/org/languagetool/rules/ar/ArabicInflectedOneWordReplaceRule.java

+    super.setCategory(Categories.MISC.getCategory(messages));
+    setLocQualityIssueType(ITSIssueType.Inconsistency);
+    addExamplePair(Example.wrong("أجريت <marker>أبحاثا</marker> في المخبر"),
+      Example.fixed("أجريت <marker>بحوثا</marker> في المخبر."));


please indent so that the Examples are aligned

...uage-modules/ar/src/main/java/org/languagetool/rules/ar/ArabicPunctuationWhitespaceRule.java

danielnaber · 2022-06-22T06:31:09Z

...agetool-language-modules/ar/src/main/java/org/languagetool/rules/ar/ArabicTransVerbData.java

+  /**
+   * Load words, normalized to lowercase unless starting with '*'.
+   */
+  private static Set<String> loadWords(String path) {


duplicated from AvsAnData, can this be moved to a Tools method?

danielnaber · 2022-06-22T06:44:12Z

...uagetool-language-modules/ar/src/main/java/org/languagetool/rules/ar/NumberPhraseFilter.java

+        String postag = tk.getPOSTag();
+        if (tagmanager.isNoun(postag) && !tagmanager.isDefinite(postag) && !tagmanager.hasPronoun(postag)) {
+          // add inflection flag
+          if (inflection == "jar") {


Does == really work here or is equals needed?

danielnaber · 2022-06-22T06:44:39Z

...uagetool-language-modules/ar/src/main/java/org/languagetool/rules/ar/NumberPhraseFilter.java

+        nextPos = size + nextPos;
+      }
+    } catch (NumberFormatException e) {
+      e.printStackTrace();


Really print?

danielnaber · 2022-06-22T06:45:52Z

...uagetool-language-modules/ar/src/main/java/org/languagetool/tagging/ar/ArabicTagManager.java

+    } catch (StringIndexOutOfBoundsException e) {
+      int pos = getFlagPos(postag, flagType);
+      // debug only
+      System.out.println("ArabicTagmanager:Exception: pos flag" + Integer.toString(pos) + "flagtype:" + flagType + " postag:" + tmp + " len:" + Integer.toString(tmp.length()));


Should be logging or exception, but not System.out.println

danielnaber · 2022-06-22T06:46:02Z

...uagetool-language-modules/ar/src/main/java/org/languagetool/tagging/ar/ArabicTagManager.java

+      newposTag = setFlag(newposTag, "CONJ", '-');
+      newposTag = setFlag(newposTag, "JAR", '-');
+      // if the word is a definated word, set the definition flag
+      if (isDefinite(postag))


danielnaber · 2022-06-22T06:48:30Z

...anguage-modules/ar/src/test/java/org/languagetool/rules/ar/ArabicMasdarToVerbFilterTest.java

+      }
+      // show only no suggestion cases
+      if (!suggestion.startsWith("[ي")) {
+        System.out.println("مثال: " + word1 + " " + word2 + " مقترح:" + suggestion);


There are usually not System.out.println in tests other than those that print progress. Is this an error? Then the test should fail.

danielnaber · 2022-07-04T21:08:19Z

Is this ready to be reviewed now?

danielnaber · 2022-12-18T10:29:49Z

@sohaibafifi It seems not all my comments from https://github.com/languagetool-org/languagetool/pull/6785/files were addressed. Also, could you please turn this into smaller PRs? This one is too large to be reviewed. There should be one PR per new rule, for example, and whitespace changes should be a separate commit.

sohaibafifi · 2022-12-18T10:47:54Z

I’m very sorry, I'm afraid it's going to be a little complicated this time. we'll be careful the next time.

sohaibafifi · 2023-05-24T05:52:36Z

Closed and replaced by #8286, #8287 and #8288.

[ar] Fix #19 and add many new rules

cc30352

sohaibafifi changed the title ~~[ar] Fix https://github.com/sohaibafifi/languagetool/issues/19 and ad…~~ [ar] Fix sohaibafifi#19 and add many new rules Jun 14, 2022

sohaibafifi changed the title ~~[ar] Fix sohaibafifi#19 and add many new rules~~ [ar] Fix some bugs add many new rules Jun 14, 2022

sohaibafifi added 3 commits June 14, 2022 21:01

[ar] update headers

c17eca7

[ar] update CHANGES.md

34fa6c5

[ar] cleanup the code

4fea22e

sohaibafifi marked this pull request as ready for review June 14, 2022 19:08

sohaibafifi changed the title ~~[ar] Fix some bugs add many new rules~~ [ar] Fix some bugs and add new rules Jun 15, 2022

danielnaber requested changes Jun 20, 2022

View reviewed changes

sohaibafifi added 2 commits June 21, 2022 10:44

[ar] cleanup the code according to reviews

f075f90

[ar] remove empty lines

c1d152d

sohaibafifi requested a review from danielnaber June 21, 2022 08:56

danielnaber requested changes Jun 21, 2022

View reviewed changes

sohaibafifi added 2 commits June 21, 2022 16:32

[ar] Fix code styling

ac78e8a

[ar] Fix code styling

abe332a

sohaibafifi requested a review from danielnaber June 21, 2022 21:24

danielnaber requested changes Jun 22, 2022

View reviewed changes

sohaibafifi marked this pull request as draft July 6, 2022 13:24

[ar] Fix code styling

4d7fa67

sohaibafifi requested a review from danielnaber December 18, 2022 10:03

sohaibafifi marked this pull request as ready for review December 18, 2022 10:03

sohaibafifi and others added 2 commits December 18, 2022 13:36

[ar] Fix code styling

6676b94

Merge branch 'master' into 5.8

cb3fe36

sohaibafifi closed this May 24, 2023

sohaibafifi mentioned this pull request May 24, 2023

[ar] add filters and many new xml rules #8288

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ar] Fix some bugs and add new rules #6785

[ar] Fix some bugs and add new rules #6785

sohaibafifi commented Jun 14, 2022 •

edited

Loading

sohaibafifi commented Jun 20, 2022

danielnaber Jun 20, 2022

danielnaber Jun 20, 2022

danielnaber Jun 20, 2022

danielnaber Jun 20, 2022

danielnaber Jun 20, 2022

danielnaber Jun 20, 2022

danielnaber Jun 20, 2022

sohaibafifi Jun 20, 2022

danielnaber Jun 22, 2022

danielnaber Jun 20, 2022

danielnaber Jun 20, 2022

danielnaber Jun 20, 2022

danielnaber commented Jun 20, 2022

danielnaber left a comment

danielnaber Jun 21, 2022

danielnaber Jun 21, 2022

danielnaber Jun 21, 2022

danielnaber left a comment

danielnaber Jun 22, 2022

danielnaber Jun 22, 2022

danielnaber Jun 22, 2022

danielnaber Jun 22, 2022

danielnaber Jun 22, 2022

danielnaber Jun 22, 2022

danielnaber Jun 22, 2022

danielnaber Jun 22, 2022

danielnaber Jun 22, 2022

danielnaber commented Jul 4, 2022

danielnaber commented Dec 18, 2022

sohaibafifi commented Dec 18, 2022 •

edited

Loading

sohaibafifi commented May 24, 2023


		/* prepare suggesiyton for a list of comparative */
		protected static List<String> prepareSuggestions(List<String> compList, String noun) {


		private boolean isCandidateVerb(AnalyzedToken mytoken) {

		if (getSuggestedPreposition(mytoken) != null)

[ar] Fix some bugs and add new rules #6785

[ar] Fix some bugs and add new rules #6785

Conversation

sohaibafifi commented Jun 14, 2022 • edited Loading

sohaibafifi commented Jun 20, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielnaber commented Jun 20, 2022

danielnaber left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielnaber left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielnaber commented Jul 4, 2022

danielnaber commented Dec 18, 2022

sohaibafifi commented Dec 18, 2022 • edited Loading

sohaibafifi commented May 24, 2023

sohaibafifi commented Jun 14, 2022 •

edited

Loading

sohaibafifi commented Dec 18, 2022 •

edited

Loading