[gwt-contrib] Re: Update i18n Messages to include: (issue796802)

2010-09-01 Thread jat 

http://gwt-code-reviews.appspot.com/796802/show

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Update i18n Messages to include: (issue796802)

2010-09-01 Thread jat 


http://gwt-code-reviews.appspot.com/796802/diff/1/23
File user/test/com/google/gwt/i18n/client/TestAnnotatedMessages.java
(right):

http://gwt-code-reviews.appspot.com/796802/diff/1/23#newcode199
user/test/com/google/gwt/i18n/client/TestAnnotatedMessages.java:199:
String reviewers(@PluralCount @Offset(2) int size,
Good suggestion, additional tests caught that the list formatting code
was totally broken for SafeHtml return values and SafeHtml argument
values.

http://gwt-code-reviews.appspot.com/796802/show

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Update i18n Messages to include: (issue796802)

2010-08-31 Thread jat 

Thanks for the review.


http://gwt-code-reviews.appspot.com/796802/diff/1/10
File user/src/com/google/gwt/i18n/rebind/MessagesMethodCreator.java
(right):

http://gwt-code-reviews.appspot.com/796802/diff/1/10#newcode425
user/src/com/google/gwt/i18n/rebind/MessagesMethodCreator.java:425:
private boolean returnsSafeHtml;
On 2010/08/31 05:02:58, xtof wrote:

final?


Done.

http://gwt-code-reviews.appspot.com/796802/diff/1/10#newcode431
user/src/com/google/gwt/i18n/rebind/MessagesMethodCreator.java:431: *
@param returnsSafeHtml if true, an expression of type {...@link SafeHtml}
is
On 2010/08/31 05:02:58, xtof wrote:

typo: is be generated


Done.

http://gwt-code-reviews.appspot.com/796802/diff/1/10#newcode506
user/src/com/google/gwt/i18n/rebind/MessagesMethodCreator.java:506: *
On 2010/08/31 05:02:58, xtof wrote:

Perhaps append a comment that this assumes the the caller is assumed

to provide

expressions that are String-valued?


Done.

http://gwt-code-reviews.appspot.com/796802/diff/1/10#newcode753
user/src/com/google/gwt/i18n/rebind/MessagesMethodCreator.java:753:
Only one list parameter supported in 
On 2010/08/31 05:02:58, xtof wrote:

Would it be better to just completely fail here instead of continuing?


Done.

http://gwt-code-reviews.appspot.com/796802/diff/1/10#newcode1001
user/src/com/google/gwt/i18n/rebind/MessagesMethodCreator.java:1001:
gen.appendExpression(val1, false, false, false);
On 2010/08/31 05:02:58, xtof wrote:

appendStringValuedExpression?



Do we actually know for sure that the val1 expression is

String-valued? (I have

to admit I'm not completely following what's going on here...)


Hmm, seems like I need to comment better then.

Basically, listPattern is always going to have {0} and {1} in it, and
the way it works is you build the list from the back.  So, {1} is always
going to be what we have built so far, which already includes doing any
formatting.

formatSecond is only true in the case of a special-case value, such as
in English 2 is special-cased as {0} and {1} (which would otherwise
be rendered as {0}, and {1}) -- in that case the second parameter is
not a string that we have built so far, but rather a value to be
formatted.

See generateListFormattingCode() below for more details on how this fits
into the bigger picture.

http://gwt-code-reviews.appspot.com/796802/diff/1/10#newcode1041
user/src/com/google/gwt/i18n/rebind/MessagesMethodCreator.java:1041:
elemType, isSafeHtml, listPattern, true, params));
Just realized this isn't as general as I thought.  CLDR currently has no
locales with special cases other than two, and this code wouldn't
support anything above 2 since it only passes two parameters, so it is
only general for 0, 1, or 2.

Maybe I should add a check for other values and give a warning and
ignore them, in case CLDR ever defines them.

http://gwt-code-reviews.appspot.com/796802/diff/1/23
File user/test/com/google/gwt/i18n/client/TestAnnotatedMessages.java
(right):

http://gwt-code-reviews.appspot.com/796802/diff/1/23#newcode67
user/test/com/google/gwt/i18n/client/TestAnnotatedMessages.java:67:
String australianDollars(double amount);
On 2010/08/31 05:02:58, xtof wrote:

Should these have SafeHtml equivalent tests too?


Will do.

