Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1258833004:
Compile plural strings to android <plurals> elem (Closed)
|
Created:
5 years, 4 months ago by conleyo Modified:
5 years, 4 months ago Reviewers:
newt (away) CC:
grit-developer_googlegroups.com Base URL:
https://chromium.googlesource.com/external/grit-i18n.git@master Target Ref:
refs/heads/master Visibility:
Public. |
DescriptionCompile plural strings to android <plurals> elem
In a .grd file, we can write a plural string like this:
<message name="IDS_THINGS">
{NUM_THINGS, plural,
=1 {1 thing}
other {# things}}
</message>
The android equivalent looks like this:
<plurals name="things">
<item quantity="one">1 thing</item>
<item quantity="other">%d things</item>
</plurals>
However, currently the raw value is being stored as a <string> element.
This change introduces some basic (certainly not fool-proof) parsing of
the plural message in a .grd file and outputs it in the proper android
format.
BUG=
R=newt@chromium.org
Committed: https://code.google.com/p/grit-i18n/source/detail?r=192
Patch Set 1 #
Total comments: 18
Patch Set 2 : Handled comments #
Total comments: 6
Patch Set 3 : Handled new review comments #Patch Set 4 : Removed debugging output (whoops) #Patch Set 5 : Added a test #
Total comments: 3
Patch Set 6 : Removed debugging output again again again (sorry) #Messages
Total messages: 23 (5 generated)
newt@chromium.org changed reviewers: + newt@chromium.org
Thanks for this change. I think it'll nice to have this available for Android strings. Lots of inline comments and two comments that didn't belong inline: 1. Mention "ICU plural syntax" in the commit message and the docstring. (or is there some other more official name for this syntax?) 2. Please update android_xml_unittest.py to test this feature. https://codereview.chromium.org/1258833004/diff/1/grit/format/android_xml.py File grit/format/android_xml.py (right): https://codereview.chromium.org/1258833004/diff/1/grit/format/android_xml.py#... grit/format/android_xml.py:106: sys.stderr.write('outputting to %s\n' % output_dir) Looks like debugging code. Remove this? https://codereview.chromium.org/1258833004/diff/1/grit/format/android_xml.py#... grit/format/android_xml.py:141: def _LoadPluralMessage(message): Please add a docstring comment. This method is far from trivial and its working aren't obvious from the name. In particular, I think you should include the example inputs and outputs you gave in commit message. https://codereview.chromium.org/1258833004/diff/1/grit/format/android_xml.py#... grit/format/android_xml.py:142: import re move import to top of file https://codereview.chromium.org/1258833004/diff/1/grit/format/android_xml.py#... grit/format/android_xml.py:144: plural_match = re.match(r'\{[A-Z_]+,\s*plural,(.*)\}$', message, re.S) Pre-compile these patterns, since they're used so many times. Grit has a class called lazy_re that you should use. https://codereview.chromium.org/1258833004/diff/1/grit/format/android_xml.py#... grit/format/android_xml.py:147: body = plural_match.group(1).strip() To make this more readable, I'd suggest naming the groups, e.g. (?P<mygroup>\w+) https://codereview.chromium.org/1258833004/diff/1/grit/format/android_xml.py#... grit/format/android_xml.py:148: for item_match in re.finditer(r'(.*)\s*\{(.*?)\}', body): I'd replace the initial .* with \S+ since you want to match only non-whitespace characters. https://codereview.chromium.org/1258833004/diff/1/grit/format/android_xml.py#... grit/format/android_xml.py:151: number_match = re.match(r'=(.*)', quantity) Explicit checks here would be better, e.g. if quantity == '=0': ret['zero'] = value ... elif quantity == 'other': ret['other'] = value You should also check for "few" and "many". Since Android doesn't support any values except zero, one, two, few, many, other, we should raise an Exception if the quantity is something else (rather than wait for a runtime failure). https://codereview.chromium.org/1258833004/diff/1/grit/format/android_xml.py#... grit/format/android_xml.py:171: plurals = _LoadPluralMessage(value) How about calling this _FormatPluralMessage() and moving the logic in lines 191-194 into it? It could return None if the message isn't a plural, or the formatted result if it is a plural. https://codereview.chromium.org/1258833004/diff/1/grit/format/android_xml.py#... grit/format/android_xml.py:174: value = xml.sax.saxutils.escape(value, {"'": "\\'", '"': '\\"'}) these replacements need to happen even for plural messages. (but the line below should only happen for non-plural messages)
Quick and thorough review. Thanks! https://codereview.chromium.org/1258833004/diff/1/grit/format/android_xml.py File grit/format/android_xml.py (right): https://codereview.chromium.org/1258833004/diff/1/grit/format/android_xml.py#... grit/format/android_xml.py:106: sys.stderr.write('outputting to %s\n' % output_dir) On 2015/08/03 23:06:40, newt wrote: > Looks like debugging code. Remove this? Done. https://codereview.chromium.org/1258833004/diff/1/grit/format/android_xml.py#... grit/format/android_xml.py:141: def _LoadPluralMessage(message): On 2015/08/03 23:06:40, newt wrote: > Please add a docstring comment. This method is far from trivial and its working > aren't obvious from the name. In particular, I think you should include the > example inputs and outputs you gave in commit message. Done. https://codereview.chromium.org/1258833004/diff/1/grit/format/android_xml.py#... grit/format/android_xml.py:142: import re On 2015/08/03 23:06:40, newt wrote: > move import to top of file Done. https://codereview.chromium.org/1258833004/diff/1/grit/format/android_xml.py#... grit/format/android_xml.py:144: plural_match = re.match(r'\{[A-Z_]+,\s*plural,(.*)\}$', message, re.S) On 2015/08/03 23:06:40, newt wrote: > Pre-compile these patterns, since they're used so many times. Grit has a class > called lazy_re that you should use. Done. https://codereview.chromium.org/1258833004/diff/1/grit/format/android_xml.py#... grit/format/android_xml.py:147: body = plural_match.group(1).strip() On 2015/08/03 23:06:40, newt wrote: > To make this more readable, I'd suggest naming the groups, e.g. (?P<mygroup>\w+) Done. https://codereview.chromium.org/1258833004/diff/1/grit/format/android_xml.py#... grit/format/android_xml.py:148: for item_match in re.finditer(r'(.*)\s*\{(.*?)\}', body): On 2015/08/03 23:06:40, newt wrote: > I'd replace the initial .* with \S+ since you want to match only non-whitespace > characters. Done. https://codereview.chromium.org/1258833004/diff/1/grit/format/android_xml.py#... grit/format/android_xml.py:151: number_match = re.match(r'=(.*)', quantity) On 2015/08/03 23:06:40, newt wrote: > Explicit checks here would be better, e.g. > > if quantity == '=0': > ret['zero'] = value > ... > elif quantity == 'other': > ret['other'] = value > > You should also check for "few" and "many". Since Android doesn't support any > values except zero, one, two, few, many, other, we should raise an Exception if > the quantity is something else (rather than wait for a runtime failure). Done. https://codereview.chromium.org/1258833004/diff/1/grit/format/android_xml.py#... grit/format/android_xml.py:171: plurals = _LoadPluralMessage(value) On 2015/08/03 23:06:40, newt wrote: > How about calling this _FormatPluralMessage() and moving the logic in lines > 191-194 into it? It could return None if the message isn't a plural, or the > formatted result if it is a plural. Done. https://codereview.chromium.org/1258833004/diff/1/grit/format/android_xml.py#... grit/format/android_xml.py:174: value = xml.sax.saxutils.escape(value, {"'": "\\'", '"': '\\"'}) On 2015/08/03 23:06:40, newt wrote: > these replacements need to happen even for plural messages. (but the line below > should only happen for non-plural messages) Done.
https://codereview.chromium.org/1258833004/diff/20001/grit/format/android_xml.py File grit/format/android_xml.py (right): https://codereview.chromium.org/1258833004/diff/20001/grit/format/android_xml... grit/format/android_xml.py:143: """Compile ICU plural syntax to the body of an Android <plurals> element. nit: "Compiles" https://codereview.chromium.org/1258833004/diff/20001/grit/format/android_xml... grit/format/android_xml.py:171: body_out = u'' A better idiom for concatenating strings in python (to avoid quadratic behavior): out_lines = '' ... out_lines.append(...) return '\n'.join(out_lines) https://codereview.chromium.org/1258833004/diff/20001/grit/format/android_xml... grit/format/android_xml.py:181: else: Why not handle "=2", "few" and "many"?
https://codereview.chromium.org/1258833004/diff/20001/grit/format/android_xml.py File grit/format/android_xml.py (right): https://codereview.chromium.org/1258833004/diff/20001/grit/format/android_xml... grit/format/android_xml.py:143: """Compile ICU plural syntax to the body of an Android <plurals> element. On 2015/08/04 03:14:26, newt wrote: > nit: "Compiles" Done. https://codereview.chromium.org/1258833004/diff/20001/grit/format/android_xml... grit/format/android_xml.py:171: body_out = u'' On 2015/08/04 03:14:26, newt wrote: > A better idiom for concatenating strings in python (to avoid quadratic > behavior): > > out_lines = '' > ... > out_lines.append(...) > > return '\n'.join(out_lines) Done. https://codereview.chromium.org/1258833004/diff/20001/grit/format/android_xml... grit/format/android_xml.py:181: else: On 2015/08/04 03:14:26, newt wrote: > Why not handle "=2", "few" and "many"? Done.
Looks good. Just waiting for changes to android_xml_unittest.py.
On 2015/08/04 17:32:54, newt wrote: > Looks good. Just waiting for changes to android_xml_unittest.py. Test added
lgtm after one last comment https://codereview.chromium.org/1258833004/diff/70001/grit/format/android_xml... File grit/format/android_xml_unittest.py (right): https://codereview.chromium.org/1258833004/diff/70001/grit/format/android_xml... grit/format/android_xml_unittest.py:83: self.assertEqual(output.strip(), expected.strip(), msg=output.strip() + '\n' + expected.strip()) why this custom msg? assertEqual() already prints out the expected and actual values?
https://codereview.chromium.org/1258833004/diff/70001/grit/format/android_xml... File grit/format/android_xml_unittest.py (right): https://codereview.chromium.org/1258833004/diff/70001/grit/format/android_xml... grit/format/android_xml_unittest.py:83: self.assertEqual(output.strip(), expected.strip(), msg=output.strip() + '\n' + expected.strip()) On 2015/08/04 18:15:11, newt wrote: > why this custom msg? assertEqual() already prints out the expected and actual > values? Sorry, debugging output. The standard string diff message is practically worthless. In my personal python projects I always override assertEqual with a more descriptive method.
https://codereview.chromium.org/1258833004/diff/70001/grit/format/android_xml... File grit/format/android_xml_unittest.py (right): https://codereview.chromium.org/1258833004/diff/70001/grit/format/android_xml... grit/format/android_xml_unittest.py:83: self.assertEqual(output.strip(), expected.strip(), msg=output.strip() + '\n' + expected.strip()) On 2015/08/04 18:15:11, newt wrote: > why this custom msg? assertEqual() already prints out the expected and actual > values? Done.
On 2015/08/04 22:56:14, conleyo wrote: > https://codereview.chromium.org/1258833004/diff/70001/grit/format/android_xml... > File grit/format/android_xml_unittest.py (right): > > https://codereview.chromium.org/1258833004/diff/70001/grit/format/android_xml... > grit/format/android_xml_unittest.py:83: self.assertEqual(output.strip(), > expected.strip(), msg=output.strip() + '\n' + expected.strip()) > On 2015/08/04 18:15:11, newt wrote: > > why this custom msg? assertEqual() already prints out the expected and actual > > values? > > Done. friendly ping
still lgtm. Do you need me to land this?
On 2015/08/07 19:17:38, newt wrote: > still lgtm. Do you need me to land this? Oh, sorry. This is my first change in chromium. I'll get some help and figure out what to do from here if that's all the approval I need.
The CQ bit was checked by conleyo@google.com
The CQ bit was unchecked by conleyo@google.com
The CQ bit was checked by conleyo@google.com
The CQ bit was unchecked by commit-bot@chromium.org
Commit queue rejected this change because it did not recognize the base URL. Please commit your change manually.
On 2015/08/07 19:17:38, newt wrote: > still lgtm. Do you need me to land this? OK, I might need some help here. What should the base url be and how can I change it?
Message was sent while issue was closed.
Committed patchset #6 (id:90001) manually as 192.
Message was sent while issue was closed.
Committed here: https://code.google.com/p/grit-i18n/source/detail?r=192
Message was sent while issue was closed.
On 2015/08/07 23:37:00, newt wrote: > Committed here: https://code.google.com/p/grit-i18n/source/detail?r=192 (Committing to grit works differently than committing to chromium. You have to commit the change via the command line, and you have to be whitelisted to make changes to the grit project.)
Message was sent while issue was closed.
On 2015/08/07 23:38:33, newt wrote: > On 2015/08/07 23:37:00, newt wrote: > > Committed here: https://code.google.com/p/grit-i18n/source/detail?r=192 > > (Committing to grit works differently than committing to chromium. You have to > commit the change via the command line, and you have to be whitelisted to make > changes to the grit project.) OK, well thanks for all your help! |
