Adds facilities for extracting Intl.message calls and generating code from translations

Adds facilities for extracting Intl.message calls and generating code from translations

This adds both general libraries for extracting messages and generating translations and scripts for using them with a trivial translation format. It removes the previous mechanism in message_lookup_local that relied on mirrors and traversing libraries in favor of one with a more explicit lookup by library that's consistent with the generated code. It also changes the code in basic_example to use that.

It also includes a couple of other changes. It now uses pathos for path manipulation, and the data_directory.dart hack for running tests from multiple different places was cleaned up and made more general. In addition, Intl.message now returns a string rather than a Future, which was changed as part of the libv2 integration.

This version does not yet support the plural syntax, as it disallows any interpolations that aren't just replacing a variable name. It also doesn't support named arguments to functions containing Intl.message, and it doesn't yet have a mechanism for designating sections of a text as not being subject to translation.

Finally, it relies on analyzer-experimental, which has a different name in the repository and in pub. So this version will run in the repository but will not run if used from pub. Bug 9071 asks for the names to be made consistent.

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

[Emily Fortuna, 2013-03-13 18:54:43]

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

https://codereview.chromium.org/12733003/diff/1/pkg/intl/lib/extract_messages...
pkg/intl/lib/extract_messages.dart:15: * break if there are changes to the API
that provides.
typo: "that it"

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

https://codereview.chromium.org/12733003/diff/1/pkg/intl/lib/generate_localiz...
pkg/intl/lib/generate_localized.dart:43: get message => translatedString;
please list the return type for getters/functions.

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