http://gwt-code-reviews.appspot.com/796802/diff/1/23#newcode199
user/test/com/google/gwt/i18n/client/TestAnnotatedMessages.java:199:
String reviewers(@PluralCount @Offset(2) int size,
On 2010/08/31 05:02:58, xtof wrote:

Might be useful to have tests of SafeHtml versions of these ones too?


Will do.

http://gwt-code-reviews.appspot.com/796802/show

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Update i18n Messages to include: (issue796802)

2010-08-30 Thread xtof 


http://gwt-code-reviews.appspot.com/796802/diff/1/10
File user/src/com/google/gwt/i18n/rebind/MessagesMethodCreator.java
(right):

http://gwt-code-reviews.appspot.com/796802/diff/1/10#newcode425
user/src/com/google/gwt/i18n/rebind/MessagesMethodCreator.java:425:
private boolean returnsSafeHtml;
final?

http://gwt-code-reviews.appspot.com/796802/diff/1/10#newcode431
user/src/com/google/gwt/i18n/rebind/MessagesMethodCreator.java:431: *
@param returnsSafeHtml if true, an expression of type {...@link SafeHtml}
is
typo: is be generated

http://gwt-code-reviews.appspot.com/796802/diff/1/10#newcode506
user/src/com/google/gwt/i18n/rebind/MessagesMethodCreator.java:506: *
Perhaps append a comment that this assumes the the caller is assumed to
provide expressions that are String-valued?

http://gwt-code-reviews.appspot.com/796802/diff/1/10#newcode753
user/src/com/google/gwt/i18n/rebind/MessagesMethodCreator.java:753:
Only one list parameter supported in 
Would it be better to just completely fail here instead of continuing?

http://gwt-code-reviews.appspot.com/796802/diff/1/10#newcode1001
user/src/com/google/gwt/i18n/rebind/MessagesMethodCreator.java:1001:
gen.appendExpression(val1, false, false, false);
appendStringValuedExpression?

Do we actually know for sure that the val1 expression is String-valued?
(I have to admit I'm not completely following what's going on here...)

http://gwt-code-reviews.appspot.com/796802/diff/1/23
File user/test/com/google/gwt/i18n/client/TestAnnotatedMessages.java
(right):

http://gwt-code-reviews.appspot.com/796802/diff/1/23#newcode67
user/test/com/google/gwt/i18n/client/TestAnnotatedMessages.java:67:
String australianDollars(double amount);
Should these have SafeHtml equivalent tests too?

http://gwt-code-reviews.appspot.com/796802/diff/1/23#newcode199
user/test/com/google/gwt/i18n/client/TestAnnotatedMessages.java:199:
String reviewers(@PluralCount @Offset(2) int size,
Might be useful to have tests of SafeHtml versions of these ones too?

http://gwt-code-reviews.appspot.com/796802/show

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Update i18n Messages to include: (issue796802)

2010-08-27 Thread pdr 

Just 2 small issues. Would it be possible to add someone to this review
with extensive i18n experience?


http://gwt-code-reviews.appspot.com/796802/diff/1/8
File user/src/com/google/gwt/i18n/rebind/MessageFormatParser.java
(left):

http://gwt-code-reviews.appspot.com/796802/diff/1/8#oldcode190
user/src/com/google/gwt/i18n/rebind/MessageFormatParser.java:190: }
Why did you move away from regular expressions here?

http://gwt-code-reviews.appspot.com/796802/diff/1/33
File user/test/com/google/gwt/i18n/rebind/MessageFormatParserTest.java
(right):

http://gwt-code-reviews.appspot.com/796802/diff/1/33#newcode61
user/test/com/google/gwt/i18n/rebind/MessageFormatParserTest.java:61: }
Indenting

http://gwt-code-reviews.appspot.com/796802/show

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Update i18n Messages to include: (issue796802)

2010-08-27 Thread jat 

There isn't really any one with extensive GWT i18n experience.  The
person who originally wrote it is no longer here, and I have been pretty
much the only person doing anything with it since then.


http://gwt-code-reviews.appspot.com/796802/diff/1/8
File user/src/com/google/gwt/i18n/rebind/MessageFormatParser.java
(left):

http://gwt-code-reviews.appspot.com/796802/diff/1/8#oldcode190
user/src/com/google/gwt/i18n/rebind/MessageFormatParser.java:190: }
On 2010/08/27 21:12:44, pdr wrote:

Why did you move away from regular expressions here?


It was insufficient to parse the argument, since there can now be
quoting inside the {}.

http://gwt-code-reviews.appspot.com/796802/diff/1/33
File user/test/com/google/gwt/i18n/rebind/MessageFormatParserTest.java
(right):

http://gwt-code-reviews.appspot.com/796802/diff/1/33#newcode61
user/test/com/google/gwt/i18n/rebind/MessageFormatParserTest.java:61: }
On 2010/08/27 21:12:44, pdr wrote:

Indenting


Done.

http://gwt-code-reviews.appspot.com/796802/show

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Update i18n Messages to include: (issue796802)

2010-08-25 Thread jat 


http://gwt-code-reviews.appspot.com/796802/diff/1/10
File user/src/com/google/gwt/i18n/rebind/MessagesMethodCreator.java
(right):

http://gwt-code-reviews.appspot.com/796802/diff/1/10#newcode645
user/src/com/google/gwt/i18n/rebind/MessagesMethodCreator.java:645:
protected static int findParam(JParameter[] params, String name) {
Just realized this isn't used, so I removed it.

http://gwt-code-reviews.appspot.com/796802/diff/1/10#newcode963
user/src/com/google/gwt/i18n/rebind/MessagesMethodCreator.java:963: //
format.
This TODO is done, removed.

http://gwt-code-reviews.appspot.com/796802/show

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors