Add async initialization for Intl.message and an example

Add async initialization for Intl.message and an example
Committed: https://code.google.com/p/dart/source/detail?r=12345

[Alan Knight, 2012-09-11 22:50:06]

This adds async initialization for internationalizable messages, and implements
the local strategy in which they are all available. The mechanism is to have a
library of the form
   <prefix><localeName>
which defines functions of the same name as the messages to be
internationalized. These need to be imported somewhere in the program
(presumably with a prefix or the names would conflict) and then these are
accessed using reflection. Pending the dart2dart transformation the user must
also pass the function name and arguments to the Intl.message function.

Also made the pattern optional on Intl.date because the most convenient usage
seemed to be 
   intl.date().add_Hms;
It might be worthwhile to add hms() etc. methods on Intl, or to think again
about the add_prefix and maybe use a prefix on the constructors instead.

The class IntlMessage was completely unused, and since Intl.message was
returning a string it didn't seem like it would get used, so I deleted it and
moved the functionality that was used into message_lookup_local.

Stopped calling verifyLocale when you create an Intl instance, because it was
relying on the DateFormat set of locales, which isn't necessarily the same as
the set for messages. Right now it will be called on DateFormat instance
creation. There's a TODO to do verification and check alternate forms of the
locale for message locales.

Since this all relies on mirrors, don't test on dart2js.

It also provides a small example application to demonstrate usage and validate
that the APIs are actually practical.

[Emily Fortuna, 2012-09-12 01:23:48]

https://chromiumcodereview.appspot.com/10915215/diff/8001/pkg/intl/example/ba...
File pkg/intl/example/basic/basic_example.dart (right):

https://chromiumcodereview.appspot.com/10915215/diff/8001/pkg/intl/example/ba...
pkg/intl/example/basic/basic_example.dart:20: // a separate package.
why not use package:intl references here too to set a complete example?

