Make Intl.message static

Make Intl.message static
Committed: https://code.google.com/p/dart/source/detail?r=8620

[Alan Knight, 2012-06-11 23:07:01]

https://chromiumcodereview.appspot.com/10536105/diff/1/lib/i18n/intl.dart
File lib/i18n/intl.dart (right):

https://chromiumcodereview.appspot.com/10536105/diff/1/lib/i18n/intl.dart#new...
lib/i18n/intl.dart:55: if (locale == '') locale = _getDefaultLocale();
This is bothering me a bit. If we're going to use the locale here, and introduce
a format message in the other file, then maybe we should actually return a
message object here instead of just the unwrapped function. Otherwise we're just
throwing away the locale, and we never have an object that format can be applied
to. And if we do start to have an object and use format(), then all the tests
need to change.

Also, I think the message definition is very rarely where you'd want to specify
a locale, and it's even possible that the locale might change between when a
message is defined and when it's used. So I think if the locale is unspecified
we should leave it as such.

[Emily Fortuna, 2012-06-12 00:56:37]

PTAL! 
(I'll actually write real tests when this actually does something) :-)

[Alan Knight, 2012-06-12 17:47:20]

I tend to be in favour of writing tests even just to validate that the code
successfully doesn't do anything.

https://chromiumcodereview.appspot.com/10536105/diff/5001/lib/i18n/intl.dart
File lib/i18n/intl.dart (right):

https://chromiumcodereview.appspot.com/10536105/diff/5001/lib/i18n/intl.dart#...
lib/i18n/intl.dart:21: static String _locale = _getDefaultLocale();
I think this would have to change in line with the below comment not to be
infinitely recursive.

https://chromiumcodereview.appspot.com/10536105/diff/5001/lib/i18n/intl.dart#...
lib/i18n/intl.dart:54: final Map examples=const {}, String locale='']) {
Does it make sense now to have the locale parameter here? Is there any way we
could actually make use of it?

https://chromiumcodereview.appspot.com/10536105/diff/5001/lib/i18n/intl.dart#...
lib/i18n/intl.dart:99: return 'en-US';
Doesn't this need to be 
  if (!(_locale == null)) { return _locale; } else {return 'en-US';} 
?

[Alan Knight, 2012-06-12 17:47:31]

I tend to be in favour of writing tests even just to validate that the code
successfully doesn't do anything.

[Alan Knight, 2012-06-13 00:52:32]

https://chromiumcodereview.appspot.com/10536105/diff/6004/lib/i18n/intl.dart
File lib/i18n/intl.dart (right):

https://chromiumcodereview.appspot.com/10536105/diff/6004/lib/i18n/intl.dart#...
lib/i18n/intl.dart:81: return result;
If I set the locale here temporarily, how does the code inside msg_function see
it? 
Also, I think calling it msg_function is misleading. More likely it's something
that will call a message function. e.g.
 Intl.withLocale('pt_BR', () { print(  msgA()); print(msgB(new Date.now()))}

https://chromiumcodereview.appspot.com/10536105/diff/6004/tests/lib/i18n/intl...
File tests/lib/i18n/intl_message_test.dart (right):

https://chromiumcodereview.appspot.com/10536105/diff/6004/tests/lib/i18n/intl...
tests/lib/i18n/intl_message_test.dart:81: expect(Intl.withLocale('en-US', () =>
hello()), equals('Hello, world!'));
But this doesn't actually test that the locale can be different. We can test
that, by writing e.g.
  Intl.message("The current locale is ${getCurrentLocale()}", ... } and then
calling withLocale('de_DE') and verify that that's the locale it sees.
But right now I don't see any way for code to access the _locale that's in Intl.

[Emily Fortuna, 2012-06-13 01:27:48]

responses to comments below. PTAL.

https://chromiumcodereview.appspot.com/10536105/diff/6004/lib/i18n/intl.dart
File lib/i18n/intl.dart (right):

https://chromiumcodereview.appspot.com/10536105/diff/6004/lib/i18n/intl.dart#...
lib/i18n/intl.dart:81: return result;
On 2012/06/13 00:52:32, Alan Knight wrote:
> If I set the locale here temporarily, how does the code inside msg_function
see
> it? 
> Also, I think calling it msg_function is misleading. More likely it's
something
> that will call a message function. e.g.
>  Intl.withLocale('pt_BR', () { print(  msgA()); print(msgB(new Date.now()))}

This is where the we take advantage of the fact that dart is single threaded,
and the call to msg_function will be redirected to our "dispatching" library
that calls the right function for the desired locale. This will all make a lot
more sense once that is implemented when this is working with the dart2js api.

https://chromiumcodereview.appspot.com/10536105/diff/6004/tests/lib/i18n/intl...
File tests/lib/i18n/intl_message_test.dart (right):

https://chromiumcodereview.appspot.com/10536105/diff/6004/tests/lib/i18n/intl...
tests/lib/i18n/intl_message_test.dart:81: expect(Intl.withLocale('en-US', () =>
hello()), equals('Hello, world!'));
On 2012/06/13 00:52:32, Alan Knight wrote:
> But this doesn't actually test that the locale can be different. We can test
> that, by writing e.g.
>   Intl.message("The current locale is ${getCurrentLocale()}", ... } and then
> calling withLocale('de_DE') and verify that that's the locale it sees.
> But right now I don't see any way for code to access the _locale that's in
Intl.

_locale is hidden, and I was planning on testing the *effect* of the locale once
we're using translated Strings. How can we test this currently? Unless we want
to break up "withLocale" into two functions, and test that the current locale is
set to the anticipated locale before calling withLocale1 and withLocale2, we're
not going to be able to test state that's changed and then restored in
withLocale.

[Alan Knight, 2012-06-13 01:34:58]

https://chromiumcodereview.appspot.com/10536105/diff/6004/tests/lib/i18n/intl...
File tests/lib/i18n/intl_message_test.dart (right):

https://chromiumcodereview.appspot.com/10536105/diff/6004/tests/lib/i18n/intl...
tests/lib/i18n/intl_message_test.dart:81: expect(Intl.withLocale('en-US', () =>
hello()), equals('Hello, world!'));
On 2012/06/13 01:27:48, Emily Fortuna wrote:
> On 2012/06/13 00:52:32, Alan Knight wrote:
> > But this doesn't actually test that the locale can be different. We can test
> > that, by writing e.g.
> >   Intl.message("The current locale is ${getCurrentLocale()}", ... } and then
> > calling withLocale('de_DE') and verify that that's the locale it sees.
> > But right now I don't see any way for code to access the _locale that's in
> Intl.
> 
> _locale is hidden, and I was planning on testing the *effect* of the locale
once
> we're using translated Strings. How can we test this currently? Unless we want
> to break up "withLocale" into two functions, and test that the current locale
is
> set to the anticipated locale before calling withLocale1 and withLocale2,
we're
> not going to be able to test state that's changed and then restored in
> withLocale.
test('withLocale actually does something', () {
    hello() => Intl.message('locale=${getCurrentLocale())', desc: 'What is the
current locale'));
    expect(hello(), equals("locale = en_US"));
    withLocale('de_DE', () {
       expect(hello(), equals("locale = de_DE"))});
    expect(hello(),equals("locale = en_US"));
});

But this requires us to have some mechanism by which code can ask what the
current locale is.

[Alan Knight, 2012-06-13 15:37:09]

https://chromiumcodereview.appspot.com/10536105/diff/13001/lib/i18n/intl.dart
File lib/i18n/intl.dart (right):

https://chromiumcodereview.appspot.com/10536105/diff/13001/lib/i18n/intl.dart...
lib/i18n/intl.dart:75: static String withLocale(String locale, Function
msg_function_future) {
"future" seems like a confusing suffix, since there's a language feature by that
name, but it's not what we're using.

https://chromiumcodereview.appspot.com/10536105/diff/13001/lib/i18n/intl.dart...
lib/i18n/intl.dart:81: var result = msg_function_future();
In the longer term, we'd probably want to use a finally to ensure that if the
function throws an exception we still reset the locale.  But we can live without
that for the moment.

[Alan Knight, 2012-06-13 15:53:10]

Forgot to say that my comments were just quibbles, so lgtm.

[Emily Fortuna, 2012-06-13 17:24:04]

https://chromiumcodereview.appspot.com/10536105/diff/13001/lib/i18n/intl.dart
File lib/i18n/intl.dart (right):

https://chromiumcodereview.appspot.com/10536105/diff/13001/lib/i18n/intl.dart...
lib/i18n/intl.dart:75: static String withLocale(String locale, Function
msg_function_future) {
On 2012/06/13 15:37:09, Alan Knight wrote:
> "future" seems like a confusing suffix, since there's a language feature by
that
> name, but it's not what we're using.

Agreed. But what do you recommend instead? msg_function seemed like the best I
could come up with, you recommended against that. msg_function_promise or
msg_function_closure seem to be the only other names I can come up with them,
but both of those are strange for other reasons...?