https://codereview.chromium.org/12733003/diff/1/pkg/intl/lib/intl.dart#newcode50
pkg/intl/lib/intl.dart:50: *      msg(num_people, place) => Intl.message(
So now we can take an arbitrary number of parameters for msg? I thought that was
the reason for taking the dictionary so that we could ensure msg always too the
same one argument, but could be passed multiple different items inside.

If you are changing this, we need to change the example on line 58.

https://codereview.chromium.org/12733003/diff/1/pkg/intl/lib/intl.dart#newcod...
pkg/intl/lib/intl.dart:131: static message(String message_str, {final String
desc: '',
should the return type here be a String?

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

https://codereview.chromium.org/12733003/diff/1/pkg/intl/lib/message_lookup_b...
pkg/intl/lib/message_lookup_by_library.dart:20: class CompositeMessageLookup {
some documentation explaining this class, please?

https://codereview.chromium.org/12733003/diff/1/pkg/intl/lib/message_lookup_b...
pkg/intl/lib/message_lookup_by_library.dart:29: // For this usage, if the locale
doesn't exist for messages, just return
random indentation?

https://codereview.chromium.org/12733003/diff/1/pkg/intl/lib/message_lookup_b...
pkg/intl/lib/message_lookup_by_library.dart:88: lookupMessage(String
message_str, [final String desc='',
list return type? String?

https://codereview.chromium.org/12733003/diff/1/pkg/intl/lib/message_lookup_b...
pkg/intl/lib/message_lookup_by_library.dart:94: if (_lookupInProgress) return
message_str;
can we combine this branch with line 91, so we have fewer spots we're returning
message_str?

https://codereview.chromium.org/12733003/diff/1/pkg/intl/lib/message_lookup_b...
pkg/intl/lib/message_lookup_by_library.dart:99: if (function == null) return
message_str;
suggestion: instead of eagerly returning here, run the next line only if
(function != null), and then both code paths will hit line 104 (so you have
fewer "return message_str" branches

https://codereview.chromium.org/12733003/diff/1/pkg/intl/lib/message_lookup_b...
pkg/intl/lib/message_lookup_by_library.dart:109: get messages;
don't these need implementation? Return types? documentation?

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

https://codereview.chromium.org/12733003/diff/1/pkg/intl/lib/message_lookup_l...
pkg/intl/lib/message_lookup_local.dart:121: return
deprecatedFutureValue(result).reflectee;
what's the plan for getting rid of this call? Need to add a TODO?

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

https://codereview.chromium.org/12733003/diff/1/pkg/intl/lib/src/intl_message...
pkg/intl/lib/src/intl_message.dart:13: *  Represents an occurence of
Intl.message in the program's source text. We
nit: you have an extra space starting this line of text

https://codereview.chromium.org/12733003/diff/1/pkg/intl/lib/src/intl_message...
pkg/intl/lib/src/intl_message.dart:22: * determine what to do with the
unfinished comment

https://codereview.chromium.org/12733003/diff/1/pkg/intl/lib/src/intl_message...
pkg/intl/lib/src/intl_message.dart:31: /** The name, which may come from the
function name, from the arguments
comment formatting. (also, end in period, here and elsewhere)

https://codereview.chromium.org/12733003/diff/1/pkg/intl/lib/src/intl_message...
pkg/intl/lib/src/intl_message.dart:140: toCode(String locale) {
return types, please

https://codereview.chromium.org/12733003/diff/1/pkg/intl/test/data_directory....
File pkg/intl/test/data_directory.dart (right):

https://codereview.chromium.org/12733003/diff/1/pkg/intl/test/data_directory....
pkg/intl/test/data_directory.dart:22: bool foundIntl;
shadowed?

https://codereview.chromium.org/12733003/diff/1/pkg/intl/test/data_directory....
pkg/intl/test/data_directory.dart:23: bool _checkForIntl() {
can this function be deleted?

https://codereview.chromium.org/12733003/diff/1/pkg/intl/test/data_directory....
pkg/intl/test/data_directory.dart:27: get intlDirectory {
Can you list the return types for these getters?

https://codereview.chromium.org/12733003/diff/1/pkg/intl/test/data_directory....
pkg/intl/test/data_directory.dart:36: bool checkForIntl(String each) {
rename suggestion "checkForIntlDir" just to more clearly explain what's
happening here.

https://codereview.chromium.org/12733003/diff/1/pkg/intl/test/intl_message_te...
File pkg/intl/test/intl_message_test.dart (right):

https://codereview.chromium.org/12733003/diff/1/pkg/intl/test/intl_message_te...
pkg/intl/test/intl_message_test.dart:32: return new Future.immediate(null);
do you need to return a future here if it's always an immediate with no value?
instead on line 25 what about initializeMessage('en_US'); 
expectAsync1(runTests);

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

https://codereview.chromium.org/12733003/diff/1/pkg/intl/test/message_extract...
pkg/intl/test/message_extraction/make_hardcoded_translation.dart:26:
r"L'interpolation est délicate quand elle se termine une phrase comme ${s}.",
80 char

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

https://codereview.chromium.org/12733003/diff/1/pkg/intl/test/message_extract...
pkg/intl/test/message_extraction/message_extraction_test.dart:44:
{"INTL_MESSAGE_OUTPUT" : dir()};
can this be a command line argument instead? (and hard coded as a "constant" for
the actual test)

https://codereview.chromium.org/12733003/diff/1/pkg/intl/test/message_extract...
pkg/intl/test/message_extraction/message_extraction_test.dart:85: "(curly braces
are ok) and xml reserved characters <& and quotes \" parameters"
indentation

https://codereview.chromium.org/12733003/diff/1/pkg/intl/test/message_extract...
pkg/intl/test/message_extraction/message_extraction_test.dart:97: // 
verify("The thing is, well");
why are these commented out?

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

https://codereview.chromium.org/12733003/diff/1/pkg/intl/test/message_extract...
pkg/intl/test/message_extraction/part_of_sample_with_messages.dart:16: return
Intl.message("This method is not a lambda", name: 'nonLambda');
What's trying to be tested here?

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

https://codereview.chromium.org/12733003/diff/1/pkg/intl/test/message_extract...
pkg/intl/test/message_extraction/sample_with_messages.dart:41:
Intl.message("Interpolation is tricky when it ends a sentence like ${s}.",
how is this different from message2?

https://codereview.chromium.org/12733003/diff/1/pkg/intl/test/message_extract...
pkg/intl/test/message_extraction/sample_with_messages.dart:46:
originalNotInBMP() => Intl.message("Ancient Greek hangman characters: 𐅆𐅇.",
bmp?

https://codereview.chromium.org/12733003/diff/1/pkg/intl/test/message_extract...
pkg/intl/test/message_extraction/sample_with_messages.dart:81:
print(message1());
why print here instead of using expect?

[Alan Knight, 2013-03-14 17:49:04]

PTAL

Also made changes to make the tests run more reliably when package-root is set
(e.g. from test.py), and to remove the dependency on scheduled_test.

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

https://codereview.chromium.org/12733003/diff/1/pkg/intl/lib/extract_messages...
pkg/intl/lib/extract_messages.dart:15: * break if there are changes to the API
that provides.
On 2013/03/13 18:54:43, Emily Fortuna wrote:
> typo: "that it"

Done.

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

https://codereview.chromium.org/12733003/diff/1/pkg/intl/lib/generate_localiz...
pkg/intl/lib/generate_localized.dart:43: get message => translatedString;
On 2013/03/13 18:54:43, Emily Fortuna wrote:
> please list the return type for getters/functions.

Done.

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

https://codereview.chromium.org/12733003/diff/1/pkg/intl/lib/intl.dart#newcode50
pkg/intl/lib/intl.dart:50: *      msg(num_people, place) => Intl.message(
On 2013/03/13 18:54:43, Emily Fortuna wrote:
> So now we can take an arbitrary number of parameters for msg? I thought that
was
> the reason for taking the dictionary so that we could ensure msg always too
the
> same one argument, but could be passed multiple different items inside.
> 
> If you are changing this, we need to change the example on line 58.

Yes, it can take an arbitrary number. I'm using Function.apply in the mechanism
using the "args" parameter. I think the original restriction was because we
didn't have that. The args and name parameters are currently mandatory, because
they're used in the runtime mechanism. That can change if we go beyond
generating additional code and start modifying the original, but right now that
doesn't happen. But they should also be in the examples.

https://codereview.chromium.org/12733003/diff/1/pkg/intl/lib/intl.dart#newcod...
pkg/intl/lib/intl.dart:131: static message(String message_str, {final String
desc: '',
On 2013/03/13 18:54:43, Emily Fortuna wrote:
> should the return type here be a String?

Oh yeah, right. Got so caught up in it having been turned into a Future I just
took it out and forgot to put String back.

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

https://codereview.chromium.org/12733003/diff/1/pkg/intl/lib/message_lookup_b...
pkg/intl/lib/message_lookup_by_library.dart:20: class CompositeMessageLookup {
On 2013/03/13 18:54:43, Emily Fortuna wrote:
> some documentation explaining this class, please?

Done.

https://codereview.chromium.org/12733003/diff/1/pkg/intl/lib/message_lookup_b...
pkg/intl/lib/message_lookup_by_library.dart:29: // For this usage, if the locale
doesn't exist for messages, just return
On 2013/03/13 18:54:43, Emily Fortuna wrote:
> random indentation?
Not random, I put each argument to the function on a separate line, and indented
them four spaces, which seemed to be one of the permitted things in the style
guide. And I put the start of the function call on a separate line because it
seemed clearer to have it indented less than its arguments. But I'm not fussy
about the formatting. How would you prefer it to look?

https://codereview.chromium.org/12733003/diff/1/pkg/intl/lib/message_lookup_b...
pkg/intl/lib/message_lookup_by_library.dart:88: lookupMessage(String
message_str, [final String desc='',
On 2013/03/13 18:54:43, Emily Fortuna wrote:
> list return type? String?

Done.

https://codereview.chromium.org/12733003/diff/1/pkg/intl/lib/message_lookup_b...
pkg/intl/lib/message_lookup_by_library.dart:94: if (_lookupInProgress) return
message_str;
On 2013/03/13 18:54:43, Emily Fortuna wrote:
> can we combine this branch with line 91, so we have fewer spots we're
returning
> message_str?

Done.

https://codereview.chromium.org/12733003/diff/1/pkg/intl/lib/message_lookup_b...
pkg/intl/lib/message_lookup_by_library.dart:99: if (function == null) return
message_str;
On 2013/03/13 18:54:43, Emily Fortuna wrote:
> suggestion: instead of eagerly returning here, run the next line only if
> (function != null), and then both code paths will hit line 104 (so you have
> fewer "return message_str" branches

Re-organized a bit more than even that so now there's only one "return
message_str" and result is initialized to message_str, so is used unless we
overwrite it it with the result of applying the message function.

https://codereview.chromium.org/12733003/diff/1/pkg/intl/lib/message_lookup_b...
pkg/intl/lib/message_lookup_by_library.dart:109: get messages;
On 2013/03/13 18:54:43, Emily Fortuna wrote:
> don't these need implementation? Return types? documentation?

They are to be implemented by the generated subclasses. Added return types and
documentation.

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

https://codereview.chromium.org/12733003/diff/1/pkg/intl/lib/message_lookup_l...
pkg/intl/lib/message_lookup_local.dart:121: return
deprecatedFutureValue(result).reflectee;
On 2013/03/13 18:54:43, Emily Fortuna wrote:
> what's the plan for getting rid of this call? Need to add a TODO?

Oops. This whole file should have been deleted. message_lookup_by_library is a
modified version of it, and looking up intl libraries via mirrors was a stopgap
that's removed in favor of the code generation/lazy loading approach.

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

https://codereview.chromium.org/12733003/diff/1/pkg/intl/lib/src/intl_message...
pkg/intl/lib/src/intl_message.dart:13: *  Represents an occurence of
Intl.message in the program's source text. We
On 2013/03/13 18:54:43, Emily Fortuna wrote:
> nit: you have an extra space starting this line of text
Done. It's 2013, where's my autoformatter?

https://codereview.chromium.org/12733003/diff/1/pkg/intl/lib/src/intl_message...
pkg/intl/lib/src/intl_message.dart:22: * determine what to do with the
On 2013/03/13 18:54:43, Emily Fortuna wrote:
> unfinished comment

Done.

https://codereview.chromium.org/12733003/diff/1/pkg/intl/lib/src/intl_message...
pkg/intl/lib/src/intl_message.dart:31: /** The name, which may come from the
function name, from the arguments
On 2013/03/13 18:54:43, Emily Fortuna wrote:
> comment formatting. (also, end in period, here and elsewhere)

Done.

https://codereview.chromium.org/12733003/diff/1/pkg/intl/lib/src/intl_message...
pkg/intl/lib/src/intl_message.dart:140: toCode(String locale) {
On 2013/03/13 18:54:43, Emily Fortuna wrote:
> return types, please

Done.

https://codereview.chromium.org/12733003/diff/1/pkg/intl/test/data_directory....
File pkg/intl/test/data_directory.dart (right):

https://codereview.chromium.org/12733003/diff/1/pkg/intl/test/data_directory....
pkg/intl/test/data_directory.dart:22: bool foundIntl;
On 2013/03/13 18:54:43, Emily Fortuna wrote:
> shadowed?
Deleted.

https://codereview.chromium.org/12733003/diff/1/pkg/intl/test/data_directory....
pkg/intl/test/data_directory.dart:23: bool _checkForIntl() {
On 2013/03/13 18:54:43, Emily Fortuna wrote:
> can this function be deleted?

Done.

https://codereview.chromium.org/12733003/diff/1/pkg/intl/test/data_directory....
pkg/intl/test/data_directory.dart:27: get intlDirectory {
On 2013/03/13 18:54:43, Emily Fortuna wrote:
> Can you list the return types for these getters?

Done.

https://codereview.chromium.org/12733003/diff/1/pkg/intl/test/data_directory....
pkg/intl/test/data_directory.dart:36: bool checkForIntl(String each) {
On 2013/03/13 18:54:43, Emily Fortuna wrote:
> rename suggestion "checkForIntlDir" just to more clearly explain what's
> happening here.

Done.

https://codereview.chromium.org/12733003/diff/1/pkg/intl/test/intl_message_te...
File pkg/intl/test/intl_message_test.dart (right):

https://codereview.chromium.org/12733003/diff/1/pkg/intl/test/intl_message_te...
pkg/intl/test/intl_message_test.dart:32: return new Future.immediate(null);
On 2013/03/13 18:54:43, Emily Fortuna wrote:
> do you need to return a future here if it's always an immediate with no value?
> instead on line 25 what about initializeMessage('en_US'); 
> expectAsync1(runTests);

Hmm. Actually, I think the thing to do is delete this entire test for the time
being. Once I really look at what it's doing it's only testing null
internationalization, which gets tested already in the round-trip test. And most
of the tests either duplicate tests in there, or test things that currently
aren't supported with actual translation, and may not be. Right now the
interpolations are limited to $<param> parameters, nothing that requires ${}.
One reason is that the translation formats may not be able to handle arbitrary
expressions. But we also don't want translators to be able to write things that
turn into non-trivial code. This is a problem for our current plural mechanism,
which is why the code generation doesn't support that yet.

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

https://codereview.chromium.org/12733003/diff/1/pkg/intl/test/message_extract...
pkg/intl/test/message_extraction/make_hardcoded_translation.dart:26:
r"L'interpolation est délicate quand elle se termine une phrase comme ${s}.",
On 2013/03/13 18:54:43, Emily Fortuna wrote:
> 80 char

Done.

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

https://codereview.chromium.org/12733003/diff/1/pkg/intl/test/message_extract...
pkg/intl/test/message_extraction/message_extraction_test.dart:44:
{"INTL_MESSAGE_OUTPUT" : dir()};
On 2013/03/13 18:54:43, Emily Fortuna wrote:
> can this be a command line argument instead? (and hard coded as a "constant"
for
> the actual test)

Done.

https://codereview.chromium.org/12733003/diff/1/pkg/intl/test/message_extract...
pkg/intl/test/message_extraction/message_extraction_test.dart:85: "(curly braces
are ok) and xml reserved characters <& and quotes \" parameters"
On 2013/03/13 18:54:43, Emily Fortuna wrote:
> indentation

Done.

https://codereview.chromium.org/12733003/diff/1/pkg/intl/test/message_extract...
pkg/intl/test/message_extraction/message_extraction_test.dart:97: // 
verify("The thing is, well");
On 2013/03/13 18:54:43, Emily Fortuna wrote:
> why are these commented out?
They test plurals and named arguments. I wrote the tests, then realized it was
going to be significant work to support them, but didn't want to delete the
tests and then have to write them over again for the follow-on CL, so I
commented them out.

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

https://codereview.chromium.org/12733003/diff/1/pkg/intl/test/message_extract...
pkg/intl/test/message_extraction/part_of_sample_with_messages.dart:16: return
Intl.message("This method is not a lambda", name: 'nonLambda');
On 2013/03/13 18:54:43, Emily Fortuna wrote:
> What's trying to be tested here? 

The parser recognizing and properly extracting a method that is not using lambda
(=>) syntax, and has additional code before the Intl.message call.

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

https://codereview.chromium.org/12733003/diff/1/pkg/intl/test/message_extract...
pkg/intl/test/message_extraction/sample_with_messages.dart:41:
Intl.message("Interpolation is tricky when it ends a sentence like ${s}.",
On 2013/03/13 18:54:43, Emily Fortuna wrote:
> how is this different from message2?

The interpolation is ${s} rather than just $x and is followed by a period. At
one point I was thinking that "$x.y" was ambiguous as an interpolation and we
needed to allow "${x}.y" but I don't think that's true, so the only interesting
thing is the curly braces around the expression, and that it's followed by
something else.

Added comments for the test sentences to say what they're exercising.

https://codereview.chromium.org/12733003/diff/1/pkg/intl/test/message_extract...
pkg/intl/test/message_extraction/sample_with_messages.dart:46:
originalNotInBMP() => Intl.message("Ancient Greek hangman characters: 𐅆𐅇.",
On 2013/03/13 18:54:43, Emily Fortuna wrote:
> bmp?

Basic Multilingual Plane. Characters that fit in UTF-16. So this sentence
contains characters that require multiple UTF-16 code units. Added a comment
saying what BMP is.

https://codereview.chromium.org/12733003/diff/1/pkg/intl/test/message_extract...
pkg/intl/test/message_extraction/sample_with_messages.dart:81:
print(message1());
On 2013/03/13 18:54:43, Emily Fortuna wrote:
> why print here instead of using expect?

I can't directly expect a constant here, because it's being run repeatedly with
multiple different locales. But I also wanted to keep the standalone program
being internationalized separate from the test code. So this isn't a test at
all, and is run from another program that is a test, so it prints and the other
program consumes its stdout.

Running this program as the test itself would be tricky, because this program
won't even compile until the code generation step has been done, because it
imports messages_all.dart, which doesn't exist. We could run it as the last
phase of the test, but then how do we synchronize its expects with the other
program that's doing the extraction and code generation?

[Emily Fortuna, 2013-03-14 19:45:00]

lgtm with the adjustments mentioned below!

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

https://codereview.chromium.org/12733003/diff/1/pkg/intl/lib/intl.dart#newcode50
pkg/intl/lib/intl.dart:50: *      msg(num_people, place) => Intl.message(
On 2013/03/14 17:49:05, Alan Knight wrote:
> On 2013/03/13 18:54:43, Emily Fortuna wrote:
> > So now we can take an arbitrary number of parameters for msg? I thought that
> was
> > the reason for taking the dictionary so that we could ensure msg always too
> the
> > same one argument, but could be passed multiple different items inside.
> > 
> > If you are changing this, we need to change the example on line 58.
> 
> Yes, it can take an arbitrary number. I'm using Function.apply in the
mechanism
> using the "args" parameter. I think the original restriction was because we
> didn't have that. The args and name parameters are currently mandatory,
because
> they're used in the runtime mechanism. That can change if we go beyond
> generating additional code and start modifying the original, but right now
that
> doesn't happen. But they should also be in the examples.
> 

sgtm.

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

https://codereview.chromium.org/12733003/diff/1/pkg/intl/lib/message_lookup_b...
pkg/intl/lib/message_lookup_by_library.dart:29: // For this usage, if the locale
doesn't exist for messages, just return
On 2013/03/14 17:49:05, Alan Knight wrote:
> On 2013/03/13 18:54:43, Emily Fortuna wrote:
> > random indentation?
> Not random, I put each argument to the function on a separate line, and
indented
> them four spaces, which seemed to be one of the permitted things in the style
> guide. And I put the start of the function call on a separate line because it
> seemed clearer to have it indented less than its arguments. But I'm not fussy
> about the formatting. How would you prefer it to look?

No, that's totally fine. I was complaining that lines 41-47 seem to all be
indented two extra spaces for no clear reason.

https://codereview.chromium.org/12733003/diff/1/pkg/intl/lib/message_lookup_b...
pkg/intl/lib/message_lookup_by_library.dart:109: get messages;
On 2013/03/14 17:49:05, Alan Knight wrote:
> On 2013/03/13 18:54:43, Emily Fortuna wrote:
> > don't these need implementation? Return types? documentation?
> 
> They are to be implemented by the generated subclasses. Added return types and
> documentation.

sgtm.

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

https://codereview.chromium.org/12733003/diff/1/pkg/intl/test/message_extract...
pkg/intl/test/message_extraction/part_of_sample_with_messages.dart:16: return
Intl.message("This method is not a lambda", name: 'nonLambda');
On 2013/03/14 17:49:05, Alan Knight wrote:
> On 2013/03/13 18:54:43, Emily Fortuna wrote:
> > What's trying to be tested here? 
> 
> The parser recognizing and properly extracting a method that is not using
lambda
> (=>) syntax, and has additional code before the Intl.message call.

gotcha!

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

https://codereview.chromium.org/12733003/diff/12001/pkg/intl/lib/intl.dart#ne...
pkg/intl/lib/intl.dart:62: * Calling `msg(2, 'Athens'});` would
nit: remove }

https://codereview.chromium.org/12733003/diff/12001/pkg/intl/test/message_ext...
File pkg/intl/test/message_extraction/message_extraction_test.dart (right):

https://codereview.chromium.org/12733003/diff/12001/pkg/intl/test/message_ext...
pkg/intl/test/message_extraction/message_extraction_test.dart:51:
.then(expectAsync1((result) => generateTranslationFiles(result)
a handy new feature for our unittesting library went in dealing with futures.
The test will pass if the future completes successfully. So you can rewrite this
nested thing like this:

test("Test round trip message generation, printing, and ", () {
  deleteGeneratedFiles();
  return extractMessages(null);
}.then((result) {
  return generateTranslationFiles(result);
}.then((result) {
  return generateCodeFromTranslation(result);
});

etc, and then you don't have to do the onError, because the unittest future
handling will propagate the errors.

[Alan Knight, 2013-03-14 20:49:42]

Committed patchset #2 manually as r20047 (presubmit successful).

[Alan Knight, 2013-03-14 20:50:04]

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

https://codereview.chromium.org/12733003/diff/1/pkg/intl/lib/message_lookup_b...
pkg/intl/lib/message_lookup_by_library.dart:29: // For this usage, if the locale
doesn't exist for messages, just return
On 2013/03/14 19:45:00, Emily Fortuna wrote:
> On 2013/03/14 17:49:05, Alan Knight wrote:
> > On 2013/03/13 18:54:43, Emily Fortuna wrote:
> > > random indentation?
> > Not random, I put each argument to the function on a separate line, and
> indented
> > them four spaces, which seemed to be one of the permitted things in the
style
> > guide. And I put the start of the function call on a separate line because
it
> > seemed clearer to have it indented less than its arguments. But I'm not
fussy
> > about the formatting. How would you prefer it to look?
> 
> No, that's totally fine. I was complaining that lines 41-47 seem to all be
> indented two extra spaces for no clear reason.

Doh! Done.

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

https://codereview.chromium.org/12733003/diff/12001/pkg/intl/lib/intl.dart#ne...
pkg/intl/lib/intl.dart:62: * Calling `msg(2, 'Athens'});` would
On 2013/03/14 19:45:00, Emily Fortuna wrote:
> nit: remove }

Done.

https://codereview.chromium.org/12733003/diff/12001/pkg/intl/test/message_ext...
File pkg/intl/test/message_extraction/message_extraction_test.dart (right):

https://codereview.chromium.org/12733003/diff/12001/pkg/intl/test/message_ext...
pkg/intl/test/message_extraction/message_extraction_test.dart:51:
.then(expectAsync1((result) => generateTranslationFiles(result)
On 2013/03/14 19:45:00, Emily Fortuna wrote:
> a handy new feature for our unittesting library went in dealing with futures.
> The test will pass if the future completes successfully. So you can rewrite
this
> nested thing like this:
> 
> test("Test round trip message generation, printing, and ", () {
>   deleteGeneratedFiles();
>   return extractMessages(null);
> }.then((result) {
>   return generateTranslationFiles(result);
> }.then((result) {
>   return generateCodeFromTranslation(result);
> });
> 
> etc, and then you don't have to do the onError, because the unittest future
> handling will propagate the errors.

Nice.