Plurals and Genders

Plurals and Genders

R=efortuna@google.com

Committed: https://code.google.com/p/dart/source/detail?r=24746

[Alan Knight, 2013-07-02 21:02:36]

A mother of all code reviews...

Major points
- Make Intl.plural and Intl.gender do something
- Introduced new analyzer visitors to extract the information from plurals and
genders
- Added a Message hierarchy in intl_message.dart to model all the permitted
message pieces, don't use strings or integers to represents literal pieces and
interpolations any more.

Minor things
- If a translation is missing, it now returns the original rather than throwing
an exception.
- The error information for the visitors is now global to that library (since
it's the same for all visitors
- It now uses serialization in the tests to encode the complicated structures.
Coming soon, stop using a custom JSON format and use something real for the open
source translation file format.

[Emily Fortuna, 2013-07-03 17:52:32]

https://codereview.chromium.org/18543009/diff/1/pkg/intl/lib/extract_messages...
File pkg/intl/lib/extract_messages.dart (right):

https://codereview.chromium.org/18543009/diff/1/pkg/intl/lib/extract_messages...
pkg/intl/lib/extract_messages.dart:39: Map<String, MainMessage> parseFile(File
file) {
why change the name to MainMessage?

https://codereview.chromium.org/18543009/diff/1/pkg/intl/lib/extract_messages...
pkg/intl/lib/extract_messages.dart:47: 
remove line here

https://codereview.chromium.org/18543009/diff/1/pkg/intl/lib/extract_messages...
pkg/intl/lib/extract_messages.dart:316: bool foundSomething = false;
perhaps change name to foundPluralOrGender so it's a little more explicit what
you found? Unless you expect this to expand a lot to also signifying found a
number of other things.

https://codereview.chromium.org/18543009/diff/1/pkg/intl/lib/extract_messages...
pkg/intl/lib/extract_messages.dart:320: if
(!looksLikePluralOrGender(node.expression)) return;
+1 space for indentation

https://codereview.chromium.org/18543009/diff/1/pkg/intl/lib/extract_messages...
pkg/intl/lib/extract_messages.dart:343: // TODO(alanknight): Add reasonable
validity checks.
nit: indent two spaces :-P

https://codereview.chromium.org/18543009/diff/1/pkg/intl/lib/generate_localiz...
File pkg/intl/lib/generate_localized.dart (right):

https://codereview.chromium.org/18543009/diff/1/pkg/intl/lib/generate_localiz...
pkg/intl/lib/generate_localized.dart:65: * The identifier for this message. In
the simplest case, this is the name,
sorry, name of what?

https://codereview.chromium.org/18543009/diff/1/pkg/intl/lib/generate_localiz...
pkg/intl/lib/generate_localized.dart:79: get message => translated;
I think we prefer to still have return types, especially for APIs that others
will use.

https://codereview.chromium.org/18543009/diff/1/pkg/intl/lib/intl.dart
File pkg/intl/lib/intl.dart (right):

https://codereview.chromium.org/18543009/diff/1/pkg/intl/lib/intl.dart#newcod...
pkg/intl/lib/intl.dart:242: * as part of an Intl.message message that is to be
translated.
`Intl.message` 
also you probably can do without the second "message" in that sentence....

https://codereview.chromium.org/18543009/diff/1/pkg/intl/lib/intl.dart#newcod...
pkg/intl/lib/intl.dart:248: throw new ArgumentError("The 'other' named argument
must be provided");
in which case should we make it a positional required argument?

https://codereview.chromium.org/18543009/diff/1/pkg/intl/lib/intl.dart#newcod...
pkg/intl/lib/intl.dart:250: // TODO(alanknight): This algorithm needs to be
locale-dependent.
how will we be able to change this behavior based on locale?

https://codereview.chromium.org/18543009/diff/1/pkg/intl/lib/src/intl_message...
File pkg/intl/lib/src/intl_message.dart (right):

https://codereview.chromium.org/18543009/diff/1/pkg/intl/lib/src/intl_message...
pkg/intl/lib/src/intl_message.dart:6: * This provides the class IntlMessage to
represent an occurence of
IntlMessage -> MainMessage?

https://codereview.chromium.org/18543009/diff/1/pkg/intl/lib/src/intl_message...
pkg/intl/lib/src/intl_message.dart:120: * main Intl.message call, plurals, and
genders.
can you give an example in the documentation of each of these types of messages?
It would help illustrate how they work together.

https://codereview.chromium.org/18543009/diff/1/pkg/intl/lib/src/intl_message...
pkg/intl/lib/src/intl_message.dart:169: pieces.map((chunk) => f(this,
chunk)).join("");
+2 spaces

https://codereview.chromium.org/18543009/diff/1/pkg/intl/lib/src/intl_message...
pkg/intl/lib/src/intl_message.dart:268: messagePieces.map((chunk) => f(this,
chunk)).join("");
+2 spaces

https://codereview.chromium.org/18543009/diff/1/pkg/intl/test/message_extract...
File pkg/intl/test/message_extraction/make_hardcoded_translation.dart (right):

https://codereview.chromium.org/18543009/diff/1/pkg/intl/test/message_extract...
pkg/intl/test/message_extraction/make_hardcoded_translation.dart:21:
..addRuleFor(new VariableSubstitution(0, null),
can we have more descriptive names than s and w here and on line 28?

also a comment explaining this added code

[Alan Knight, 2013-07-03 18:41:07]

PTAL

https://codereview.chromium.org/18543009/diff/1/pkg/intl/lib/extract_messages...
File pkg/intl/lib/extract_messages.dart (right):

https://codereview.chromium.org/18543009/diff/1/pkg/intl/lib/extract_messages...
pkg/intl/lib/extract_messages.dart:39: Map<String, MainMessage> parseFile(File
file) {
On 2013/07/03 17:52:33, Emily Fortuna wrote:
> why change the name to MainMessage? 

I was getting too many classes that were all Intl something, so I thought the
Intl was all implied by context and removed it everywhere, and I used Message as
the name for the abstract class at the top of the hierarchy, so wanted to
differentiate the top-level one created by the Intl.message call.

https://codereview.chromium.org/18543009/diff/1/pkg/intl/lib/extract_messages...
pkg/intl/lib/extract_messages.dart:47: 
On 2013/07/03 17:52:33, Emily Fortuna wrote:
> remove line here

Done.

https://codereview.chromium.org/18543009/diff/1/pkg/intl/lib/extract_messages...
pkg/intl/lib/extract_messages.dart:316: bool foundSomething = false;
On 2013/07/03 17:52:33, Emily Fortuna wrote:
> perhaps change name to foundPluralOrGender so it's a little more explicit what
> you found? Unless you expect this to expand a lot to also signifying found a
> number of other things.

Done.

https://codereview.chromium.org/18543009/diff/1/pkg/intl/lib/extract_messages...
pkg/intl/lib/extract_messages.dart:320: if
(!looksLikePluralOrGender(node.expression)) return;
On 2013/07/03 17:52:33, Emily Fortuna wrote:
> +1 space for indentation

Done.

https://codereview.chromium.org/18543009/diff/1/pkg/intl/lib/extract_messages...
pkg/intl/lib/extract_messages.dart:343: // TODO(alanknight): Add reasonable
validity checks.
On 2013/07/03 17:52:33, Emily Fortuna wrote:
> nit: indent two spaces :-P

Done.

https://codereview.chromium.org/18543009/diff/1/pkg/intl/lib/generate_localiz...
File pkg/intl/lib/generate_localized.dart (right):

https://codereview.chromium.org/18543009/diff/1/pkg/intl/lib/generate_localiz...
pkg/intl/lib/generate_localized.dart:65: * The identifier for this message. In
the simplest case, this is the name,
On 2013/07/03 17:52:33, Emily Fortuna wrote:
> sorry, name of what?

Clarified.

https://codereview.chromium.org/18543009/diff/1/pkg/intl/lib/generate_localiz...
pkg/intl/lib/generate_localized.dart:79: get message => translated;
On 2013/07/03 17:52:33, Emily Fortuna wrote:
> I think we prefer to still have return types, especially for APIs that others
> will use.

There should be very few others using these APIs, but done.

https://codereview.chromium.org/18543009/diff/1/pkg/intl/lib/intl.dart
File pkg/intl/lib/intl.dart (right):

https://codereview.chromium.org/18543009/diff/1/pkg/intl/lib/intl.dart#newcod...
pkg/intl/lib/intl.dart:242: * as part of an Intl.message message that is to be
translated.
On 2013/07/03 17:52:33, Emily Fortuna wrote:
> `Intl.message` 
> also you probably can do without the second "message" in that sentence....

Done. Changed second "message" to "text"

https://codereview.chromium.org/18543009/diff/1/pkg/intl/lib/intl.dart#newcod...
pkg/intl/lib/intl.dart:248: throw new ArgumentError("The 'other' named argument
must be provided");
On 2013/07/03 17:52:33, Emily Fortuna wrote:
> in which case should we make it a positional required argument?

That would mean you'd have to write it first, which seems awkward. Also, since
these names correspond to the known ICU names, it seems nice to have them all be
there and named consistently to match. In different locales, different other
forms will also be mandatory, so we can't use this as a way to be sure all the
required forms are provided.

https://codereview.chromium.org/18543009/diff/1/pkg/intl/lib/intl.dart#newcod...
pkg/intl/lib/intl.dart:250: // TODO(alanknight): This algorithm needs to be
locale-dependent.
On 2013/07/03 17:52:33, Emily Fortuna wrote:
> how will we be able to change this behavior based on locale?

I was thinking that there would be a generated implementation included with the
generated message libraries for each locale, and that this would either delegate
to that, or possibly that the translated messages would directly call the
version specific to their locale. e.g. the translation might be 
   Intl.message("${localPlural(...)}");
rather than
   Intl.message("${Intl.plural(...)}");
where localPlural is defined in each generated library.

https://codereview.chromium.org/18543009/diff/1/pkg/intl/lib/src/intl_message...
File pkg/intl/lib/src/intl_message.dart (right):

https://codereview.chromium.org/18543009/diff/1/pkg/intl/lib/src/intl_message...
pkg/intl/lib/src/intl_message.dart:6: * This provides the class IntlMessage to
represent an occurence of
On 2013/07/03 17:52:33, Emily Fortuna wrote:
> IntlMessage -> MainMessage?

Yes. Rephrased the whole thing, as this now provides lots of classes.

https://codereview.chromium.org/18543009/diff/1/pkg/intl/lib/src/intl_message...
pkg/intl/lib/src/intl_message.dart:120: * main Intl.message call, plurals, and
genders.
On 2013/07/03 17:52:33, Emily Fortuna wrote:
> can you give an example in the documentation of each of these types of
messages?
> It would help illustrate how they work together.

Done.

https://codereview.chromium.org/18543009/diff/1/pkg/intl/lib/src/intl_message...
pkg/intl/lib/src/intl_message.dart:169: pieces.map((chunk) => f(this,
chunk)).join("");
On 2013/07/03 17:52:33, Emily Fortuna wrote:
> +2 spaces

Done.

https://codereview.chromium.org/18543009/diff/1/pkg/intl/lib/src/intl_message...
pkg/intl/lib/src/intl_message.dart:268: messagePieces.map((chunk) => f(this,
chunk)).join("");
On 2013/07/03 17:52:33, Emily Fortuna wrote:
> +2 spaces

Done.

https://codereview.chromium.org/18543009/diff/1/pkg/intl/test/message_extract...
File pkg/intl/test/message_extraction/make_hardcoded_translation.dart (right):

https://codereview.chromium.org/18543009/diff/1/pkg/intl/test/message_extract...
pkg/intl/test/message_extraction/make_hardcoded_translation.dart:21:
..addRuleFor(new VariableSubstitution(0, null),
On 2013/07/03 17:52:33, Emily Fortuna wrote:
> can we have more descriptive names than s and w here and on line 28?
> 
> also a comment explaining this added code

Done.

[Emily Fortuna, 2013-07-03 19:47:40]

lgtm, modulo one typo!

https://codereview.chromium.org/18543009/diff/9001/pkg/intl/lib/generate_loca...
File pkg/intl/lib/generate_localized.dart (right):

https://codereview.chromium.org/18543009/diff/9001/pkg/intl/lib/generate_loca...
pkg/intl/lib/generate_localized.dart:67: * but it can be any identifier that
thiss program and the output of the
typo thiss -> this

[Alan Knight, 2013-07-03 19:58:54]

https://codereview.chromium.org/18543009/diff/9001/pkg/intl/lib/generate_loca...
File pkg/intl/lib/generate_localized.dart (right):

https://codereview.chromium.org/18543009/diff/9001/pkg/intl/lib/generate_loca...
pkg/intl/lib/generate_localized.dart:67: * but it can be any identifier that
thiss program and the output of the
On 2013/07/03 19:47:40, Emily Fortuna wrote:
> typo thiss -> this

Done.

[Alan Knight, 2013-07-03 20:23:57]

Committed patchset #3 manually as r24746 (presubmit successful).