Re: [PATCH] Completed unaccent dictionary with many missing characters

Michael Paquier wrote on 20.06.2022 03:49:

On Wed, Jun 15, 2022 at 01:01:37PM +0200, Przemysław Sztoch wrote:

Two fixes (bad comment and fixed Latin-ASCII.xml).

         if codepoint.general_category.startswith('L') and \
-           len(codepoint.combining_ids) > 1:
+           len(codepoint.combining_ids) > 0:
So, this one checks for the case where a codepoint is within the
letter category.  As far as I can see this indeed adds a couple of
characters, with a combination of Greek and Latin letters.  So that
looks fine.

Previously, there were only multi-letter conversions. Now we also have single letters.

+        elif codepoint.general_category.startswith('N') and \
+           len(codepoint.combining_ids) > 0 and \
+           args.noLigaturesExpansion is False and is_ligature(codepoint, table):
+            charactersSet.add((codepoint.id,
+                               "".join(chr(combining_codepoint.id)
+                                       for combining_codepoint
+                                       in get_plain_letters(codepoint, table))))
And this one is for the numerical part of the change.  Do you actually
need to apply is_ligature() here?  I would have thought that this only
applies to letters.

But ligature check is performed on combining_ids (result of translation), not on base codepoint.
Without it, you will get assertions in get_plain_letters.

The idea is that we take translations that turn into normal letters. Others (strange) are rejected.
Maybe it could be done better. I didn't like it as much as you did, but I couldn't do better.
In the end, I left it just like in the original script.

Note that the plain letter list (PLAIN_LETTER_RANGES) has now been expanded with numbers.

-    assert(False)
+    assert False, 'Codepoint U+%0.2X' % codepoint.id
[...]
-    assert(is_ligature(codepoint, table))
+    assert is_ligature(codepoint, table), 'Codepoint U+%0.2X' % codepoint.id
These two are a good idea for debugging.

-    return all(is_letter(table[i], table) for i in codepoint.combining_ids)
+    return all(i in table and is_letter(table[i], table) for i in codepoint.combining_ids)
It looks like this makes the code weaker, as we would silently skip
characters that are not part of the table rather than checking for
them all the time?

Unfortunately, there are entries in combining_ids that are not in the character table being used.
This protection is necessary so that there is no error. But unfamiliar characters are omitted.

While recreating unaccent.rules with your patch, I have noticed what
looks like an error.  An extra rule mapping U+210C (black-letter
capital h) to "x" gets added on top of te existing one for "H", but
the correct answer is the existing rule, not the one added by the
patch.

The problem with the sign of U+210C is that there are conflicting translations for it.
As the name suggests "(black-letter capital h)", it should be converted to a capital H.
However, the current Latin-ASCII.xml suggests a conversion to x.
I found an open discussion on the internet about this and the suggestion that the Latin-ASCII.xml file should be corrected for this letter.
But I wouldn't expect that Unicode makes the revised Latin-ASCII.xml quickly into the official repo.