First iteration on number formatting

First iteration on number formatting
Committed: https://code.google.com/p/dart/source/detail?r=10834

[Alan Knight, 2012-08-14 22:46:21]

Initial version, does basic number formatting for fixed format only and en
locale.

[Emily Fortuna, 2012-08-15 21:38:15]

https://chromiumcodereview.appspot.com/10834318/diff/2001/pkg/i18n/number_for...
File pkg/i18n/number_format.dart (right):

https://chromiumcodereview.appspot.com/10834318/diff/2001/pkg/i18n/number_for...
pkg/i18n/number_format.dart:5: #library("number_format");
nit: space between library and imports section?

https://chromiumcodereview.appspot.com/10834318/diff/2001/pkg/i18n/number_for...
pkg/i18n/number_format.dart:11: 
remove extra newline

https://chromiumcodereview.appspot.com/10834318/diff/2001/pkg/i18n/number_for...
pkg/i18n/number_format.dart:16: NumberFormat([String newPattern, String locale])
{
Can you add a TODO around here reminding us this API needs to change to be
async?

https://chromiumcodereview.appspot.com/10834318/diff/2001/pkg/i18n/number_for...
pkg/i18n/number_format.dart:22: String _negativePrefix = '-';
put fields above the constructor.

https://chromiumcodereview.appspot.com/10834318/diff/2001/pkg/i18n/number_for...
pkg/i18n/number_format.dart:27: num _groupingSize = 3;
what's this one mean? add a comment?

https://chromiumcodereview.appspot.com/10834318/diff/2001/pkg/i18n/number_for...
pkg/i18n/number_format.dart:30: int _maximumIntegerDigits = 40;
how are these determined?

https://chromiumcodereview.appspot.com/10834318/diff/2001/pkg/i18n/number_for...
pkg/i18n/number_format.dart:31: int _minimumIntegerDigits = 1;
should these be final?

https://chromiumcodereview.appspot.com/10834318/diff/2001/pkg/i18n/number_for...
pkg/i18n/number_format.dart:45: *  operation. We can have this be just an
instance variable because Dart is
indentation

https://chromiumcodereview.appspot.com/10834318/diff/2001/pkg/i18n/number_for...
pkg/i18n/number_format.dart:48: *  at a time. In languages with threads we'd
need to pass this on the stack.
Is this comment relevant? We don't have threads.....

https://chromiumcodereview.appspot.com/10834318/diff/2001/pkg/i18n/number_for...
pkg/i18n/number_format.dart:67: _setPattern(x) {}
type for arg x?

https://chromiumcodereview.appspot.com/10834318/diff/2001/pkg/i18n/number_for...
pkg/i18n/number_format.dart:91: _formatExponential(number);
indentation :-(

https://chromiumcodereview.appspot.com/10834318/diff/2001/pkg/i18n/number_for...
pkg/i18n/number_format.dart:134: * Format the basic number portion, inluding the
digits after the decimal
"digits after the decimal point" --> "fractional digits" (europe uses the ,)

https://chromiumcodereview.appspot.com/10834318/diff/2001/pkg/i18n/number_for...
pkg/i18n/number_format.dart:138: // round the number
Comments start with a capital letter and end in a period.

https://chromiumcodereview.appspot.com/10834318/diff/2001/pkg/i18n/number_for...
pkg/i18n/number_format.dart:143: 
is this an extra newline here?

https://chromiumcodereview.appspot.com/10834318/diff/2001/pkg/i18n/number_for...
pkg/i18n/number_format.dart:146: // On dartj2s the integer part may be large
enough to be a floating
but we just populated intValue = number.truncate().toInt(). So intValue *must*
be a dart int. could could it be larger...?

https://chromiumcodereview.appspot.com/10834318/diff/2001/pkg/i18n/number_for...
pkg/i18n/number_format.dart:160: _addDigit(integerDigits[i]);
question -- are numbers always RTL, or are there some LTR? (aka should we  be
starting at the front of intengerDigits?)

https://chromiumcodereview.appspot.com/10834318/diff/2001/pkg/i18n/number_for...
pkg/i18n/number_format.dart:199: bool _hasPrintableIntegerPart(int intValue) =>
intValue > 0 ||
if the function is more than one line, make it use {}

https://chromiumcodereview.appspot.com/10834318/diff/2001/pkg/i18n/number_for...
pkg/i18n/number_format.dart:206: void _newBuffer() { _buffer = new
StringBuffer();}
space after ;

https://chromiumcodereview.appspot.com/10834318/diff/2001/pkg/i18n/number_for...
pkg/i18n/number_format.dart:211: void _add(String x) { _buffer.add(x);}
space after ; , here and below

https://chromiumcodereview.appspot.com/10834318/diff/2001/pkg/i18n/number_for...
pkg/i18n/number_format.dart:230: String _group(int digitLen, int position) {
what do digitLen vs position actually mean?

https://chromiumcodereview.appspot.com/10834318/diff/2001/pkg/i18n/number_for...
pkg/i18n/number_format.dart:244: * Returns the prefix for [x] based on wether
it's positive or negative.
wether -> whether

https://chromiumcodereview.appspot.com/10834318/diff/2001/pkg/i18n/number_sym...
File pkg/i18n/number_symbols.dart (right):

https://chromiumcodereview.appspot.com/10834318/diff/2001/pkg/i18n/number_sym...
pkg/i18n/number_symbols.dart:7: * This holds onto information about how a
particular locale formats dates. It
this comment seems out of sync with the code.

[Alan Knight, 2012-08-15 23:57:44]

https://chromiumcodereview.appspot.com/10834318/diff/2001/pkg/i18n/number_for...
File pkg/i18n/number_format.dart (right):

https://chromiumcodereview.appspot.com/10834318/diff/2001/pkg/i18n/number_for...
pkg/i18n/number_format.dart:5: #library("number_format");
On 2012/08/15 21:38:16, Emily Fortuna wrote:
> nit: space between library and imports section?

Done.

https://chromiumcodereview.appspot.com/10834318/diff/2001/pkg/i18n/number_for...
pkg/i18n/number_format.dart:11: 
On 2012/08/15 21:38:16, Emily Fortuna wrote:
> remove extra newline

Done.

https://chromiumcodereview.appspot.com/10834318/diff/2001/pkg/i18n/number_for...
pkg/i18n/number_format.dart:16: NumberFormat([String newPattern, String locale])
{
On 2012/08/15 21:38:16, Emily Fortuna wrote:
> Can you add a TODO around here reminding us this API needs to change to be
> async?

Done.

https://chromiumcodereview.appspot.com/10834318/diff/2001/pkg/i18n/number_for...
pkg/i18n/number_format.dart:22: String _negativePrefix = '-';
On 2012/08/15 21:38:16, Emily Fortuna wrote:
> put fields above the constructor.

Done.

https://chromiumcodereview.appspot.com/10834318/diff/2001/pkg/i18n/number_for...
pkg/i18n/number_format.dart:27: num _groupingSize = 3;
On 2012/08/15 21:38:16, Emily Fortuna wrote:
> what's this one mean? add a comment?

Done.

https://chromiumcodereview.appspot.com/10834318/diff/2001/pkg/i18n/number_for...
pkg/i18n/number_format.dart:30: int _maximumIntegerDigits = 40;
On 2012/08/15 21:38:16, Emily Fortuna wrote:
> how are these determined?
These are hard-coded versions appropriate for an en_US type locale. Ultimately I
expect them to come from ICU data for a particular locale. They may also
disappear as variables and turn into references into the locale data, but that's
not clear.

https://chromiumcodereview.appspot.com/10834318/diff/2001/pkg/i18n/number_for...
pkg/i18n/number_format.dart:31: int _minimumIntegerDigits = 1;
On 2012/08/15 21:38:16, Emily Fortuna wrote:
> should these be final? 
They will depend on the locale. At least with the current implementation I don't
think they can be final if they rely on the result of calling a method
(verifiedLocale()).

https://chromiumcodereview.appspot.com/10834318/diff/2001/pkg/i18n/number_for...
pkg/i18n/number_format.dart:45: *  operation. We can have this be just an
instance variable because Dart is
On 2012/08/15 21:38:16, Emily Fortuna wrote:
> indentation

Done.

https://chromiumcodereview.appspot.com/10834318/diff/2001/pkg/i18n/number_for...
pkg/i18n/number_format.dart:48: *  at a time. In languages with threads we'd
need to pass this on the stack.
On 2012/08/15 21:38:16, Emily Fortuna wrote:
> Is this comment relevant? We don't have threads.....
I'm saying we can do this precisely because we don't have threads, so it's a
pattern that works in Dart but would be very bad practice in Java, C++, etc. I
thought it needed explaining so as to show the contrast.

https://chromiumcodereview.appspot.com/10834318/diff/2001/pkg/i18n/number_for...
pkg/i18n/number_format.dart:67: _setPattern(x) {}
On 2012/08/15 21:38:16, Emily Fortuna wrote:
> type for arg x?

Done.

https://chromiumcodereview.appspot.com/10834318/diff/2001/pkg/i18n/number_for...
pkg/i18n/number_format.dart:91: _formatExponential(number);
On 2012/08/15 21:38:16, Emily Fortuna wrote:
> indentation :-(

Done.

https://chromiumcodereview.appspot.com/10834318/diff/2001/pkg/i18n/number_for...
pkg/i18n/number_format.dart:134: * Format the basic number portion, inluding the
digits after the decimal
On 2012/08/15 21:38:16, Emily Fortuna wrote:
> "digits after the decimal point" --> "fractional digits" (europe uses the ,)

Done.

https://chromiumcodereview.appspot.com/10834318/diff/2001/pkg/i18n/number_for...
pkg/i18n/number_format.dart:138: // round the number
On 2012/08/15 21:38:16, Emily Fortuna wrote:
> Comments start with a capital letter and end in a period.

Done.

https://chromiumcodereview.appspot.com/10834318/diff/2001/pkg/i18n/number_for...
pkg/i18n/number_format.dart:143: 
On 2012/08/15 21:38:16, Emily Fortuna wrote:
> is this an extra newline here?

Done.

https://chromiumcodereview.appspot.com/10834318/diff/2001/pkg/i18n/number_for...
pkg/i18n/number_format.dart:146: // On dartj2s the integer part may be large
enough to be a floating
On 2012/08/15 21:38:16, Emily Fortuna wrote:
> but we just populated intValue = number.truncate().toInt(). So intValue *must*
> be a dart int. could could it be larger...?

Sadly, that's not true on dart2js. Like, seriously.
http://code.google.com/p/dart/issues/detail?id=1533

https://chromiumcodereview.appspot.com/10834318/diff/2001/pkg/i18n/number_for...
pkg/i18n/number_format.dart:160: _addDigit(integerDigits[i]);
On 2012/08/15 21:38:16, Emily Fortuna wrote:
> question -- are numbers always RTL, or are there some LTR? (aka should we  be
> starting at the front of intengerDigits?)

Good question. Added a TODO. At the moment this mirrors Closure code which
doesn't seem to do anything about printing them bidi, but that may be a
desirable thing.

https://chromiumcodereview.appspot.com/10834318/diff/2001/pkg/i18n/number_for...
pkg/i18n/number_format.dart:199: bool _hasPrintableIntegerPart(int intValue) =>
intValue > 0 ||
On 2012/08/15 21:38:16, Emily Fortuna wrote:
> if the function is more than one line, make it use {}

Done.

https://chromiumcodereview.appspot.com/10834318/diff/2001/pkg/i18n/number_for...
pkg/i18n/number_format.dart:206: void _newBuffer() { _buffer = new
StringBuffer();}
On 2012/08/15 21:38:16, Emily Fortuna wrote:
> space after ;

Done.

https://chromiumcodereview.appspot.com/10834318/diff/2001/pkg/i18n/number_for...
pkg/i18n/number_format.dart:211: void _add(String x) { _buffer.add(x);}
On 2012/08/15 21:38:16, Emily Fortuna wrote:
> space after ; , here and below

Done.

https://chromiumcodereview.appspot.com/10834318/diff/2001/pkg/i18n/number_for...
pkg/i18n/number_format.dart:230: String _group(int digitLen, int position) {
On 2012/08/15 21:38:16, Emily Fortuna wrote:
> what do digitLen vs position actually mean?

Yes, that method was confusing. Rewrote and improved the comment.

https://chromiumcodereview.appspot.com/10834318/diff/2001/pkg/i18n/number_for...
pkg/i18n/number_format.dart:244: * Returns the prefix for [x] based on wether
it's positive or negative.
On 2012/08/15 21:38:16, Emily Fortuna wrote:
> wether -> whether

Done.

https://chromiumcodereview.appspot.com/10834318/diff/2001/pkg/i18n/number_sym...
File pkg/i18n/number_symbols.dart (right):

https://chromiumcodereview.appspot.com/10834318/diff/2001/pkg/i18n/number_sym...
pkg/i18n/number_symbols.dart:7: * This holds onto information about how a
particular locale formats dates. It
On 2012/08/15 21:38:16, Emily Fortuna wrote:
> this comment seems out of sync with the code.

Oops. Done.

[Emily Fortuna, 2012-08-16 00:10:24]

lgtm, with one comment to be fixed first.

https://chromiumcodereview.appspot.com/10834318/diff/2001/pkg/i18n/number_for...
File pkg/i18n/number_format.dart (right):

https://chromiumcodereview.appspot.com/10834318/diff/2001/pkg/i18n/number_for...
pkg/i18n/number_format.dart:31: int _minimumIntegerDigits = 1;
On 2012/08/15 23:57:44, Alan Knight wrote:
> On 2012/08/15 21:38:16, Emily Fortuna wrote:
> > should these be final? 
> They will depend on the locale. At least with the current implementation I
don't
> think they can be final if they rely on the result of calling a method
> (verifiedLocale()).

What about declaring the variable as final and initializing in the constructor
after verifiedLocale is called? (or is that not a legal use of final? I can
never remember)

https://chromiumcodereview.appspot.com/10834318/diff/2001/pkg/i18n/number_for...
pkg/i18n/number_format.dart:48: *  at a time. In languages with threads we'd
need to pass this on the stack.
On 2012/08/15 23:57:44, Alan Knight wrote:
> On 2012/08/15 21:38:16, Emily Fortuna wrote:
> > Is this comment relevant? We don't have threads.....
> I'm saying we can do this precisely because we don't have threads, so it's a
> pattern that works in Dart but would be very bad practice in Java, C++, etc. I
> thought it needed explaining so as to show the contrast.

Gotcha. I wasn't reading this carefully.

https://chromiumcodereview.appspot.com/10834318/diff/1004/pkg/i18n/number_for...
File pkg/i18n/number_format.dart (right):

https://chromiumcodereview.appspot.com/10834318/diff/1004/pkg/i18n/number_for...
pkg/i18n/number_format.dart:241: if (distanceFromEnd <=1 || _groupingSize <= 0)
return;
spacing:
distanceFromEnd <= 1

[Alan Knight, 2012-08-16 16:41:49]

Fixed and committed.

https://chromiumcodereview.appspot.com/10834318/diff/2001/pkg/i18n/number_for...
File pkg/i18n/number_format.dart (right):

https://chromiumcodereview.appspot.com/10834318/diff/2001/pkg/i18n/number_for...
pkg/i18n/number_format.dart:31: int _minimumIntegerDigits = 1;
On 2012/08/16 00:10:24, Emily Fortuna wrote:
> On 2012/08/15 23:57:44, Alan Knight wrote:
> > On 2012/08/15 21:38:16, Emily Fortuna wrote:
> > > should these be final? 
> > They will depend on the locale. At least with the current implementation I
> don't
> > think they can be final if they rely on the result of calling a method
> > (verifiedLocale()).
> 
> What about declaring the variable as final and initializing in the constructor
> after verifiedLocale is called? (or is that not a legal use of final? I can
> never remember)
Hmm. I didn't think it was, but now it looks like it is, but could be tricky. To
be final, I need to initialize them in the initializer list, not in the
constructor body, but they will depend on both the locale and the pattern, so
we'd have to call the logic repeatedly to parse those, or else we'd need to make
this a factory constructor. That seems complicated, and these might disappear as
variables anyway, so just made it a TODO to see if they can be made final.

https://chromiumcodereview.appspot.com/10834318/diff/1004/pkg/i18n/number_for...
File pkg/i18n/number_format.dart (right):

https://chromiumcodereview.appspot.com/10834318/diff/1004/pkg/i18n/number_for...
pkg/i18n/number_format.dart:241: if (distanceFromEnd <=1 || _groupingSize <= 0)
return;
On 2012/08/16 00:10:24, Emily Fortuna wrote:
> spacing:
> distanceFromEnd <= 1 

Done.

[ahe, 2012-08-16 17:17:29]

This broke web-chrome-linux due to this bug: http://dartbug.com/2264