https://chromiumcodereview.appspot.com/10915215/diff/8001/pkg/intl/example/ba...
pkg/intl/example/basic/basic_example.dart:47: runProgram(_) {
I don't quite understand why you're passed a collection of futures. In
intl_message_test_using_example.dart it looks like you're just passing in the
print function(?) Maybe I'm mis-reading this.

If you're going to pass a not used argument, I'd still give the _ a type, and
maybe call it "notUsed" (_ is also an okay name, though)

https://chromiumcodereview.appspot.com/10915215/diff/8001/pkg/intl/example/ba...
File pkg/intl/example/basic/basic_example_shell.dart (right):

https://chromiumcodereview.appspot.com/10915215/diff/8001/pkg/intl/example/ba...
pkg/intl/example/basic/basic_example_shell.dart:6: * This is just a shell that
runs the code in basic_example.dart, leaving
I'd rename this file to something like basic_example_runner.dart or
basic_example_main.dart. I first opened the basic_example.dart file and was
confused how it got called, and I think making this file be the more
straightforward "look at me first" name will fix that.

https://chromiumcodereview.appspot.com/10915215/diff/8001/pkg/intl/intl.dart
File pkg/intl/intl.dart (right):

https://chromiumcodereview.appspot.com/10915215/diff/8001/pkg/intl/intl.dart#...
pkg/intl/intl.dart:14: #import('lib/intl_helpers.dart');
I don't see any "intl_helpers.dart" did this not get added to the repo?

https://chromiumcodereview.appspot.com/10915215/diff/8001/pkg/intl/intl.dart#...
pkg/intl/intl.dart:46: _locale = aLocale;
why not set a default here? then you don't have to do those extra checks later
in this class.

https://chromiumcodereview.appspot.com/10915215/diff/8001/pkg/intl/intl.dart#...
pkg/intl/intl.dart:69: final Map examples=const {}, String locale, String name,
indent at least two more spaces here

https://chromiumcodereview.appspot.com/10915215/diff/8001/pkg/intl/intl.dart#...
pkg/intl/intl.dart:71: return _messageLookup.
do the line break at the opening of the ( please instead of the .

https://chromiumcodereview.appspot.com/10915215/diff/8001/pkg/intl/intl.dart#...
pkg/intl/intl.dart:71: return _messageLookup.
indent two less spaces here

https://chromiumcodereview.appspot.com/10915215/diff/8001/pkg/intl/intl.dart#...
pkg/intl/intl.dart:198: /** The internal mechanism for looking up messages. We
expect this to be set
/**
 * The internal....
 */

https://chromiumcodereview.appspot.com/10915215/diff/8001/pkg/intl/intl.dart#...
pkg/intl/intl.dart:205: /** Initialize the message lookup mechanism. This is for
internal use only.
same formatting here

https://chromiumcodereview.appspot.com/10915215/diff/8001/pkg/intl/message_lo...
File pkg/intl/message_lookup_local.dart (right):

https://chromiumcodereview.appspot.com/10915215/diff/8001/pkg/intl/message_lo...
pkg/intl/message_lookup_local.dart:6: * Message/plural format library with
locale support. This can have different
can we have a skeleton at least for the nonlocal version of this class? or is
that coming in a later CL?

https://chromiumcodereview.appspot.com/10915215/diff/8001/pkg/intl/message_lo...
pkg/intl/message_lookup_local.dart:43: #import('intl.dart');
documentation nit: this was sourced by intl.dart so that this class shows up as
something connected with (and subordinate to) the Intl object. Are we no longer
doing that?

https://chromiumcodereview.appspot.com/10915215/diff/8001/pkg/intl/message_lo...
pkg/intl/message_lookup_local.dart:71: String source;
call it sourcePrefix then.

https://chromiumcodereview.appspot.com/10915215/diff/8001/pkg/intl/message_lo...
pkg/intl/message_lookup_local.dart:90: * substituted into the message. For
example, if message="Hello, $name", then
make your "for example" section a new paragraph, so we don't have such a wall of
text.

https://chromiumcodereview.appspot.com/10915215/diff/8001/pkg/intl/test/intl_...
File pkg/intl/test/intl_test.dart (right):

https://chromiumcodereview.appspot.com/10915215/diff/8001/pkg/intl/test/intl_...
pkg/intl/test/intl_test.dart:10: #import('../message_lookup_local.dart');
seems odd that we have to import message_lookup_local and run initializeMessage
for a test that doesn't do any message formatting.

https://chromiumcodereview.appspot.com/10915215/diff/8001/pkg/pkg.status
File pkg/pkg.status (right):

https://chromiumcodereview.appspot.com/10915215/diff/8001/pkg/pkg.status#newc...
pkg/pkg.status:25: intl/test/intl_message_test: skip
is there a bug open on dart2js about this? If so, put the issue number in
comment, so when it gets fixed, these will be changed.

[Alan Knight, 2012-09-12 20:58:20]

PTAL

https://chromiumcodereview.appspot.com/10915215/diff/8001/pkg/intl/example/ba...
File pkg/intl/example/basic/basic_example.dart (right):

https://chromiumcodereview.appspot.com/10915215/diff/8001/pkg/intl/example/ba...
pkg/intl/example/basic/basic_example.dart:20: // a separate package.
On 2012/09/12 01:23:48, Emily Fortuna wrote:
> why not use package:intl references here too to set a complete example?

Sadly, I had them, but took them out because I set it up to be used in a test,
rather than duplicating the logic to write a test, and the buildbots aren't
going to understand pub until post-M1.

https://chromiumcodereview.appspot.com/10915215/diff/8001/pkg/intl/example/ba...
pkg/intl/example/basic/basic_example.dart:47: runProgram(_) {
On 2012/09/12 01:23:48, Emily Fortuna wrote:
> I don't quite understand why you're passed a collection of futures. In
> intl_message_test_using_example.dart it looks like you're just passing in the
> print function(?) Maybe I'm mis-reading this.
> 
> If you're going to pass a not used argument, I'd still give the _ a type, and
> maybe call it "notUsed" (_ is also an okay name, though)

runProgram is called in the "then" call from Futures.wait on locale
initialization, so it gets the result, which is a collection of the Futures that
were waited on. 

The call from the test was wrong, and didn't get caught because the name didn't
end in _test, so it wasn't run. Fixed.

I liked the _ convention without a type because it made the thing visually
minimal, making it easy to ignore. But I put in a type.

https://chromiumcodereview.appspot.com/10915215/diff/8001/pkg/intl/example/ba...
File pkg/intl/example/basic/basic_example_shell.dart (right):

https://chromiumcodereview.appspot.com/10915215/diff/8001/pkg/intl/example/ba...
pkg/intl/example/basic/basic_example_shell.dart:6: * This is just a shell that
runs the code in basic_example.dart, leaving
On 2012/09/12 01:23:48, Emily Fortuna wrote:
> I'd rename this file to something like basic_example_runner.dart or
> basic_example_main.dart. I first opened the basic_example.dart file and was
> confused how it got called, and I think making this file be the more
> straightforward "look at me first" name will fix that.

Done.

https://chromiumcodereview.appspot.com/10915215/diff/8001/pkg/intl/intl.dart
File pkg/intl/intl.dart (right):

https://chromiumcodereview.appspot.com/10915215/diff/8001/pkg/intl/intl.dart#...
pkg/intl/intl.dart:14: #import('lib/intl_helpers.dart');
On 2012/09/12 01:23:48, Emily Fortuna wrote:
> I don't see any "intl_helpers.dart" did this not get added to the repo?

It's in the /lib directory, but should be there.
http://code.google.com/p/dart/source/browse/branches/bleeding_edge/dart/pkg/i...

https://chromiumcodereview.appspot.com/10915215/diff/8001/pkg/intl/intl.dart#...
pkg/intl/intl.dart:46: _locale = aLocale;
On 2012/09/12 01:23:48, Emily Fortuna wrote:
> why not set a default here? then you don't have to do those extra checks later
> in this class.

Done.

https://chromiumcodereview.appspot.com/10915215/diff/8001/pkg/intl/intl.dart#...
pkg/intl/intl.dart:69: final Map examples=const {}, String locale, String name,
On 2012/09/12 01:23:48, Emily Fortuna wrote:
> indent at least two more spaces here

Done.

https://chromiumcodereview.appspot.com/10915215/diff/8001/pkg/intl/intl.dart#...
pkg/intl/intl.dart:71: return _messageLookup.
On 2012/09/12 01:23:48, Emily Fortuna wrote:
> do the line break at the opening of the ( please instead of the .

Done.

https://chromiumcodereview.appspot.com/10915215/diff/8001/pkg/intl/intl.dart#...
pkg/intl/intl.dart:71: return _messageLookup.
On 2012/09/12 01:23:48, Emily Fortuna wrote:
> do the line break at the opening of the ( please instead of the .

Done.

https://chromiumcodereview.appspot.com/10915215/diff/8001/pkg/intl/intl.dart#...
pkg/intl/intl.dart:198: /** The internal mechanism for looking up messages. We
expect this to be set
On 2012/09/12 01:23:48, Emily Fortuna wrote:
> /**
>  * The internal....
>  */

Done.

https://chromiumcodereview.appspot.com/10915215/diff/8001/pkg/intl/intl.dart#...
pkg/intl/intl.dart:205: /** Initialize the message lookup mechanism. This is for
internal use only.
On 2012/09/12 01:23:48, Emily Fortuna wrote:
> same formatting here

Done.

https://chromiumcodereview.appspot.com/10915215/diff/8001/pkg/intl/message_lo...
File pkg/intl/message_lookup_local.dart (right):

https://chromiumcodereview.appspot.com/10915215/diff/8001/pkg/intl/message_lo...
pkg/intl/message_lookup_local.dart:6: * Message/plural format library with
locale support. This can have different
On 2012/09/12 01:23:48, Emily Fortuna wrote:
> can we have a skeleton at least for the nonlocal version of this class? or is
> that coming in a later CL?

I'm thinking that the skeleton should be the same as this. That is, similar to
the date formatting, you import the appropriate library, call
initializeMessages(localeName, source)
where the interpretation of the source string is appropriate to the mechanism,
and at the user level that's all that happens. Internally, Intl.message calls
the lookupMessage method in the same way.

There's also a stub of  initializeIndividualLocaleMessageFormatting, which does
nothing for this implementation, but would for a non-local interpretation.
That's also not user-visible.

Any non-local version requires dynamic code loading, which has some open
questions, but shouldn't affect the API.

Made the non-essential parts of this private to make the skeleton of an
alternate implementation clearer.

https://chromiumcodereview.appspot.com/10915215/diff/8001/pkg/intl/message_lo...
pkg/intl/message_lookup_local.dart:43: #import('intl.dart');
On 2012/09/12 01:23:48, Emily Fortuna wrote:
> documentation nit: this was sourced by intl.dart so that this class shows up
as
> something connected with (and subordinate to) the Intl object. Are we no
longer
> doing that?

We can't source it if we have multiple incompatible implementations, and there
isn't anything left here that isn't part of that implementation. We never really
used the IntlMessage object, so the API is essentially just the Intl.message
method and the implementation-dependent initialization.

There's a broader issue about sourcing things versus importing and how they show
up in the api docs, that I've talked to Seth about some. But the dominant
consideration seems to be that with multiple implementations we can't use
source.

https://chromiumcodereview.appspot.com/10915215/diff/8001/pkg/intl/message_lo...
pkg/intl/message_lookup_local.dart:71: String source;
On 2012/09/12 01:23:48, Emily Fortuna wrote:
> call it sourcePrefix then.

Done.

https://chromiumcodereview.appspot.com/10915215/diff/8001/pkg/intl/message_lo...
pkg/intl/message_lookup_local.dart:90: * substituted into the message. For
example, if message="Hello, $name", then
On 2012/09/12 01:23:48, Emily Fortuna wrote:
> make your "for example" section a new paragraph, so we don't have such a wall
of
> text.

Done.

https://chromiumcodereview.appspot.com/10915215/diff/8001/pkg/intl/test/intl_...
File pkg/intl/test/intl_test.dart (right):

https://chromiumcodereview.appspot.com/10915215/diff/8001/pkg/intl/test/intl_...
pkg/intl/test/intl_test.dart:10: #import('../message_lookup_local.dart');
On 2012/09/12 01:23:48, Emily Fortuna wrote:
> seems odd that we have to import message_lookup_local and run
initializeMessage
> for a test that doesn't do any message formatting.

Good point. At one point I'd put code in there that did require it, but then
removed it. Deleted.

https://chromiumcodereview.appspot.com/10915215/diff/8001/pkg/pkg.status
File pkg/pkg.status (right):

https://chromiumcodereview.appspot.com/10915215/diff/8001/pkg/pkg.status#newc...
pkg/pkg.status:25: intl/test/intl_message_test: skip
On 2012/09/12 01:23:48, Emily Fortuna wrote:
> is there a bug open on dart2js about this? If so, put the issue number in
> comment, so when it gets fixed, these will be changed. 
I can't find one.

[Emily Fortuna, 2012-09-12 21:55:59]

lgtm, with a few additional edits.

https://chromiumcodereview.appspot.com/10915215/diff/8001/pkg/intl/example/ba...
File pkg/intl/example/basic/basic_example.dart (right):

https://chromiumcodereview.appspot.com/10915215/diff/8001/pkg/intl/example/ba...
pkg/intl/example/basic/basic_example.dart:20: // a separate package.
On 2012/09/12 20:58:20, Alan Knight wrote:
> On 2012/09/12 01:23:48, Emily Fortuna wrote:
> > why not use package:intl references here too to set a complete example?
> 
> Sadly, I had them, but took them out because I set it up to be used in a test,
> rather than duplicating the logic to write a test, and the buildbots aren't
> going to understand pub until post-M1.

Ah. Fair enough. Can we add a TODO?

https://chromiumcodereview.appspot.com/10915215/diff/8001/pkg/intl/example/ba...
pkg/intl/example/basic/basic_example.dart:47: runProgram(_) {
On 2012/09/12 20:58:20, Alan Knight wrote:
> On 2012/09/12 01:23:48, Emily Fortuna wrote:
> > I don't quite understand why you're passed a collection of futures. In
> > intl_message_test_using_example.dart it looks like you're just passing in
the
> > print function(?) Maybe I'm mis-reading this.
> > 
> > If you're going to pass a not used argument, I'd still give the _ a type,
and
> > maybe call it "notUsed" (_ is also an okay name, though)
> 
> runProgram is called in the "then" call from Futures.wait on locale
> initialization, so it gets the result, which is a collection of the Futures
that
> were waited on. 
> 
> The call from the test was wrong, and didn't get caught because the name
didn't
> end in _test, so it wasn't run. Fixed.
> 
> I liked the _ convention without a type because it made the thing visually
> minimal, making it easy to ignore. But I put in a type.

Fair enough. I'm convinced. Feel free to disregard my suggestion then.

https://chromiumcodereview.appspot.com/10915215/diff/6015/pkg/intl/intl.dart
File pkg/intl/intl.dart (right):

https://chromiumcodereview.appspot.com/10915215/diff/6015/pkg/intl/intl.dart#...
pkg/intl/intl.dart:101: static String verifiedLocale(String newLocale) {
looks like this method is no longer being used (used to be used in the
constructor.) Shouldn't we call it from the constructor?

https://chromiumcodereview.appspot.com/10915215/diff/6015/pkg/intl/message_lo...
File pkg/intl/message_lookup_local.dart (right):

https://chromiumcodereview.appspot.com/10915215/diff/6015/pkg/intl/message_lo...
pkg/intl/message_lookup_local.dart:118: LibraryMirror messagesForThisLocale =
_libraries['$_sourcePrefix$actualLocale'];
80 char alert :-P

[Alan Knight, 2012-09-13 18:20:08]

https://chromiumcodereview.appspot.com/10915215/diff/8001/pkg/intl/example/ba...
File pkg/intl/example/basic/basic_example.dart (right):

https://chromiumcodereview.appspot.com/10915215/diff/8001/pkg/intl/example/ba...
pkg/intl/example/basic/basic_example.dart:20: // a separate package.
On 2012/09/12 21:55:59, Emily Fortuna wrote:
> On 2012/09/12 20:58:20, Alan Knight wrote:
> > On 2012/09/12 01:23:48, Emily Fortuna wrote:
> > > why not use package:intl references here too to set a complete example?
> > 
> > Sadly, I had them, but took them out because I set it up to be used in a
test,
> > rather than duplicating the logic to write a test, and the buildbots aren't
> > going to understand pub until post-M1.
> 
> Ah. Fair enough. Can we add a TODO?

Done.

https://chromiumcodereview.appspot.com/10915215/diff/8001/pkg/intl/example/ba...
pkg/intl/example/basic/basic_example.dart:47: runProgram(_) {
On 2012/09/12 21:55:59, Emily Fortuna wrote:
> On 2012/09/12 20:58:20, Alan Knight wrote:
> > On 2012/09/12 01:23:48, Emily Fortuna wrote:
> > > I don't quite understand why you're passed a collection of futures. In
> > > intl_message_test_using_example.dart it looks like you're just passing in
> the
> > > print function(?) Maybe I'm mis-reading this.
> > > 
> > > If you're going to pass a not used argument, I'd still give the _ a type,
> and
> > > maybe call it "notUsed" (_ is also an okay name, though)
> > 
> > runProgram is called in the "then" call from Futures.wait on locale
> > initialization, so it gets the result, which is a collection of the Futures
> that
> > were waited on. 
> > 
> > The call from the test was wrong, and didn't get caught because the name
> didn't
> > end in _test, so it wasn't run. Fixed.
> > 
> > I liked the _ convention without a type because it made the thing visually
> > minimal, making it easy to ignore. But I put in a type.
> 
> Fair enough. I'm convinced. Feel free to disregard my suggestion then.

Done :-)

https://chromiumcodereview.appspot.com/10915215/diff/6015/pkg/intl/intl.dart
File pkg/intl/intl.dart (right):

https://chromiumcodereview.appspot.com/10915215/diff/6015/pkg/intl/intl.dart#...
pkg/intl/intl.dart:101: static String verifiedLocale(String newLocale) {
On 2012/09/12 21:55:59, Emily Fortuna wrote:
> looks like this method is no longer being used (used to be used in the
> constructor.) Shouldn't we call it from the constructor?
It's used by DateFormat, because it uses the set of locales that have date
formats, which doesn't make sense to use generally, as the set might be quite
different for locales. Added a TODO to move, rename, or generalize it. There's
another TODO to add similar functionality for messages, at which point it might
be easier to see how to generalize it.

https://chromiumcodereview.appspot.com/10915215/diff/6015/pkg/intl/message_lo...
File pkg/intl/message_lookup_local.dart (right):

https://chromiumcodereview.appspot.com/10915215/diff/6015/pkg/intl/message_lo...
pkg/intl/message_lookup_local.dart:118: LibraryMirror messagesForThisLocale =
_libraries['$_sourcePrefix$actualLocale'];
On 2012/09/12 21:55:59, Emily Fortuna wrote:
> 80 char alert :-P

Done.