Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10494005:
Updated message formatting to be more compatible with Dart string interpolation. (Closed)
|
Created:
8 years, 6 months ago by Alan Knight Modified:
8 years, 6 months ago Reviewers:
Emily Fortuna CC:
reviews_dartlang.org Visibility:
Public. |
DescriptionUpdated message formatting to be more compatible with Dart string interpolation.
BUG=
TEST=
Committed: https://code.google.com/p/dart/source/detail?r=8412
Patch Set 1 #
Total comments: 29
Patch Set 2 : Updated to reflect changes from review #
Total comments: 20
Patch Set 3 : Changes made from review #
Total comments: 18
Patch Set 4 : More changes from review #
Total comments: 11
Patch Set 5 : Yet more review cleanups #
Total comments: 6
Patch Set 6 : And a few more cleanups. #
Total comments: 16
Patch Set 7 : Even more #Patch Set 8 : Merged up to head, and wasn't that fun #
Total comments: 22
Patch Set 9 : Fixed up some merge errors, tweaked comments a little. #
Total comments: 30
Patch Set 10 : Mostly formatting #
Messages
Total messages: 19 (0 generated)
My very first review...
https://chromiumcodereview.appspot.com/10494005/diff/1/lib/i18n/intl.dart File lib/i18n/intl.dart (right): https://chromiumcodereview.appspot.com/10494005/diff/1/lib/i18n/intl.dart#new... lib/i18n/intl.dart:8: get rid of extra whitespace (one or 2 lines only) https://chromiumcodereview.appspot.com/10494005/diff/1/lib/i18n/intl.dart#new... lib/i18n/intl.dart:21: * TODO(alanknight) Actually make this class do something with locales, just a skeleton right now. line break this line so it's less than 80 char https://chromiumcodereview.appspot.com/10494005/diff/1/lib/i18n/intl.dart#new... lib/i18n/intl.dart:28: Intl(); How about instead of the named constructor, do: Intl([this._locale]); https://chromiumcodereview.appspot.com/10494005/diff/1/lib/i18n/intl.dart#new... lib/i18n/intl.dart:33: **/ nits: ending comment line is */ not **/ (yes, we're that picky) https://chromiumcodereview.appspot.com/10494005/diff/1/lib/i18n/intl.dart#new... lib/i18n/intl.dart:38: extra whitespace https://chromiumcodereview.appspot.com/10494005/diff/1/lib/i18n/intl.dart#new... lib/i18n/intl.dart:43: String plural(num howMany, Map actions) { Can we add a select method, here, too? It can be dumb and defer to the plural method or whatever, but let's just make it available. https://chromiumcodereview.appspot.com/10494005/diff/1/lib/i18n/message_forma... File lib/i18n/message_format.dart (right): https://chromiumcodereview.appspot.com/10494005/diff/1/lib/i18n/message_forma... lib/i18n/message_format.dart:9: * Messages are written as functions with either one parameter (which can omitted). add line breaks for 80 char here and below https://chromiumcodereview.appspot.com/10494005/diff/1/lib/i18n/message_forma... lib/i18n/message_format.dart:26: * "$NUM other ${intl.plural(NUM, {'1':'person', 'other': 'people'})} here."; get rid of tabs in this line and below --> only spaces https://chromiumcodereview.appspot.com/10494005/diff/1/lib/i18n/message_forma... lib/i18n/message_format.dart:40: * The message which we are formatting. if it's a one line comment, we can shorten it to: /** The message which we are formatting. */ https://chromiumcodereview.appspot.com/10494005/diff/1/lib/i18n/message_forma... lib/i18n/message_format.dart:47: MessageFormat.on(this._messageFunction); My impression so far is Dart is much more likely to take optional parameters than named constructors. https://chromiumcodereview.appspot.com/10494005/diff/1/lib/i18n/message_forma... lib/i18n/message_format.dart:53: * and one where the message is passed in as an argument. comments please explaining what messageParameters is https://chromiumcodereview.appspot.com/10494005/diff/1/lib/i18n/message_forma... lib/i18n/message_format.dart:60: String formatMessage(Function messageFunction,var messageParameters) { comments please? https://chromiumcodereview.appspot.com/10494005/diff/1/tests/lib/i18n/message... File tests/lib/i18n/message_format_test.dart (right): https://chromiumcodereview.appspot.com/10494005/diff/1/tests/lib/i18n/message... tests/lib/i18n/message_format_test.dart:27: }); this line should be indented two spaces, I believe. Here and below. https://chromiumcodereview.appspot.com/10494005/diff/1/tests/lib/i18n/message... tests/lib/i18n/message_format_test.dart:45: //@desc Doesn't matter get rid of tabs ==> replace with spaces.
I think I fixed everything. https://chromiumcodereview.appspot.com/10494005/diff/1/lib/i18n/intl.dart File lib/i18n/intl.dart (right): https://chromiumcodereview.appspot.com/10494005/diff/1/lib/i18n/intl.dart#new... lib/i18n/intl.dart:8: On 2012/06/02 01:19:26, Emily Fortuna wrote: > get rid of extra whitespace (one or 2 lines only) Done. https://chromiumcodereview.appspot.com/10494005/diff/1/lib/i18n/intl.dart#new... lib/i18n/intl.dart:21: * TODO(alanknight) Actually make this class do something with locales, just a skeleton right now. On 2012/06/02 01:19:26, Emily Fortuna wrote: > line break this line so it's less than 80 char Done. https://chromiumcodereview.appspot.com/10494005/diff/1/lib/i18n/intl.dart#new... lib/i18n/intl.dart:28: Intl(); On 2012/06/02 01:19:26, Emily Fortuna wrote: > How about instead of the named constructor, do: > Intl([this._locale]); Ah, I didn't realize you could do that without making them specify the name of the argument when calling it. Cool. https://chromiumcodereview.appspot.com/10494005/diff/1/lib/i18n/intl.dart#new... lib/i18n/intl.dart:28: Intl(); On 2012/06/02 01:19:26, Emily Fortuna wrote: > How about instead of the named constructor, do: > Intl([this._locale]); Cool. I didn't realize you could do that without forcing people to use the parameter name when calling it. Done. https://chromiumcodereview.appspot.com/10494005/diff/1/lib/i18n/intl.dart#new... lib/i18n/intl.dart:33: **/ On 2012/06/02 01:19:26, Emily Fortuna wrote: > nits: ending comment line is */ not **/ (yes, we're that picky) Done. https://chromiumcodereview.appspot.com/10494005/diff/1/lib/i18n/intl.dart#new... lib/i18n/intl.dart:33: **/ On 2012/06/02 01:19:26, Emily Fortuna wrote: > nits: ending comment line is */ not **/ (yes, we're that picky) Done. And fixed in various other places. https://chromiumcodereview.appspot.com/10494005/diff/1/lib/i18n/intl.dart#new... lib/i18n/intl.dart:38: On 2012/06/02 01:19:26, Emily Fortuna wrote: > extra whitespace Done. https://chromiumcodereview.appspot.com/10494005/diff/1/lib/i18n/intl.dart#new... lib/i18n/intl.dart:43: String plural(num howMany, Map actions) { On 2012/06/02 01:19:26, Emily Fortuna wrote: > Can we add a select method, here, too? It can be dumb and defer to the plural > method or whatever, but let's just make it available. Done. Also added a check to plural that if the 'other' case is missing, we return an empty string, which is also the fallback in select. https://chromiumcodereview.appspot.com/10494005/diff/1/lib/i18n/message_forma... File lib/i18n/message_format.dart (right): https://chromiumcodereview.appspot.com/10494005/diff/1/lib/i18n/message_forma... lib/i18n/message_format.dart:9: * Messages are written as functions with either one parameter (which can omitted). On 2012/06/02 01:19:26, Emily Fortuna wrote: > add line breaks for 80 char here and below Done. https://chromiumcodereview.appspot.com/10494005/diff/1/lib/i18n/message_forma... lib/i18n/message_format.dart:26: * "$NUM other ${intl.plural(NUM, {'1':'person', 'other': 'people'})} here."; On 2012/06/02 01:19:26, Emily Fortuna wrote: > get rid of tabs in this line and below --> only spaces Done. https://chromiumcodereview.appspot.com/10494005/diff/1/lib/i18n/message_forma... lib/i18n/message_format.dart:40: * The message which we are formatting. On 2012/06/02 01:19:26, Emily Fortuna wrote: > if it's a one line comment, we can shorten it to: > /** The message which we are formatting. */ Done. https://chromiumcodereview.appspot.com/10494005/diff/1/lib/i18n/message_forma... lib/i18n/message_format.dart:47: MessageFormat.on(this._messageFunction); On 2012/06/02 01:19:26, Emily Fortuna wrote: > My impression so far is Dart is much more likely to take optional parameters > than named constructors. Changed to an optional parameter https://chromiumcodereview.appspot.com/10494005/diff/1/lib/i18n/message_forma... lib/i18n/message_format.dart:53: * and one where the message is passed in as an argument. On 2012/06/02 01:19:26, Emily Fortuna wrote: > comments please explaining what messageParameters is Done. https://chromiumcodereview.appspot.com/10494005/diff/1/tests/lib/i18n/message... File tests/lib/i18n/message_format_test.dart (right): https://chromiumcodereview.appspot.com/10494005/diff/1/tests/lib/i18n/message... tests/lib/i18n/message_format_test.dart:27: }); On 2012/06/02 01:19:26, Emily Fortuna wrote: > this line should be indented two spaces, I believe. Here and below. Done. https://chromiumcodereview.appspot.com/10494005/diff/1/tests/lib/i18n/message... tests/lib/i18n/message_format_test.dart:45: //@desc Doesn't matter On 2012/06/02 01:19:26, Emily Fortuna wrote: > get rid of tabs ==> replace with spaces. Done.
looking good! a few more style nits. https://chromiumcodereview.appspot.com/10494005/diff/6002/lib/i18n/intl.dart File lib/i18n/intl.dart (right): https://chromiumcodereview.appspot.com/10494005/diff/6002/lib/i18n/intl.dart#... lib/i18n/intl.dart:25: **/ get rid of extra astrisk here https://chromiumcodereview.appspot.com/10494005/diff/6002/lib/i18n/intl.dart#... lib/i18n/intl.dart:30: */ indent one more space here. https://chromiumcodereview.appspot.com/10494005/diff/6002/lib/i18n/intl.dart#... lib/i18n/intl.dart:39: String plural(num howMany, Map actions) { one more space here https://chromiumcodereview.appspot.com/10494005/diff/6002/lib/i18n/intl.dart#... lib/i18n/intl.dart:47: {return "";}; nit: up above you use the single quotes for strings and here you use double. Can we make them the same throughout unless there's a compelling reason otherwise? (personally I prefer single quotes in general) https://chromiumcodereview.appspot.com/10494005/diff/6002/lib/i18n/intl.dart#... lib/i18n/intl.dart:53: return ""; same here https://chromiumcodereview.appspot.com/10494005/diff/6002/lib/i18n/message_fo... File lib/i18n/message_format.dart (right): https://chromiumcodereview.appspot.com/10494005/diff/6002/lib/i18n/message_fo... lib/i18n/message_format.dart:54: * The variable messageParameters can be null, in which case the message [messageParameters] (see comment below) https://chromiumcodereview.appspot.com/10494005/diff/6002/lib/i18n/message_fo... lib/i18n/message_format.dart:69: * The variable messageParameters can be null, in which case the message with markdown syntax, put a bracket around the parameter name [messageParameters] when you're describing it. This will make a nice automatic link for the dart documentation. https://chromiumcodereview.appspot.com/10494005/diff/6002/tests/lib/i18n/mess... File tests/lib/i18n/message_format_test.dart (right): https://chromiumcodereview.appspot.com/10494005/diff/6002/tests/lib/i18n/mess... tests/lib/i18n/message_format_test.dart:39: /** indent two spaces here, please! also the rest of this comment should be indented with test(... https://chromiumcodereview.appspot.com/10494005/diff/6002/tests/lib/i18n/mess... tests/lib/i18n/message_format_test.dart:44: test('Evaluate the function by mistake', () { I think this should actually be un-indented two spaces -- we want it to line up with the }); on line 50 https://chromiumcodereview.appspot.com/10494005/diff/6002/tests/lib/i18n/mess... tests/lib/i18n/message_format_test.dart:53: test('Complex message with plural', () { same indentation nits here and below.
Changed. Though now I'm not sure where we stand with these changes versus other changes to these files in other CLs https://chromiumcodereview.appspot.com/10494005/diff/6002/lib/i18n/intl.dart File lib/i18n/intl.dart (right): https://chromiumcodereview.appspot.com/10494005/diff/6002/lib/i18n/intl.dart#... lib/i18n/intl.dart:25: **/ On 2012/06/04 23:34:52, Emily Fortuna wrote: > get rid of extra astrisk here Done. https://chromiumcodereview.appspot.com/10494005/diff/6002/lib/i18n/intl.dart#... lib/i18n/intl.dart:30: */ On 2012/06/04 23:34:52, Emily Fortuna wrote: > indent one more space here. Done. https://chromiumcodereview.appspot.com/10494005/diff/6002/lib/i18n/intl.dart#... lib/i18n/intl.dart:39: String plural(num howMany, Map actions) { On 2012/06/04 23:34:52, Emily Fortuna wrote: > one more space here Done. https://chromiumcodereview.appspot.com/10494005/diff/6002/lib/i18n/intl.dart#... lib/i18n/intl.dart:47: {return "";}; On 2012/06/04 23:34:52, Emily Fortuna wrote: > nit: up above you use the single quotes for strings and here you use double. Can > we make them the same throughout unless there's a compelling reason otherwise? > (personally I prefer single quotes in general) Done. https://chromiumcodereview.appspot.com/10494005/diff/6002/lib/i18n/intl.dart#... lib/i18n/intl.dart:53: return ""; On 2012/06/04 23:34:52, Emily Fortuna wrote: > same here Done. https://chromiumcodereview.appspot.com/10494005/diff/6002/lib/i18n/message_fo... File lib/i18n/message_format.dart (right): https://chromiumcodereview.appspot.com/10494005/diff/6002/lib/i18n/message_fo... lib/i18n/message_format.dart:54: * The variable messageParameters can be null, in which case the message On 2012/06/04 23:34:52, Emily Fortuna wrote: > [messageParameters] (see comment below) Done. https://chromiumcodereview.appspot.com/10494005/diff/6002/lib/i18n/message_fo... lib/i18n/message_format.dart:69: * The variable messageParameters can be null, in which case the message On 2012/06/04 23:34:52, Emily Fortuna wrote: > with markdown syntax, put a bracket around the parameter name > [messageParameters] when you're describing it. This will make a nice automatic > link for the dart documentation. Done. https://chromiumcodereview.appspot.com/10494005/diff/6002/tests/lib/i18n/mess... File tests/lib/i18n/message_format_test.dart (right): https://chromiumcodereview.appspot.com/10494005/diff/6002/tests/lib/i18n/mess... tests/lib/i18n/message_format_test.dart:39: /** On 2012/06/04 23:34:52, Emily Fortuna wrote: > indent two spaces here, please! > also the rest of this comment should be indented with test(... Done. https://chromiumcodereview.appspot.com/10494005/diff/6002/tests/lib/i18n/mess... tests/lib/i18n/message_format_test.dart:44: test('Evaluate the function by mistake', () { On 2012/06/04 23:34:52, Emily Fortuna wrote: > I think this should actually be un-indented two spaces -- we want it to line up > with the }); on line 50 Done. https://chromiumcodereview.appspot.com/10494005/diff/6002/tests/lib/i18n/mess... tests/lib/i18n/message_format_test.dart:53: test('Complex message with plural', () { On 2012/06/04 23:34:52, Emily Fortuna wrote: > same indentation nits here and below. Done.
In regard to your general comment -- I think these changes are still relevant. whoever commits second will have a little bit of resolving conflicts to do, but I think you should carry forward with this. https://chromiumcodereview.appspot.com/10494005/diff/14001/lib/i18n/intl.dart File lib/i18n/intl.dart (right): https://chromiumcodereview.appspot.com/10494005/diff/14001/lib/i18n/intl.dart... lib/i18n/intl.dart:22: var _locale; this guy should be de-indented one space. It should line up with the first / on line 14 https://chromiumcodereview.appspot.com/10494005/diff/14001/lib/i18n/intl.dart... lib/i18n/intl.dart:47: else Also, preferred formatting for this is: if (blah) { do stuff; } else { do other stuff; } https://chromiumcodereview.appspot.com/10494005/diff/14001/lib/i18n/intl.dart... lib/i18n/intl.dart:48: {return '';;}; extra semicolon! https://chromiumcodereview.appspot.com/10494005/diff/14001/lib/i18n/intl.dart... lib/i18n/intl.dart:53: if (choice == key) {return actions[key];}; the preferred format is if you have a one-line if statement, exclude brackets {}: if (choice == key) return actions[key]; https://chromiumcodereview.appspot.com/10494005/diff/14001/lib/i18n/intl.dart... lib/i18n/intl.dart:54: return ''; I think this return should be one line down -- otherrwise you're returning after the first iteration of your loop? https://chromiumcodereview.appspot.com/10494005/diff/14001/lib/i18n/message_f... File lib/i18n/message_format.dart (right): https://chromiumcodereview.appspot.com/10494005/diff/14001/lib/i18n/message_f... lib/i18n/message_format.dart:9: * Messages are written as functions with either one parameter (which can omitted). 80 char https://chromiumcodereview.appspot.com/10494005/diff/14001/lib/i18n/message_f... lib/i18n/message_format.dart:54: * The variable messageParameters can be null, in which case the message brackets...? https://chromiumcodereview.appspot.com/10494005/diff/14001/lib/i18n/message_f... lib/i18n/message_format.dart:66: * Formats the messageFunction argument and returns the correctly also brackets around this guy. https://chromiumcodereview.appspot.com/10494005/diff/14001/tests/lib/i18n/mes... File tests/lib/i18n/message_format_test.dart (right): https://chromiumcodereview.appspot.com/10494005/diff/14001/tests/lib/i18n/mes... tests/lib/i18n/message_format_test.dart:44: test('Evaluate the function by mistake', () { lovely!
https://chromiumcodereview.appspot.com/10494005/diff/14001/lib/i18n/intl.dart File lib/i18n/intl.dart (right): https://chromiumcodereview.appspot.com/10494005/diff/14001/lib/i18n/intl.dart... lib/i18n/intl.dart:22: var _locale; On 2012/06/05 00:30:39, Emily Fortuna wrote: > this guy should be de-indented one space. It should line up with the first / on > line 14 Done. https://chromiumcodereview.appspot.com/10494005/diff/14001/lib/i18n/intl.dart... lib/i18n/intl.dart:47: else On 2012/06/05 00:30:39, Emily Fortuna wrote: > Also, preferred formatting for this is: > if (blah) { > do stuff; > } else { > do other stuff; > } Done. https://chromiumcodereview.appspot.com/10494005/diff/14001/lib/i18n/intl.dart... lib/i18n/intl.dart:48: {return '';;}; On 2012/06/05 00:30:39, Emily Fortuna wrote: > extra semicolon! Done. https://chromiumcodereview.appspot.com/10494005/diff/14001/lib/i18n/intl.dart... lib/i18n/intl.dart:53: if (choice == key) {return actions[key];}; On 2012/06/05 00:30:39, Emily Fortuna wrote: > the preferred format is if you have a one-line if statement, exclude brackets > {}: > if (choice == key) return actions[key]; Done. https://chromiumcodereview.appspot.com/10494005/diff/14001/lib/i18n/intl.dart... lib/i18n/intl.dart:54: return ''; On 2012/06/05 00:30:39, Emily Fortuna wrote: > I think this return should be one line down -- otherrwise you're returning after > the first iteration of your loop? Oops. Done. https://chromiumcodereview.appspot.com/10494005/diff/14001/lib/i18n/message_f... File lib/i18n/message_format.dart (right): https://chromiumcodereview.appspot.com/10494005/diff/14001/lib/i18n/message_f... lib/i18n/message_format.dart:9: * Messages are written as functions with either one parameter (which can omitted). On 2012/06/05 00:30:39, Emily Fortuna wrote: > 80 char Done. https://chromiumcodereview.appspot.com/10494005/diff/14001/lib/i18n/message_f... lib/i18n/message_format.dart:54: * The variable messageParameters can be null, in which case the message On 2012/06/05 00:30:39, Emily Fortuna wrote: > brackets...? Done. https://chromiumcodereview.appspot.com/10494005/diff/14001/lib/i18n/message_f... lib/i18n/message_format.dart:66: * Formats the messageFunction argument and returns the correctly On 2012/06/05 00:30:39, Emily Fortuna wrote: > also brackets around this guy. Done. https://chromiumcodereview.appspot.com/10494005/diff/14001/tests/lib/i18n/mes... File tests/lib/i18n/message_format_test.dart (right): https://chromiumcodereview.appspot.com/10494005/diff/14001/tests/lib/i18n/mes... tests/lib/i18n/message_format_test.dart:44: test('Evaluate the function by mistake', () { On 2012/06/05 00:30:39, Emily Fortuna wrote: > lovely! :-)
https://chromiumcodereview.appspot.com/10494005/diff/18001/lib/i18n/date_form... File lib/i18n/date_format.dart (right): https://chromiumcodereview.appspot.com/10494005/diff/18001/lib/i18n/date_form... lib/i18n/date_format.dart:150: DateFormat.date() {formatDefinition = _fullDate;} change these to use the initializer list syntax: DateFormat.time() : formatDefinition = _fullTime; https://chromiumcodereview.appspot.com/10494005/diff/18001/lib/i18n/intl.dart File lib/i18n/intl.dart (right): https://chromiumcodereview.appspot.com/10494005/diff/18001/lib/i18n/intl.dart... lib/i18n/intl.dart:24: /** what happened on the indentation here? it looks like this / is one space ahead of the one on line 14? https://chromiumcodereview.appspot.com/10494005/diff/18001/lib/i18n/intl.dart... lib/i18n/intl.dart:32: get date() => new DateFormat.date(); This this is a public API, let's explicitly list the return type of these getters. https://chromiumcodereview.appspot.com/10494005/diff/18001/lib/i18n/intl.dart... lib/i18n/intl.dart:43: if(desiredKey == key) {return actions[key];} no {} here either (consistency throughout the code is key!)
https://chromiumcodereview.appspot.com/10494005/diff/18001/lib/i18n/date_form... File lib/i18n/date_format.dart (right): https://chromiumcodereview.appspot.com/10494005/diff/18001/lib/i18n/date_form... lib/i18n/date_format.dart:150: DateFormat.date() {formatDefinition = _fullDate;} On 2012/06/05 17:40:09, Emily Fortuna wrote: > change these to use the initializer list syntax: > DateFormat.time() : formatDefinition = _fullTime; Done. https://chromiumcodereview.appspot.com/10494005/diff/18001/lib/i18n/intl.dart File lib/i18n/intl.dart (right): https://chromiumcodereview.appspot.com/10494005/diff/18001/lib/i18n/intl.dart... lib/i18n/intl.dart:24: /** On 2012/06/05 17:40:09, Emily Fortuna wrote: > what happened on the indentation here? it looks like this / is one space ahead > of the one on line 14? Done. https://chromiumcodereview.appspot.com/10494005/diff/18001/lib/i18n/intl.dart... lib/i18n/intl.dart:32: get date() => new DateFormat.date(); On 2012/06/05 17:40:09, Emily Fortuna wrote: > This this is a public API, let's explicitly list the return type of these > getters. I notice that in the implementation, right now these return a new instance every time, which isn't what we were saying in the meeting. Possibly if they do return a new instance they shouldn't be getters. Done. https://chromiumcodereview.appspot.com/10494005/diff/18001/lib/i18n/intl.dart... lib/i18n/intl.dart:43: if(desiredKey == key) {return actions[key];} On 2012/06/05 17:40:09, Emily Fortuna wrote: > no {} here either (consistency throughout the code is key!) Done.
getting close! https://chromiumcodereview.appspot.com/10494005/diff/18001/lib/i18n/date_form... File lib/i18n/date_format.dart (right): https://chromiumcodereview.appspot.com/10494005/diff/18001/lib/i18n/date_form... lib/i18n/date_format.dart:111: // TODO(alanknight): There's a style question of whether to use fullDate or hooray for trailing whitespace removal! https://chromiumcodereview.appspot.com/10494005/diff/18001/lib/i18n/intl.dart File lib/i18n/intl.dart (right): https://chromiumcodereview.appspot.com/10494005/diff/18001/lib/i18n/intl.dart... lib/i18n/intl.dart:32: get date() => new DateFormat.date(); On 2012/06/05 21:47:34, Alan Knight wrote: > On 2012/06/05 17:40:09, Emily Fortuna wrote: > > This this is a public API, let's explicitly list the return type of these > > getters. > > I notice that in the implementation, right now these return a new instance every > time, which isn't what we were saying in the meeting. Possibly if they do return > a new instance they shouldn't be getters. > > Done. Absolutely. I hadn't harped on this much because I figured this would probably be changed once we started implementing things. It goes with what's mentioned about getter style: http://www.dartlang.org/articles/style-guide/ (don't have getters with side effects) https://chromiumcodereview.appspot.com/10494005/diff/24001/lib/i18n/date_form... File lib/i18n/date_format.dart (right): https://chromiumcodereview.appspot.com/10494005/diff/24001/lib/i18n/date_form... lib/i18n/date_format.dart:107: /** Definition of how this object formats dates. the top line of this sort of comment should be blank: /** * Comment begins here. https://chromiumcodereview.appspot.com/10494005/diff/24001/lib/i18n/date_form... lib/i18n/date_format.dart:112: /** The locale we use to format dates. same. https://chromiumcodereview.appspot.com/10494005/diff/24001/lib/i18n/intl.dart File lib/i18n/intl.dart (right): https://chromiumcodereview.appspot.com/10494005/diff/24001/lib/i18n/intl.dart... lib/i18n/intl.dart:32: DateFormat get date() => new DateFormat.date(); I'm sorry to keep bugging you about this indentation thing -- it is improving! I think this guy should indented one less space, right? same with lines 33-35 and 40 (and the other lines involved with the function at 40)? Just FYI, in the event you're not using the Dart editor, there are syntax files in tools/utils for emacs, vim, textmate (and if you ask around, some of people around here use sublime and must have something that works for that). These guys will help enforce the proper indentation and give nice Dart syntax highlighting.
https://chromiumcodereview.appspot.com/10494005/diff/18001/lib/i18n/intl.dart File lib/i18n/intl.dart (right): https://chromiumcodereview.appspot.com/10494005/diff/18001/lib/i18n/intl.dart... lib/i18n/intl.dart:32: get date() => new DateFormat.date(); On 2012/06/05 22:34:16, Emily Fortuna wrote: > On 2012/06/05 21:47:34, Alan Knight wrote: > > On 2012/06/05 17:40:09, Emily Fortuna wrote: > > > This this is a public API, let's explicitly list the return type of these > > > getters. > > > > I notice that in the implementation, right now these return a new instance > every > > time, which isn't what we were saying in the meeting. Possibly if they do > return > > a new instance they shouldn't be getters. > > > > Done. > > > Absolutely. I hadn't harped on this much because I figured this would probably > be changed once we started implementing things. It goes with what's mentioned > about getter style: http://www.dartlang.org/articles/style-guide/ (don't have > getters with side effects) OK, made them non-getters. https://chromiumcodereview.appspot.com/10494005/diff/24001/lib/i18n/date_form... File lib/i18n/date_format.dart (right): https://chromiumcodereview.appspot.com/10494005/diff/24001/lib/i18n/date_form... lib/i18n/date_format.dart:107: /** Definition of how this object formats dates. On 2012/06/05 22:34:17, Emily Fortuna wrote: > the top line of this sort of comment should be blank: > /** > * Comment begins here. Done. https://chromiumcodereview.appspot.com/10494005/diff/24001/lib/i18n/date_form... lib/i18n/date_format.dart:112: /** The locale we use to format dates. On 2012/06/05 22:34:17, Emily Fortuna wrote: > same. Done. https://chromiumcodereview.appspot.com/10494005/diff/24001/lib/i18n/intl.dart File lib/i18n/intl.dart (right): https://chromiumcodereview.appspot.com/10494005/diff/24001/lib/i18n/intl.dart... lib/i18n/intl.dart:32: DateFormat get date() => new DateFormat.date(); On 2012/06/05 22:34:17, Emily Fortuna wrote: > I'm sorry to keep bugging you about this indentation thing -- it is improving! I > think this guy should indented one less space, right? same with lines 33-35 and > 40 (and the other lines involved with the function at 40)? > > Just FYI, in the event you're not using the Dart editor, there are syntax files > in tools/utils for emacs, vim, textmate (and if you ask around, some of people > around here use sublime and must have something that works for that). These guys > will help enforce the proper indentation and give nice Dart syntax highlighting. If only the emacs mode actually worked... Done.
https://chromiumcodereview.appspot.com/10494005/diff/25002/lib/i18n/date_form... File lib/i18n/date_format.dart (right): https://chromiumcodereview.appspot.com/10494005/diff/25002/lib/i18n/date_form... lib/i18n/date_format.dart:120: /** Date/Time format patterns. */ 120-134 is all indented one extra space. not such a big deal though, since this is getting replaced. https://chromiumcodereview.appspot.com/10494005/diff/25002/lib/i18n/date_form... lib/i18n/date_format.dart:153: * Explicit constructor given a particular format comment must end in period. https://chromiumcodereview.appspot.com/10494005/diff/25002/lib/i18n/intl.dart File lib/i18n/intl.dart (right): https://chromiumcodereview.appspot.com/10494005/diff/25002/lib/i18n/intl.dart... lib/i18n/intl.dart:41: var desiredKey = howMany.toString(); indented 2 spaces, please. https://chromiumcodereview.appspot.com/10494005/diff/25002/lib/i18n/intl.dart... lib/i18n/intl.dart:53: for (var key in actions.getKeys()) { 2 spaces, please https://chromiumcodereview.appspot.com/10494005/diff/25002/lib/i18n/message_f... File lib/i18n/message_format.dart (right): https://chromiumcodereview.appspot.com/10494005/diff/25002/lib/i18n/message_f... lib/i18n/message_format.dart:27: * //@desc Lists how many people are here This syntax isn't the latest anymore. :-( https://chromiumcodereview.appspot.com/10494005/diff/25002/lib/i18n/message_f... lib/i18n/message_format.dart:60: extra whitespace. https://chromiumcodereview.appspot.com/10494005/diff/25002/lib/i18n/message_f... lib/i18n/message_format.dart:77: extra whitespace. https://chromiumcodereview.appspot.com/10494005/diff/25002/lib/i18n/message_f... lib/i18n/message_format.dart:86: 2 lines of extra whitespace.
https://chromiumcodereview.appspot.com/10494005/diff/25002/lib/i18n/date_form... File lib/i18n/date_format.dart (right): https://chromiumcodereview.appspot.com/10494005/diff/25002/lib/i18n/date_form... lib/i18n/date_format.dart:153: * Explicit constructor given a particular format On 2012/06/06 22:57:33, Emily Fortuna wrote: > comment must end in period. Done. https://chromiumcodereview.appspot.com/10494005/diff/25002/lib/i18n/intl.dart File lib/i18n/intl.dart (right): https://chromiumcodereview.appspot.com/10494005/diff/25002/lib/i18n/intl.dart... lib/i18n/intl.dart:41: var desiredKey = howMany.toString(); On 2012/06/06 22:57:33, Emily Fortuna wrote: > indented 2 spaces, please. Done. https://chromiumcodereview.appspot.com/10494005/diff/25002/lib/i18n/intl.dart... lib/i18n/intl.dart:53: for (var key in actions.getKeys()) { On 2012/06/06 22:57:33, Emily Fortuna wrote: > 2 spaces, please Done. https://chromiumcodereview.appspot.com/10494005/diff/25002/lib/i18n/message_f... File lib/i18n/message_format.dart (right): https://chromiumcodereview.appspot.com/10494005/diff/25002/lib/i18n/message_f... lib/i18n/message_format.dart:27: * //@desc Lists how many people are here On 2012/06/06 22:57:33, Emily Fortuna wrote: > This syntax isn't the latest anymore. :-( Neither is any of the rest of it. Isn't that something that should be handled via merging rather than copy-pasting from other CLs in progress? https://chromiumcodereview.appspot.com/10494005/diff/25002/lib/i18n/message_f... lib/i18n/message_format.dart:60: On 2012/06/06 22:57:33, Emily Fortuna wrote: > extra whitespace. Done. https://chromiumcodereview.appspot.com/10494005/diff/25002/lib/i18n/message_f... lib/i18n/message_format.dart:77: On 2012/06/06 22:57:33, Emily Fortuna wrote: > extra whitespace. Done. https://chromiumcodereview.appspot.com/10494005/diff/25002/lib/i18n/message_f... lib/i18n/message_format.dart:86: On 2012/06/06 22:57:33, Emily Fortuna wrote: > 2 lines of extra whitespace. Done.
lgtm! https://chromiumcodereview.appspot.com/10494005/diff/25002/lib/i18n/message_f... File lib/i18n/message_format.dart (right): https://chromiumcodereview.appspot.com/10494005/diff/25002/lib/i18n/message_f... lib/i18n/message_format.dart:27: * //@desc Lists how many people are here On 2012/06/06 23:08:09, Alan Knight wrote: > On 2012/06/06 22:57:33, Emily Fortuna wrote: > > This syntax isn't the latest anymore. :-( > > Neither is any of the rest of it. Isn't that something that should be handled > via merging rather than copy-pasting from other CLs in progress? Yes. I was expecting you to upload a merged version at some point but that's okay.
more comments from after the merge. Please address before committing. https://chromiumcodereview.appspot.com/10494005/diff/32001/lib/i18n/intl.dart File lib/i18n/intl.dart (right): https://chromiumcodereview.appspot.com/10494005/diff/32001/lib/i18n/intl.dart... lib/i18n/intl.dart:31: **/ remove extra astrisk. https://chromiumcodereview.appspot.com/10494005/diff/32001/lib/i18n/intl.dart... lib/i18n/intl.dart:32: get date() => new DateFormat.fullDate(); put return types for getters since this is an API. also, I thought we weren't returning new objects from getters? https://chromiumcodereview.appspot.com/10494005/diff/32001/lib/i18n/intl.dart... lib/i18n/intl.dart:46: return actions['other']; remove extra space here and below. https://chromiumcodereview.appspot.com/10494005/diff/32001/lib/i18n/message_f... File lib/i18n/message_format.dart (right): https://chromiumcodereview.appspot.com/10494005/diff/32001/lib/i18n/message_f... lib/i18n/message_format.dart:8: * Messages are written as functions with either one parameter (which can omitted). 80 char https://chromiumcodereview.appspot.com/10494005/diff/32001/lib/i18n/message_f... lib/i18n/message_format.dart:17: * intl_dict (dict) => "Hello ${dict['name']}, your waiting time is ${dict['minutes']} minutes"; 80 char https://chromiumcodereview.appspot.com/10494005/diff/32001/lib/i18n/message_f... lib/i18n/message_format.dart:23: * //@desc Lists how many people are here //@desc? Guess we can ignore this for now since it's going away in the next CL... https://chromiumcodereview.appspot.com/10494005/diff/32001/lib/i18n/message_f... lib/i18n/message_format.dart:25: * "$NUM other ${intl.plural(NUM, {'1':'person', 'other': 'people'})} here."; no tabs! :-( here and below! https://chromiumcodereview.appspot.com/10494005/diff/32001/lib/i18n/message_f... lib/i18n/message_format.dart:38: * String that is used to determine the particular case and gender needed to be 80 char https://chromiumcodereview.appspot.com/10494005/diff/32001/lib/i18n/message_f... lib/i18n/message_format.dart:91: */ add extra space back here. https://chromiumcodereview.appspot.com/10494005/diff/32001/lib/i18n/message_f... lib/i18n/message_format.dart:92: extra whitespace
Some things hadn't merged right. I think it should be good now (fingers crossed) https://chromiumcodereview.appspot.com/10494005/diff/32001/lib/i18n/intl.dart File lib/i18n/intl.dart (right): https://chromiumcodereview.appspot.com/10494005/diff/32001/lib/i18n/intl.dart... lib/i18n/intl.dart:31: **/ On 2012/06/07 18:42:23, Emily Fortuna wrote: > remove extra astrisk. Done. https://chromiumcodereview.appspot.com/10494005/diff/32001/lib/i18n/intl.dart... lib/i18n/intl.dart:32: get date() => new DateFormat.fullDate(); On 2012/06/07 18:42:23, Emily Fortuna wrote: > put return types for getters since this is an API. also, I thought we weren't > returning new objects from getters? Hmm, I'd made them non-getters. So that wasn't merged properly. Done. And switched back to non-getters. https://chromiumcodereview.appspot.com/10494005/diff/32001/lib/i18n/intl.dart... lib/i18n/intl.dart:46: return actions['other']; On 2012/06/07 18:42:23, Emily Fortuna wrote: > remove extra space here and below. Done. https://chromiumcodereview.appspot.com/10494005/diff/32001/lib/i18n/message_f... File lib/i18n/message_format.dart (right): https://chromiumcodereview.appspot.com/10494005/diff/32001/lib/i18n/message_f... lib/i18n/message_format.dart:8: * Messages are written as functions with either one parameter (which can omitted). On 2012/06/07 18:42:23, Emily Fortuna wrote: > 80 char Done. https://chromiumcodereview.appspot.com/10494005/diff/32001/lib/i18n/message_f... lib/i18n/message_format.dart:17: * intl_dict (dict) => "Hello ${dict['name']}, your waiting time is ${dict['minutes']} minutes"; On 2012/06/07 18:42:23, Emily Fortuna wrote: > 80 char Done. https://chromiumcodereview.appspot.com/10494005/diff/32001/lib/i18n/message_f... lib/i18n/message_format.dart:23: * //@desc Lists how many people are here On 2012/06/07 18:42:23, Emily Fortuna wrote: > //@desc? Guess we can ignore this for now since it's going away in the next > CL... Done. https://chromiumcodereview.appspot.com/10494005/diff/32001/lib/i18n/message_f... lib/i18n/message_format.dart:25: * "$NUM other ${intl.plural(NUM, {'1':'person', 'other': 'people'})} here."; On 2012/06/07 18:42:23, Emily Fortuna wrote: > no tabs! :-( here and below! Done. https://chromiumcodereview.appspot.com/10494005/diff/32001/lib/i18n/message_f... lib/i18n/message_format.dart:38: * String that is used to determine the particular case and gender needed to be On 2012/06/07 18:42:23, Emily Fortuna wrote: > 80 char Done. https://chromiumcodereview.appspot.com/10494005/diff/32001/lib/i18n/message_f... lib/i18n/message_format.dart:91: */ On 2012/06/07 18:42:23, Emily Fortuna wrote: > add extra space back here. Done. https://chromiumcodereview.appspot.com/10494005/diff/32001/lib/i18n/message_f... lib/i18n/message_format.dart:92: On 2012/06/07 18:42:23, Emily Fortuna wrote: > extra whitespace Done.
https://chromiumcodereview.appspot.com/10494005/diff/32001/lib/i18n/date_form... File lib/i18n/date_format.dart (right): https://chromiumcodereview.appspot.com/10494005/diff/32001/lib/i18n/date_form... lib/i18n/date_format.dart:75: * "h:mm a" ->> 12:08 PM Please put the formatting back for this. Markdown will read this correctly a table if it is indented 4 spaces. I have a fix that solves this in my CL. https://chromiumcodereview.appspot.com/10494005/diff/22002/lib/i18n/date_form... File lib/i18n/date_format.dart (right): https://chromiumcodereview.appspot.com/10494005/diff/22002/lib/i18n/date_form... lib/i18n/date_format.dart:114: * (TODO) alanknight This might just be a String, but it's not clear yet. Move the TODO out of the doc comments into regular line comments in this form: // TODO (alanknight): blah we probably don't want this showing up in the API documentation https://chromiumcodereview.appspot.com/10494005/diff/22002/lib/i18n/date_form... lib/i18n/date_format.dart:176: **/ remove extra asterisk, but replace with whitespace. https://chromiumcodereview.appspot.com/10494005/diff/22002/lib/i18n/date_form... lib/i18n/date_format.dart:177: remove extra whitespace line. https://chromiumcodereview.appspot.com/10494005/diff/22002/lib/i18n/intl.dart File lib/i18n/intl.dart (right): https://chromiumcodereview.appspot.com/10494005/diff/22002/lib/i18n/intl.dart... lib/i18n/intl.dart:20: * just a skeleton right now. move TODO out of doc comments. https://chromiumcodereview.appspot.com/10494005/diff/22002/lib/i18n/intl.dart... lib/i18n/intl.dart:31: */ add one more space so that this asterisk lines up with the one above. https://chromiumcodereview.appspot.com/10494005/diff/22002/lib/i18n/intl.dart... lib/i18n/intl.dart:40: String plural(num howMany, Map actions) { Now string is indented 3 spaces. Add back the spacing here, please. It was just the return statements that we mis-indented. Can we set up some setting on your editor so that it automatically indents correctly with the two spaces? I've set up my .vimrc to do this, and I'm certain sure others have this set up in other IDEs. https://chromiumcodereview.appspot.com/10494005/diff/22002/lib/i18n/message_f... File lib/i18n/message_format.dart (right): https://chromiumcodereview.appspot.com/10494005/diff/22002/lib/i18n/message_f... lib/i18n/message_format.dart:53: remove extra whitespace https://chromiumcodereview.appspot.com/10494005/diff/22002/lib/i18n/message_f... lib/i18n/message_format.dart:68: const MessageFormat([this._messageFunction, this._locale = 'en-US']); why is messageFunction optional? don't we want to require that argument? https://chromiumcodereview.appspot.com/10494005/diff/22002/lib/i18n/message_f... lib/i18n/message_format.dart:79: */ add space back in please https://chromiumcodereview.appspot.com/10494005/diff/22002/lib/i18n/message_f... lib/i18n/message_format.dart:95: */ add one more space please https://chromiumcodereview.appspot.com/10494005/diff/22002/lib/i18n/message_f... lib/i18n/message_format.dart:98: return messageFunction(); there are three spaces indented here and it should be 2. https://chromiumcodereview.appspot.com/10494005/diff/22002/lib/i18n/message_f... lib/i18n/message_format.dart:100: return messageFunction(messageParameters); same here. https://chromiumcodereview.appspot.com/10494005/diff/22002/tests/lib/i18n/mes... File tests/lib/i18n/message_format_test.dart (right): https://chromiumcodereview.appspot.com/10494005/diff/22002/tests/lib/i18n/mes... tests/lib/i18n/message_format_test.dart:29: remove extra line of whitespace https://chromiumcodereview.appspot.com/10494005/diff/22002/tests/lib/i18n/mes... tests/lib/i18n/message_format_test.dart:52: extra whitespace. https://chromiumcodereview.appspot.com/10494005/diff/22002/tests/lib/i18n/mes... tests/lib/i18n/message_format_test.dart:72: whitespace
https://chromiumcodereview.appspot.com/10494005/diff/32001/lib/i18n/date_form... File lib/i18n/date_format.dart (right): https://chromiumcodereview.appspot.com/10494005/diff/32001/lib/i18n/date_form... lib/i18n/date_format.dart:75: * "h:mm a" ->> 12:08 PM On 2012/06/07 21:33:36, Emily Fortuna wrote: > Please put the formatting back for this. Markdown will read this correctly a > table if it is indented 4 spaces. I have a fix that solves this in my CL. Done. https://chromiumcodereview.appspot.com/10494005/diff/22002/lib/i18n/date_form... File lib/i18n/date_format.dart (right): https://chromiumcodereview.appspot.com/10494005/diff/22002/lib/i18n/date_form... lib/i18n/date_format.dart:114: * (TODO) alanknight This might just be a String, but it's not clear yet. On 2012/06/07 21:33:36, Emily Fortuna wrote: > Move the TODO out of the doc comments into regular line comments in this form: > // TODO (alanknight): blah > we probably don't want this showing up in the API documentation Done. https://chromiumcodereview.appspot.com/10494005/diff/22002/lib/i18n/date_form... lib/i18n/date_format.dart:176: **/ On 2012/06/07 21:33:36, Emily Fortuna wrote: > remove extra asterisk, but replace with whitespace. Done. https://chromiumcodereview.appspot.com/10494005/diff/22002/lib/i18n/date_form... lib/i18n/date_format.dart:177: On 2012/06/07 21:33:36, Emily Fortuna wrote: > remove extra whitespace line. Done. https://chromiumcodereview.appspot.com/10494005/diff/22002/lib/i18n/intl.dart File lib/i18n/intl.dart (right): https://chromiumcodereview.appspot.com/10494005/diff/22002/lib/i18n/intl.dart... lib/i18n/intl.dart:20: * just a skeleton right now. On 2012/06/07 21:33:36, Emily Fortuna wrote: > move TODO out of doc comments. Done. https://chromiumcodereview.appspot.com/10494005/diff/22002/lib/i18n/intl.dart... lib/i18n/intl.dart:31: */ On 2012/06/07 21:33:36, Emily Fortuna wrote: > add one more space so that this asterisk lines up with the one above. Done. https://chromiumcodereview.appspot.com/10494005/diff/22002/lib/i18n/intl.dart... lib/i18n/intl.dart:40: String plural(num howMany, Map actions) { On 2012/06/07 21:33:36, Emily Fortuna wrote: > Now string is indented 3 spaces. Add back the spacing here, please. It was just > the return statements that we mis-indented. Can we set up some setting on your > editor so that it automatically indents correctly with the two spaces? I've set > up my .vimrc to do this, and I'm certain sure others have this set up in other > IDEs. String looks correctly indented to me. It looks like the comments above weren't. I did set up the emacs mode now and that seems to think it knows what to do. Hopefully it and the Dart editor will do things correctly going forward. Right now, fixed the comments and the not-getters-anymore up above. https://chromiumcodereview.appspot.com/10494005/diff/22002/lib/i18n/message_f... File lib/i18n/message_format.dart (right): https://chromiumcodereview.appspot.com/10494005/diff/22002/lib/i18n/message_f... lib/i18n/message_format.dart:53: On 2012/06/07 21:33:36, Emily Fortuna wrote: > remove extra whitespace Done. https://chromiumcodereview.appspot.com/10494005/diff/22002/lib/i18n/message_f... lib/i18n/message_format.dart:68: const MessageFormat([this._messageFunction, this._locale = 'en-US']); On 2012/06/07 21:33:36, Emily Fortuna wrote: > why is messageFunction optional? don't we want to require that argument? We have a version of format below which can take a messageFunction as a paramter, which suggests you can say (new MessageFormat()).format(thing) And if you ask intl.message() it definitely doesn't know what message to give it. https://chromiumcodereview.appspot.com/10494005/diff/22002/lib/i18n/message_f... lib/i18n/message_format.dart:79: */ On 2012/06/07 21:33:36, Emily Fortuna wrote: > add space back in please Done. https://chromiumcodereview.appspot.com/10494005/diff/22002/lib/i18n/message_f... lib/i18n/message_format.dart:95: */ On 2012/06/07 21:33:36, Emily Fortuna wrote: > add one more space please Done. https://chromiumcodereview.appspot.com/10494005/diff/22002/lib/i18n/message_f... lib/i18n/message_format.dart:98: return messageFunction(); On 2012/06/07 21:33:36, Emily Fortuna wrote: > there are three spaces indented here and it should be 2. Done. https://chromiumcodereview.appspot.com/10494005/diff/22002/lib/i18n/message_f... lib/i18n/message_format.dart:100: return messageFunction(messageParameters); On 2012/06/07 21:33:36, Emily Fortuna wrote: > same here. Done. https://chromiumcodereview.appspot.com/10494005/diff/22002/tests/lib/i18n/mes... File tests/lib/i18n/message_format_test.dart (right): https://chromiumcodereview.appspot.com/10494005/diff/22002/tests/lib/i18n/mes... tests/lib/i18n/message_format_test.dart:29: On 2012/06/07 21:33:36, Emily Fortuna wrote: > remove extra line of whitespace Done. https://chromiumcodereview.appspot.com/10494005/diff/22002/tests/lib/i18n/mes... tests/lib/i18n/message_format_test.dart:52: On 2012/06/07 21:33:36, Emily Fortuna wrote: > extra whitespace. Done. https://chromiumcodereview.appspot.com/10494005/diff/22002/tests/lib/i18n/mes... tests/lib/i18n/message_format_test.dart:72: On 2012/06/07 21:33:36, Emily Fortuna wrote: > whitespace Done.
whew. lgtm |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
