Add helper for plural rules placeholders #131

dbendilas · 2018-12-12T12:15:46Z

Checklist (for the reviewer)

Problem and/or solution

Add a helper function that replaces all serialized plural rule placeholders (hashes), that correspond to the source language, with those that correspond to the given target language rules.

This is useful when the template does not serialize all pluralized content for a string (e.g. "{count, plural, }"), but rather adds one hash placeholder for each rule (e.g. "{count, plural, one {} other {}").

In the latter case, when compiling, the existing source placeholders need to be removed completely, and the target placeholders need to appear in their place.

coveralls · 2018-12-12T12:19:11Z

Coverage increased (+0.02%) to 96.351% when pulling fda56ed on plural-rules-compilation-helper into 6493aaf on devel.

rigaspapas · 2018-12-17T10:16:23Z

openformats/utils/icu.py

+            corresponding strings
+        :rtype: str
+        """
+        return self.serialize_strings(pluralized_string.string, delimiter)


What is the purpose of this method? It just calls another

It was used by the JSON handler. You are right, it doesn't make any sense to keep it.

rigaspapas · 2018-12-17T10:19:11Z

openformats/utils/icu.py

+                                     allow_numeric_plural_values=True):
+    """Update the given content so that it contains all
+    necessary placeholders the target language supports,
+    no more, no less.


The "no more, no less" part can be removed. Or you can specify that this happens "independently of the source language's rules"

I liked the "no more, no less" part, but I like your suggestion more.

rigaspapas · 2018-12-17T10:26:49Z

openformats/utils/icu.py

+            offset += len(line)
+            if separator_pos > -1:
+                offset += sep_length
+            continue  # noqa


Why the # noqa here?

(this was actually replaced with a # pragma: no cover on the next commit, I assume you had commented on the commit diff only)

The reason this was added is that during testing, execution never reached that point. I spent 1-2 hours on this and I am pretty sure that the code was actually reached, but because of the way continue is handled in terms of execution, the coverage script did not understand that. I had to add the no cover mark, otherwise the coverage fell to a point that the PR was blocked from merging.

rigaspapas · 2018-12-17T10:28:39Z

openformats/utils/icu.py

+            offset += len(line)
+            if separator_pos > -1:
+                offset += sep_length
+            continue  # noqa


The 4 lines above are repeated. Should we move them in a different helper and re-use this?

If we made such a helper, we would have to pass 3 arguments (line or its length, separator_pos and sep_length) in order to make it work. And we would still have to manually call continue. So I don't think it makes much sense, it would be just noise.

rigaspapas · 2018-12-17T10:30:30Z

openformats/utils/icu.py

+        strings_by_rule = {
+            rule: hash_str + str(index)
+            for index, rule in enumerate(target_plural_forms)
+        }


Since this method is big, should we move the above 3 commands to a different method?

rigaspapas · 2018-12-17T10:32:04Z

openformats/utils/icu.py

+        # }
+        hash_str = icu_string.strings_by_rule[5]
+        hash_str = hash_str[:-1]  # remove the last char, which is the plural index
+        strings_by_rule = {


I think it's better to rename this as hashes_by_rule or plural_placeholders. There are no actual strings there.

rigaspapas · 2018-12-17T10:37:14Z

openformats/utils/icu.py

+
+        # If there are no more lines, stop iterating
+        if offset > 0 and separator_pos == -1:
+            break


We could make the code more descriptive here. Something like:

no_more_lines = offset > 0 and separator_pos == -1 if no_more_lines: break

Should we move this to the end of the loop and change the while condition to something like: while more_lines?

I tried it and I prefer the current way.

Although I would prefer to not have while True myself, changing the logic to while <condition> requires significant changes due to the the 2 continue blocks we have. I'm reluctant to make such as change at this point.

rigaspapas · 2018-12-17T10:47:30Z

openformats/tests/util_tests/test_icu.py

+        self.assertSetEqual(
+            set(icu_string.strings_by_rule.keys()),
+            rule_set,
+        )


I think that the following cases are missing from the unittests:

A case with \r\n as separator

A case with a line without braces

A case with a line with braces but without ICU contents

Sounds good, I'll amend the tests.

Update: the previous implementation made assumptions about the file format (assumed key-value syntax). The functionality of the ICU methods and functions is now specific to ICU syntax only, agnostic to any specific file format. Therefore, the tests mentioned above are no longer valid.

rigaspapas · 2018-12-18T15:09:44Z

openformats/tests/util_tests/test_icu.py

+            u'keyE=brace {\n'
+        )
+
+    def test_with_alternate_newline_char(self):


I think we should use "alternative", not "alternate"

Test was removed.

rigaspapas · 2018-12-18T15:54:59Z

openformats/utils/icu.py

+        for a specific set of plural rules. This works regardless
+        how many plural rules are found in `icu_string` (source language
+        string) and how many languages are found in `plural_rules`
+        (target language). The reason this works is that all placholders


Typo: placholders

rigaspapas · 2018-12-18T16:00:18Z

openformats/tests/util_tests/test_icu.py

+            serialized,
+        )
+
+    def test_with_less_target_languages(self):


I think that this test should be named test_with_less_target_plural_rules, not languages. Also, the plural rules are equal in quantity but different. Should we add some more tests on this?

Renamed to test_with_less_target_plurals. Added an additional test for equal number of rules, but different types.

Add a helper function that replaces all serialized plural rule placeholders (hashes), that correspond to the source language, with those that correspond to the given target language rules. This is useful when the template does not serialize all pluralized content for a string (e.g. "{count, plural, <hash>}"), but rather adds one hash placeholder for each rule (e.g. "{count, plural, one {<hash1>} other {<hash2>}"). In the latter case, when compiling, the existing source placeholders need to be removed completely, and the target placeholders need to appear in their place.

dbendilas force-pushed the plural-rules-compilation-helper branch from 357f0c8 to d18ea0c Compare December 12, 2018 12:18

dbendilas force-pushed the plural-rules-compilation-helper branch 2 times, most recently from 78f4003 to 2c2e449 Compare December 12, 2018 15:11

rigaspapas suggested changes Dec 17, 2018

View reviewed changes

rigaspapas reviewed Dec 18, 2018

View reviewed changes

dbendilas force-pushed the plural-rules-compilation-helper branch from 86a699f to fda56ed Compare December 19, 2018 13:25

rigaspapas approved these changes Dec 19, 2018

View reviewed changes

dbendilas merged commit 3313c26 into devel Dec 19, 2018

dbendilas deleted the plural-rules-compilation-helper branch December 19, 2018 13:36

wyngarde mentioned this pull request Dec 19, 2018

openformats update (0.0.46) #133

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add helper for plural rules placeholders #131

Add helper for plural rules placeholders #131

dbendilas commented Dec 12, 2018 •

edited by rigaspapas

coveralls commented Dec 12, 2018 •

edited

rigaspapas Dec 17, 2018

dbendilas Dec 17, 2018

rigaspapas Dec 17, 2018

dbendilas Dec 17, 2018

rigaspapas Dec 17, 2018

dbendilas Dec 17, 2018

rigaspapas Dec 17, 2018

dbendilas Dec 17, 2018

rigaspapas Dec 17, 2018

rigaspapas Dec 17, 2018

rigaspapas Dec 17, 2018

dbendilas Dec 17, 2018

rigaspapas Dec 17, 2018

dbendilas Dec 17, 2018

dbendilas Dec 18, 2018

rigaspapas Dec 18, 2018

dbendilas Dec 19, 2018

rigaspapas Dec 18, 2018

rigaspapas Dec 18, 2018

dbendilas Dec 19, 2018

Add helper for plural rules placeholders #131

Add helper for plural rules placeholders #131

Conversation

dbendilas commented Dec 12, 2018 • edited by rigaspapas

Checklist (for the reviewer)

Problem and/or solution

coveralls commented Dec 12, 2018 • edited

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

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

dbendilas commented Dec 12, 2018 •

edited by rigaspapas

coveralls commented Dec 12, 2018 •

edited