Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10807096:
Add date formatting and parsing code. (Closed)
|
Created:
8 years, 5 months ago by Alan Knight Modified:
8 years, 4 months ago Reviewers:
Emily Fortuna CC:
reviews_dartlang.org Visibility:
Public. |
DescriptionAdd date formatting and parsing code.
Committed: https://code.google.com/p/dart/source/detail?r=10419
Patch Set 1 #Patch Set 2 : #
Total comments: 160
Patch Set 3 : #
Total comments: 108
Patch Set 4 : #
Total comments: 17
Patch Set 5 : #Patch Set 6 : #Patch Set 7 : #
Messages
Total messages: 9 (0 generated)
some starting comments. In general, I appreciate doing more incremental code reviews, so we can work on the direction of the code before you spend too much time implementing. https://chromiumcodereview.appspot.com/10807096/diff/2001/lib/i18n/date_forma... File lib/i18n/date_format.dart (left): https://chromiumcodereview.appspot.com/10807096/diff/2001/lib/i18n/date_forma... lib/i18n/date_format.dart:74: * "yyyy.MM.dd G 'at' HH:mm:ss vvvv"->> 1996.07.10 AD at 15:08:56 Pacific Time When was the last time you sync'd? This was fixed a month ago. https://chromiumcodereview.appspot.com/10807096/diff/2001/lib/i18n/date_forma... File lib/i18n/date_format.dart (right): https://chromiumcodereview.appspot.com/10807096/diff/2001/lib/i18n/date_forma... lib/i18n/date_format.dart:105: * Items marked with '#' work differently than in Java. Now that this is implemented, does it in fact work differently than Java? If so, in what way? https://chromiumcodereview.appspot.com/10807096/diff/2001/lib/i18n/date_forma... lib/i18n/date_format.dart:177: * The constructor accepts a [formatDefinition] which is a String. If the 1) No need to say formatDefinition is a string, just type it in the declaration. 2) It doesn't look like formatDefinition exists in this file anymore. Please update the documentation here. https://chromiumcodereview.appspot.com/10807096/diff/2001/lib/i18n/date_forma... lib/i18n/date_format.dart:193: //TODO(alanknight): It should be possible to specify multiple skeletons, e.g indent these comments two more spaces. Also, add a space between the "//" and "TODO". See google style guide. https://chromiumcodereview.appspot.com/10807096/diff/2001/lib/i18n/date_forma... lib/i18n/date_format.dart:218: * Returns a date string indicating how long ago (3 hours, 2 minutes) remove the extra whitespace here. comments should be: /** * foo */ https://chromiumcodereview.appspot.com/10807096/diff/2001/lib/i18n/date_forma... lib/i18n/date_format.dart:242: _formatFields.forEach( indentation?? https://chromiumcodereview.appspot.com/10807096/diff/2001/lib/i18n/date_forma... lib/i18n/date_format.dart:243: (each) { this looks like a great case for (each) => each.parse(stream, dateFields) https://chromiumcodereview.appspot.com/10807096/diff/2001/lib/i18n/date_forma... lib/i18n/date_format.dart:252: * format, using UTC. The inputString is in UTC, or we parse the result into UTC? https://chromiumcodereview.appspot.com/10807096/diff/2001/lib/i18n/date_forma... lib/i18n/date_format.dart:265: return _locale;} move } to new line. https://chromiumcodereview.appspot.com/10807096/diff/2001/lib/i18n/date_forma... lib/i18n/date_format.dart:296: this(YEAR_NUM_MONTH_WEEKDAY_DAY, locale); if you have to break a line, the next line should be indented 4 spaces. (here and throughout) https://chromiumcodereview.appspot.com/10807096/diff/2001/lib/i18n/date_forma... lib/i18n/date_format.dart:368: static final String DAY = 'd'; Perhaps I'm missing something here. Why doesn't this collide with the named constructor DateFormat.DAY()? https://chromiumcodereview.appspot.com/10807096/diff/2001/lib/i18n/date_forma... lib/i18n/date_format.dart:418: String _pattern; -1 space indentation. https://chromiumcodereview.appspot.com/10807096/diff/2001/lib/i18n/date_forma... lib/i18n/date_format.dart:432: //TODO(alanknight): This is an expensive operation. Caching recently used this never gets used/tested? Why provide this function? Why not just use the various constructors? https://chromiumcodereview.appspot.com/10807096/diff/2001/lib/i18n/date_forma... lib/i18n/date_format.dart:447: Map get availableSkeletons() { do we want this to be a public function? or is this more of just a private helper? https://chromiumcodereview.appspot.com/10807096/diff/2001/lib/i18n/date_forma... lib/i18n/date_format.dart:452: * Return true if the locale exists, or if it is null. The null case seems like all of this locale code is not unique to date_format. Move it to it's own class or into the intl object? https://chromiumcodereview.appspot.com/10807096/diff/2001/lib/i18n/date_forma... lib/i18n/date_format.dart:455: bool _localeExists(localeName) { -1 space indentation. Please check that your code is properly formatted prior to submitting for review. I'm a very expensive linter. https://chromiumcodereview.appspot.com/10807096/diff/2001/lib/i18n/date_forma... lib/i18n/date_format.dart:469: // i18n comments are a complete sentence and end in "." Please follow google style guide. Also, yes, please move this into Intl or elsewhere https://chromiumcodereview.appspot.com/10807096/diff/2001/lib/i18n/date_forma... lib/i18n/date_format.dart:475: return _locale = each; don't combine assignment with return values. It's not as clear as could be. https://chromiumcodereview.appspot.com/10807096/diff/2001/lib/i18n/date_forma... lib/i18n/date_format.dart:483: if (aLocale.length < 2) return aLocale; are there locales that are just one letter? If not we can get rid of this line. https://chromiumcodereview.appspot.com/10807096/diff/2001/lib/i18n/date_forma... lib/i18n/date_format.dart:492: if (aLocale.length != 5) return aLocale; what other acceptable locales are there? if it's just two letters like "en" (are there one letter locales?), then seems like we could just test for the presence of '-' or '_'. If there are one letter locales, then what if the locale is e-fr (in which case we still need to do canonicalization) https://chromiumcodereview.appspot.com/10807096/diff/2001/lib/i18n/date_forma... lib/i18n/date_format.dart:503: // Quoted String an example for this? https://chromiumcodereview.appspot.com/10807096/diff/2001/lib/i18n/date_forma... lib/i18n/date_format.dart:520: if (pattern.isEmpty()) return [ ]; [] https://chromiumcodereview.appspot.com/10807096/diff/2001/lib/i18n/date_forma... lib/i18n/date_format.dart:545: /** Polyfill for missing library function. */ is there a bug filed /feature request? If so, please add bug number https://chromiumcodereview.appspot.com/10807096/diff/2001/lib/i18n/date_forma... lib/i18n/date_format.dart:562: //TODO(alanknight): Is including these all in the same file as private the best source seems more readable than a really long file. I'd support that instead https://chromiumcodereview.appspot.com/10807096/diff/2001/lib/i18n/date_forma... lib/i18n/date_format.dart:567: static List<String> _matchTypes = const ['QUOTED_STRING', 'FIELD', 'LITERAL']; Use an enum pattern here. See https://groups.google.com/a/dartlang.org/d/topic/misc/MOEK5Q0VcKg/discussion for an example (there are better examples, this was just the first I could find) https://chromiumcodereview.appspot.com/10807096/diff/2001/lib/i18n/date_forma... lib/i18n/date_format.dart:570: String string; can you give this variable a different name? https://chromiumcodereview.appspot.com/10807096/diff/2001/lib/i18n/date_forma... lib/i18n/date_format.dart:579: * Return the width of our string field. Different widths represent different [string] https://chromiumcodereview.appspot.com/10807096/diff/2001/lib/i18n/date_forma... lib/i18n/date_format.dart:580: * formatting options. See the comment for DateFormat for details. where is this comment for DateFormat? https://chromiumcodereview.appspot.com/10807096/diff/2001/lib/i18n/date_forma... lib/i18n/date_format.dart:584: _DateFormatField(this.string, this.type,this.parent); space after , https://chromiumcodereview.appspot.com/10807096/diff/2001/lib/i18n/date_forma... lib/i18n/date_format.dart:594: var someRegex = new RegExp("\'\'"); @ before a string also can denote a raw string https://chromiumcodereview.appspot.com/10807096/diff/2001/lib/i18n/date_forma... lib/i18n/date_format.dart:888: return 'not implemented'; throw NotImplementedException https://chromiumcodereview.appspot.com/10807096/diff/2001/lib/i18n/date_forma... lib/i18n/date_format.dart:911: /** A class for holding onto the data for a date so that it can be built separate line /** https://chromiumcodereview.appspot.com/10807096/diff/2001/lib/i18n/date_forma... lib/i18n/date_format.dart:926: void setYear(x) {year = x;} void setYear(x) => year = x or void setYear(x) { year = x; } https://chromiumcodereview.appspot.com/10807096/diff/2001/lib/i18n/date_forma... lib/i18n/date_format.dart:939: if (year == 0) { if year !=0 but month or day == 0? https://chromiumcodereview.appspot.com/10807096/diff/2001/lib/i18n/date_forma... lib/i18n/date_format.dart:972: /** Return the next [howMany] items, or as many as there are remaining. /** on own line https://chromiumcodereview.appspot.com/10807096/diff/2001/lib/i18n/date_forma... lib/i18n/date_format.dart:1002: int findIndex(Function f) { This also has the side effect of modifying index/advancing to that element, which you should probably mention. https://chromiumcodereview.appspot.com/10807096/diff/2001/lib/i18n/date_forma... lib/i18n/date_format.dart:1026: digits.add(next().charCodeAt(0)); this could be done faster with a regular expression... https://chromiumcodereview.appspot.com/10807096/diff/2001/lib/i18n/date_forma... lib/i18n/date_format.dart:1043: whitespace https://chromiumcodereview.appspot.com/10807096/diff/2001/lib/i18n/date_symbo... File lib/i18n/date_symbol_data.dart (right): https://chromiumcodereview.appspot.com/10807096/diff/2001/lib/i18n/date_symbo... lib/i18n/date_symbol_data.dart:5: /** Data for the strings used in a particular locale for date/time constants, why do we have two doc comment sections, one after the other? https://chromiumcodereview.appspot.com/10807096/diff/2001/lib/i18n/date_symbo... lib/i18n/date_symbol_data.dart:13: * http://go/generate_datetime_constants.py using the --for_dart flag. - 'http://' Also, I'd like to see a CL of the changes to this script https://chromiumcodereview.appspot.com/10807096/diff/2001/lib/i18n/date_symbo... lib/i18n/date_symbol_data.dart:25: whitespace https://chromiumcodereview.appspot.com/10807096/diff/2001/lib/i18n/date_symbo... lib/i18n/date_symbol_data.dart:30: NAME: 'en_ISO', indentation https://chromiumcodereview.appspot.com/10807096/diff/2001/lib/i18n/date_symbo... lib/i18n/date_symbol_data.dart:34: 'N', 'D'], indentation! https://chromiumcodereview.appspot.com/10807096/diff/2001/lib/i18n/date_symbo... lib/i18n/date_symbol_data.dart:71: NAME: "af", more indentation here... https://chromiumcodereview.appspot.com/10807096/diff/2001/lib/i18n/date_symbo... lib/i18n/date_symbol_data.dart:3620: whitespace! https://chromiumcodereview.appspot.com/10807096/diff/2001/lib/i18n/date_symbo... File lib/i18n/date_symbols.dart (right): https://chromiumcodereview.appspot.com/10807096/diff/2001/lib/i18n/date_symbo... lib/i18n/date_symbols.dart:4: * This holds onto information about how a particular locale formats dates. It this this file generated? If so, please say so in the comments. Same goes for all the other files. https://chromiumcodereview.appspot.com/10807096/diff/2001/lib/i18n/date_symbo... lib/i18n/date_symbols.dart:15: STANDALONEMONTHS, SHORTMONTHS,STANDALONESHORTMONTHS,WEEKDAYS, indent 2 more spaces here if the line is broken. Also, spacing between the commas here is irregular. Also, use underscores to separate words (NARROW_MONTHS)? https://chromiumcodereview.appspot.com/10807096/diff/2001/lib/i18n/date_symbo... lib/i18n/date_symbols.dart:48: ); move this up to the above line. https://chromiumcodereview.appspot.com/10807096/diff/2001/lib/i18n/date_time_... File lib/i18n/date_time_patterns.dart (right): https://chromiumcodereview.appspot.com/10807096/diff/2001/lib/i18n/date_time_... lib/i18n/date_time_patterns.dart:5: extra whitespace https://chromiumcodereview.appspot.com/10807096/diff/2001/lib/i18n/date_time_... lib/i18n/date_time_patterns.dart:10: * generate_datetime_pattern_dart.cc (Google internal) Add "DO NOT EDIT" notice at top to make the autogeneration more obvious. Also, I'd like to see how this script was modified. https://chromiumcodereview.appspot.com/10807096/diff/2001/lib/i18n/date_time_... lib/i18n/date_time_patterns.dart:20: 'd': 'd', // DAY indentation... https://chromiumcodereview.appspot.com/10807096/diff/2001/lib/i18n/date_time_... lib/i18n/date_time_patterns.dart:3966: extra whitespace https://chromiumcodereview.appspot.com/10807096/diff/2001/lib/i18n/intl.dart File lib/i18n/intl.dart (right): https://chromiumcodereview.appspot.com/10807096/diff/2001/lib/i18n/intl.dart#... lib/i18n/intl.dart:27: * will be set later, otherwise the format is not useful. why make pattern optional? https://chromiumcodereview.appspot.com/10807096/diff/2001/lib/i18n/intl.dart#... lib/i18n/intl.dart:28: * If [locale] is not specified, then we default to the locale specified desiredLocale https://chromiumcodereview.appspot.com/10807096/diff/2001/lib/i18n/intl.dart#... lib/i18n/intl.dart:29: * in this instance. Note that this is different from using the static "locale specified in this instance" please clarify. https://chromiumcodereview.appspot.com/10807096/diff/2001/lib/i18n/intl.dart#... lib/i18n/intl.dart:30: * version of this, in which case we will default to the overall default please rephrase this last sentence. "this" is a bit unclear unless you re-read the sentence carefully. https://chromiumcodereview.appspot.com/10807096/diff/2001/lib/i18n/intl.dart#... lib/i18n/intl.dart:47: static DateFormat Date([pattern, desiredLocale]) { types for parameters https://chromiumcodereview.appspot.com/10807096/diff/2001/lib/i18n/intl.dart#... lib/i18n/intl.dart:50: // to do this both statically and on an instance. if you're going to do this statically the user might as well just create a Date object explicitly. Let's provide one way to use the Intl object, and you have to construct the DateFormatting objects explicitly if you want to do "on the fly" locale detection versus a specific locale. https://chromiumcodereview.appspot.com/10807096/diff/2001/lib/i18n/intl.dart#... lib/i18n/intl.dart:95: if (_locale == null) { please sync your files before submitting for review. This mechanism is built into the constructor in the current version in the repo. https://chromiumcodereview.appspot.com/10807096/diff/2001/tests/lib/i18n/date... File tests/lib/i18n/date_time_format_test.dart (left): https://chromiumcodereview.appspot.com/10807096/diff/2001/tests/lib/i18n/date... tests/lib/i18n/date_time_format_test.dart:8: keep this in. https://chromiumcodereview.appspot.com/10807096/diff/2001/tests/lib/i18n/date... File tests/lib/i18n/date_time_format_test.dart (right): https://chromiumcodereview.appspot.com/10807096/diff/2001/tests/lib/i18n/date... tests/lib/i18n/date_time_format_test.dart:20: const [ move this up to the previous line https://chromiumcodereview.appspot.com/10807096/diff/2001/tests/lib/i18n/date... tests/lib/i18n/date_time_format_test.dart:47: //TODO(alanknight): CLDR and ICU appear to disagree on these for Japanese indentation https://chromiumcodereview.appspot.com/10807096/diff/2001/tests/lib/i18n/date... tests/lib/i18n/date_time_format_test.dart:128: extra whitespace https://chromiumcodereview.appspot.com/10807096/diff/2001/tests/lib/i18n/date... tests/lib/i18n/date_time_format_test.dart:130: var date_format = new DateFormat(); indentation. please! https://chromiumcodereview.appspot.com/10807096/diff/2001/tests/lib/i18n/date... tests/lib/i18n/date_time_format_test.dart:133: (date_format.parsePattern("hh:mm:ss")).map((x)=>x.string), extra paren here starting https://chromiumcodereview.appspot.com/10807096/diff/2001/tests/lib/i18n/date... tests/lib/i18n/date_time_format_test.dart:140: orderedEquals(types)); test some other cases for parsing? https://chromiumcodereview.appspot.com/10807096/diff/2001/tests/lib/i18n/date... tests/lib/i18n/date_time_format_test.dart:244: * locale. Expects to be given a Map from ICU constant names to the info about the parameters? nit: Can this function be up at the top before it's used? https://chromiumcodereview.appspot.com/10807096/diff/2001/tests/lib/i18n/date... tests/lib/i18n/date_time_format_test.dart:249: for(int i=0; i<formatsToTest.length;i++) { spacing. i < formatsToTest.length; i++ here and other places https://chromiumcodereview.appspot.com/10807096/diff/2001/tests/lib/i18n/date... tests/lib/i18n/date_time_format_test.dart:254: // print("Expect: ${expectedResults[formatName]} from format $formatName " cut commented out code :-/ https://chromiumcodereview.appspot.com/10807096/diff/2001/tests/lib/i18n/date... tests/lib/i18n/date_time_format_test.dart:259: expect(expectedResults[icuName],equals(actualResult)); spacing after , https://chromiumcodereview.appspot.com/10807096/diff/2001/tests/lib/i18n/date... tests/lib/i18n/date_time_format_test.dart:281: var format = new DateFormat(skeleton,localeName); space after , https://chromiumcodereview.appspot.com/10807096/diff/2001/tests/lib/i18n/date... tests/lib/i18n/date_time_format_test.dart:284: if (!badPatterns.some((x) => x == format.pattern)) { ? https://chromiumcodereview.appspot.com/10807096/diff/2001/tests/lib/i18n/date... tests/lib/i18n/date_time_format_test.dart:288: expect(thenPrintAgain,equals(actualResult)); space after , . https://chromiumcodereview.appspot.com/10807096/diff/2001/tests/lib/i18n/date... File tests/lib/i18n/date_time_format_test_data.dart (right): https://chromiumcodereview.appspot.com/10807096/diff/2001/tests/lib/i18n/date... tests/lib/i18n/date_time_format_test_data.dart:1: var English = const { copyright notice? some comment explanation of what this file is? an explanation of why this set of locales? https://chromiumcodereview.appspot.com/10807096/diff/2001/tests/lib/i18n/date... tests/lib/i18n/date_time_format_test_data.dart:2: "DAY" : "27", indentation https://chromiumcodereview.appspot.com/10807096/diff/2001/tests/lib/i18n/date... tests/lib/i18n/date_time_format_test_data.dart:46: "YEAR_MONTH_WEEKDAY_DAY + HOUR_MINUTE_SECOND + GENERIC_TZ" : "Friday, January 27, 2012 8:58:59 PM Pacific Time", 80 char https://chromiumcodereview.appspot.com/10807096/diff/2001/tests/lib/i18n/date... tests/lib/i18n/date_time_format_test_data.dart:100: extra whitespace https://chromiumcodereview.appspot.com/10807096/diff/2001/tests/lib/i18n/date... tests/lib/i18n/date_time_format_test_data.dart:301: extra whitespace https://chromiumcodereview.appspot.com/10807096/diff/2001/tests/lib/i18n/intl... File tests/lib/i18n/intl_message_test.dart (right): https://chromiumcodereview.appspot.com/10807096/diff/2001/tests/lib/i18n/intl... tests/lib/i18n/intl_message_test.dart:82: expect(de.locale,equals('de_DE')); space after , why put this in intl_message_test?
New version, and a CL for the generating programs. https://chromiumcodereview.appspot.com/10807096/diff/2001/lib/i18n/date_forma... File lib/i18n/date_format.dart (left): https://chromiumcodereview.appspot.com/10807096/diff/2001/lib/i18n/date_forma... lib/i18n/date_format.dart:74: * "yyyy.MM.dd G 'at' HH:mm:ss vvvv"->> 1996.07.10 AD at 15:08:56 Pacific Time On 2012/07/31 05:52:53, Emily Fortuna wrote: > When was the last time you sync'd? This was fixed a month ago. Yes, it was before you left. https://chromiumcodereview.appspot.com/10807096/diff/2001/lib/i18n/date_forma... File lib/i18n/date_format.dart (right): https://chromiumcodereview.appspot.com/10807096/diff/2001/lib/i18n/date_forma... lib/i18n/date_format.dart:105: * Items marked with '#' work differently than in Java. On 2012/07/31 05:52:53, Emily Fortuna wrote: > Now that this is implemented, does it in fact work differently than Java? If so, > in what way? Good question. Shanjian believes this refers to issues around printing with a year of zero. I removed the qualifier and put in handling of years <= 0 as a TODO. https://chromiumcodereview.appspot.com/10807096/diff/2001/lib/i18n/date_forma... lib/i18n/date_format.dart:177: * The constructor accepts a [formatDefinition] which is a String. If the On 2012/07/31 05:52:53, Emily Fortuna wrote: > 1) No need to say formatDefinition is a string, just type it in the declaration. > 2) It doesn't look like formatDefinition exists in this file anymore. Please > update the documentation here. Done. https://chromiumcodereview.appspot.com/10807096/diff/2001/lib/i18n/date_forma... lib/i18n/date_format.dart:193: //TODO(alanknight): It should be possible to specify multiple skeletons, e.g On 2012/07/31 05:52:53, Emily Fortuna wrote: > indent these comments two more spaces. Also, add a space between the "//" and > "TODO". See google style guide. Done, along with other occurrences. https://chromiumcodereview.appspot.com/10807096/diff/2001/lib/i18n/date_forma... lib/i18n/date_format.dart:218: * Returns a date string indicating how long ago (3 hours, 2 minutes) On 2012/07/31 05:52:53, Emily Fortuna wrote: > remove the extra whitespace here. comments should be: > /** > * foo > */ Done. https://chromiumcodereview.appspot.com/10807096/diff/2001/lib/i18n/date_forma... lib/i18n/date_format.dart:242: _formatFields.forEach( On 2012/07/31 05:52:53, Emily Fortuna wrote: > indentation?? Done. https://chromiumcodereview.appspot.com/10807096/diff/2001/lib/i18n/date_forma... lib/i18n/date_format.dart:243: (each) { On 2012/07/31 05:52:53, Emily Fortuna wrote: > this looks like a great case for (each) => each.parse(stream, dateFields) Done. https://chromiumcodereview.appspot.com/10807096/diff/2001/lib/i18n/date_forma... lib/i18n/date_format.dart:252: * format, using UTC. On 2012/07/31 05:52:53, Emily Fortuna wrote: > The inputString is in UTC, or we parse the result into UTC? That is rather ambiguous. Changed it so that it's doing what seems sensible, reading the string as being in UTC, and changed the comment. https://chromiumcodereview.appspot.com/10807096/diff/2001/lib/i18n/date_forma... lib/i18n/date_format.dart:265: return _locale;} On 2012/07/31 05:52:53, Emily Fortuna wrote: > move } to new line. Done. https://chromiumcodereview.appspot.com/10807096/diff/2001/lib/i18n/date_forma... lib/i18n/date_format.dart:296: this(YEAR_NUM_MONTH_WEEKDAY_DAY, locale); On 2012/07/31 05:52:53, Emily Fortuna wrote: > if you have to break a line, the next line should be indented 4 spaces. (here > and throughout) Done. https://chromiumcodereview.appspot.com/10807096/diff/2001/lib/i18n/date_forma... lib/i18n/date_format.dart:368: static final String DAY = 'd'; On 2012/07/31 05:52:53, Emily Fortuna wrote: > Perhaps I'm missing something here. Why doesn't this collide with the named > constructor DateFormat.DAY()? An interesting point. But apparently it doesn't. Constructors seem to get their own namespace. Do you think we should change it to avoid possible confusion? https://chromiumcodereview.appspot.com/10807096/diff/2001/lib/i18n/date_forma... lib/i18n/date_format.dart:418: String _pattern; On 2012/07/31 05:52:53, Emily Fortuna wrote: > -1 space indentation. Done. https://chromiumcodereview.appspot.com/10807096/diff/2001/lib/i18n/date_forma... lib/i18n/date_format.dart:432: //TODO(alanknight): This is an expensive operation. Caching recently used On 2012/07/31 05:52:53, Emily Fortuna wrote: > this never gets used/tested? Why provide this function? Why not just use the > various constructors? The constructor calls this. It's also possible, and arguably reasonable, to do something like var x = intl.date(); x.pattern('jms'); and one of the tests does that. I wouldn't object very strongly to making this private so that only the constructors can use it. https://chromiumcodereview.appspot.com/10807096/diff/2001/lib/i18n/date_forma... lib/i18n/date_format.dart:447: Map get availableSkeletons() { On 2012/07/31 05:52:53, Emily Fortuna wrote: > do we want this to be a public function? or is this more of just a private > helper? Changed to private. I had a lot more private, but ran into the difficulty of testing it, so ended up making more things public. This one seems like it definitely should be private. https://chromiumcodereview.appspot.com/10807096/diff/2001/lib/i18n/date_forma... lib/i18n/date_format.dart:452: * Return true if the locale exists, or if it is null. The null case On 2012/07/31 05:52:53, Emily Fortuna wrote: > seems like all of this locale code is not unique to date_format. Move it to it's > own class or into the intl object? As per this and later comment, moved into Intl as static methods. https://chromiumcodereview.appspot.com/10807096/diff/2001/lib/i18n/date_forma... lib/i18n/date_format.dart:455: bool _localeExists(localeName) { On 2012/07/31 05:52:53, Emily Fortuna wrote: > -1 space indentation. Please check that your code is properly formatted prior to > submitting for review. I'm a very expensive linter. Sorry, I did try to find formatting issues, but am both expensive and error-prone as a linter. Is there any kind of automated mechanism? https://chromiumcodereview.appspot.com/10807096/diff/2001/lib/i18n/date_forma... lib/i18n/date_format.dart:475: return _locale = each; On 2012/07/31 05:52:53, Emily Fortuna wrote: > don't combine assignment with return values. It's not as clear as could be. Done. https://chromiumcodereview.appspot.com/10807096/diff/2001/lib/i18n/date_forma... lib/i18n/date_format.dart:483: if (aLocale.length < 2) return aLocale; On 2012/07/31 05:52:53, Emily Fortuna wrote: > are there locales that are just one letter? If not we can get rid of this line. Locales should be at least two letters. This deals with the case where the user gives us a string that's too short. Rather than an exception in the substring call we'll just return the locale, which then gets checked for existence, and presumably fails. https://chromiumcodereview.appspot.com/10807096/diff/2001/lib/i18n/date_forma... lib/i18n/date_format.dart:492: if (aLocale.length != 5) return aLocale; On 2012/07/31 05:52:53, Emily Fortuna wrote: > what other acceptable locales are there? > if it's just two letters like "en" (are there one letter locales?), then seems > like we could just test for the presence of '-' or '_'. If there are one letter > locales, then what if the locale is e-fr (in which case we still need to do > canonicalization) Locales should be of the form either 'en' or 'en_US', with the apparent exception of 'en_ISO' which is manually added to the generated list. There are also some things that use hyphens instead of underscores. Closure also accepts lowercase countries. More recent things allow passing longer strings that indicate what locale to use for particular aspects, e.g. English locale, but with Portugese date formats and German phone book collation. But we're not handling those right now. What are you suggesting to do here? We could test for the presence anywhere in the string of '-' or '_'. That's probably slower than testing length first and looking in the known required position, but might be shorter to write. https://chromiumcodereview.appspot.com/10807096/diff/2001/lib/i18n/date_forma... lib/i18n/date_format.dart:503: // Quoted String On 2012/07/31 05:52:53, Emily Fortuna wrote: > an example for this? Done. https://chromiumcodereview.appspot.com/10807096/diff/2001/lib/i18n/date_forma... lib/i18n/date_format.dart:520: if (pattern.isEmpty()) return [ ]; On 2012/07/31 05:52:53, Emily Fortuna wrote: > [] Done. https://chromiumcodereview.appspot.com/10807096/diff/2001/lib/i18n/date_forma... lib/i18n/date_format.dart:545: /** Polyfill for missing library function. */ On 2012/07/31 05:52:53, Emily Fortuna wrote: > is there a bug filed /feature request? If so, please add bug number Done. https://chromiumcodereview.appspot.com/10807096/diff/2001/lib/i18n/date_forma... lib/i18n/date_format.dart:562: //TODO(alanknight): Is including these all in the same file as private the best On 2012/07/31 05:52:53, Emily Fortuna wrote: > source seems more readable than a really long file. I'd support that instead OK, split out into two additional files, one for the format fields, and one for smaller classes (dateBuilder, stream, exception). I can go all the way down to one per class if you think that's best. https://chromiumcodereview.appspot.com/10807096/diff/2001/lib/i18n/date_forma... lib/i18n/date_format.dart:567: static List<String> _matchTypes = const ['QUOTED_STRING', 'FIELD', 'LITERAL']; On 2012/07/31 05:52:53, Emily Fortuna wrote: > Use an enum pattern here. See > https://groups.google.com/a/dartlang.org/d/topic/misc/MOEK5Q0VcKg/discussion for > an example (there are better examples, this was just the first I could find) Rather than an enum, split the field class into three with an abstract superclass, which also simplifies the code. https://chromiumcodereview.appspot.com/10807096/diff/2001/lib/i18n/date_forma... lib/i18n/date_format.dart:570: String string; On 2012/07/31 05:52:53, Emily Fortuna wrote: > can you give this variable a different name? Done. https://chromiumcodereview.appspot.com/10807096/diff/2001/lib/i18n/date_forma... lib/i18n/date_format.dart:579: * Return the width of our string field. Different widths represent different On 2012/07/31 05:52:53, Emily Fortuna wrote: > [string] Done. https://chromiumcodereview.appspot.com/10807096/diff/2001/lib/i18n/date_forma... lib/i18n/date_format.dart:580: * formatting options. See the comment for DateFormat for details. On 2012/07/31 05:52:53, Emily Fortuna wrote: > where is this comment for DateFormat? Lines 80 through 112, particularly the 107-112 part. https://chromiumcodereview.appspot.com/10807096/diff/2001/lib/i18n/date_forma... lib/i18n/date_format.dart:584: _DateFormatField(this.string, this.type,this.parent); On 2012/07/31 05:52:53, Emily Fortuna wrote: > space after , Done. https://chromiumcodereview.appspot.com/10807096/diff/2001/lib/i18n/date_forma... lib/i18n/date_format.dart:594: var someRegex = new RegExp("\'\'"); On 2012/07/31 05:52:53, Emily Fortuna wrote: > @ before a string also can denote a raw string I'm not clear what the implications of that are. Do you mean that we need to check for raw strings and process them differently? https://chromiumcodereview.appspot.com/10807096/diff/2001/lib/i18n/date_forma... lib/i18n/date_format.dart:888: return 'not implemented'; On 2012/07/31 05:52:53, Emily Fortuna wrote: > throw NotImplementedException Done. https://chromiumcodereview.appspot.com/10807096/diff/2001/lib/i18n/date_forma... lib/i18n/date_format.dart:911: /** A class for holding onto the data for a date so that it can be built On 2012/07/31 05:52:53, Emily Fortuna wrote: > separate line /** Done. https://chromiumcodereview.appspot.com/10807096/diff/2001/lib/i18n/date_forma... lib/i18n/date_format.dart:926: void setYear(x) {year = x;} On 2012/07/31 05:52:53, Emily Fortuna wrote: > void setYear(x) => year = x > or > void setYear(x) { year = x; } Done. I'd prefer the => form, but using it with a void function caused issues, since => means to return the value. Found a VM bug that way, but was told the code was incorrect and should be changed. https://chromiumcodereview.appspot.com/10807096/diff/2001/lib/i18n/date_forma... lib/i18n/date_format.dart:939: if (year == 0) { On 2012/07/31 05:52:53, Emily Fortuna wrote: > if year !=0 but month or day == 0? Done. https://chromiumcodereview.appspot.com/10807096/diff/2001/lib/i18n/date_forma... lib/i18n/date_format.dart:972: /** Return the next [howMany] items, or as many as there are remaining. On 2012/07/31 05:52:53, Emily Fortuna wrote: > /** on own line Done. https://chromiumcodereview.appspot.com/10807096/diff/2001/lib/i18n/date_forma... lib/i18n/date_format.dart:1002: int findIndex(Function f) { On 2012/07/31 05:52:53, Emily Fortuna wrote: > This also has the side effect of modifying index/advancing to that element, > which you should probably mention. Done. https://chromiumcodereview.appspot.com/10807096/diff/2001/lib/i18n/date_forma... lib/i18n/date_format.dart:1026: digits.add(next().charCodeAt(0)); On 2012/07/31 05:52:53, Emily Fortuna wrote: > this could be done faster with a regular expression... It's not obvious to me how to do that without complicating things, or at least changing them significantly. Doing it in the same way, testing a single character at a time, is actually slower using a regular expression. It might be possible to do something with a regular expression to match a whole number at once, but that's tricky to do as a local change, especially without a regular expression method to start matching part-way through the string, so we'd need to copy the string and match from that. It doesn't seem to be a bottleneck anyway. The most obvious performance optimization not in here is to cache previously used formats for a particular skeleton. https://chromiumcodereview.appspot.com/10807096/diff/2001/lib/i18n/date_forma... lib/i18n/date_format.dart:1043: On 2012/07/31 05:52:53, Emily Fortuna wrote: > whitespace Done. https://chromiumcodereview.appspot.com/10807096/diff/2001/lib/i18n/date_symbo... File lib/i18n/date_symbol_data.dart (right): https://chromiumcodereview.appspot.com/10807096/diff/2001/lib/i18n/date_symbo... lib/i18n/date_symbol_data.dart:5: /** Data for the strings used in a particular locale for date/time constants, On 2012/07/31 05:52:53, Emily Fortuna wrote: > why do we have two doc comment sections, one after the other? Done. https://chromiumcodereview.appspot.com/10807096/diff/2001/lib/i18n/date_symbo... lib/i18n/date_symbol_data.dart:13: * http://go/generate_datetime_constants.py using the --for_dart flag. On 2012/07/31 05:52:53, Emily Fortuna wrote: > - 'http://' > Also, I'd like to see a CL of the changes to this script Done. https://chromiumcodereview.appspot.com/10807096/diff/2001/lib/i18n/date_symbo... lib/i18n/date_symbol_data.dart:25: On 2012/07/31 05:52:53, Emily Fortuna wrote: > whitespace Done. https://chromiumcodereview.appspot.com/10807096/diff/2001/lib/i18n/date_symbo... lib/i18n/date_symbol_data.dart:30: NAME: 'en_ISO', On 2012/07/31 05:52:53, Emily Fortuna wrote: > indentation Done. https://chromiumcodereview.appspot.com/10807096/diff/2001/lib/i18n/date_symbo... lib/i18n/date_symbol_data.dart:34: 'N', 'D'], On 2012/07/31 05:52:53, Emily Fortuna wrote: > indentation! Done. https://chromiumcodereview.appspot.com/10807096/diff/2001/lib/i18n/date_symbo... lib/i18n/date_symbol_data.dart:71: NAME: "af", On 2012/07/31 05:52:53, Emily Fortuna wrote: > more indentation here... Done. https://chromiumcodereview.appspot.com/10807096/diff/2001/lib/i18n/date_symbo... lib/i18n/date_symbol_data.dart:3620: On 2012/07/31 05:52:53, Emily Fortuna wrote: > whitespace! Done. https://chromiumcodereview.appspot.com/10807096/diff/2001/lib/i18n/date_symbo... File lib/i18n/date_symbols.dart (right): https://chromiumcodereview.appspot.com/10807096/diff/2001/lib/i18n/date_symbo... lib/i18n/date_symbols.dart:4: * This holds onto information about how a particular locale formats dates. It On 2012/07/31 05:52:53, Emily Fortuna wrote: > this this file generated? If so, please say so in the comments. Same goes for > all the other files. It's not. The generated files do say so. https://chromiumcodereview.appspot.com/10807096/diff/2001/lib/i18n/date_symbo... lib/i18n/date_symbols.dart:15: STANDALONEMONTHS, SHORTMONTHS,STANDALONESHORTMONTHS,WEEKDAYS, On 2012/07/31 05:52:53, Emily Fortuna wrote: > indent 2 more spaces here if the line is broken. Also, spacing between the > commas here is irregular. Also, use underscores to separate words > (NARROW_MONTHS)? First two done. The constant names are from Shanjian's script, and the same across all the different Google things that he generates for, so I'm reluctant to change them. https://chromiumcodereview.appspot.com/10807096/diff/2001/lib/i18n/date_symbo... lib/i18n/date_symbols.dart:48: ); On 2012/07/31 05:52:53, Emily Fortuna wrote: > move this up to the above line. Done. https://chromiumcodereview.appspot.com/10807096/diff/2001/lib/i18n/date_time_... File lib/i18n/date_time_patterns.dart (right): https://chromiumcodereview.appspot.com/10807096/diff/2001/lib/i18n/date_time_... lib/i18n/date_time_patterns.dart:5: On 2012/07/31 05:52:53, Emily Fortuna wrote: > extra whitespace Done. https://chromiumcodereview.appspot.com/10807096/diff/2001/lib/i18n/date_time_... lib/i18n/date_time_patterns.dart:10: * generate_datetime_pattern_dart.cc (Google internal) On 2012/07/31 05:52:53, Emily Fortuna wrote: > Add "DO NOT EDIT" notice at top to make the autogeneration more obvious. Also, > I'd like to see how this script was modified. Done. https://chromiumcodereview.appspot.com/10807096/diff/2001/lib/i18n/date_time_... lib/i18n/date_time_patterns.dart:20: 'd': 'd', // DAY On 2012/07/31 05:52:53, Emily Fortuna wrote: > indentation... Done. https://chromiumcodereview.appspot.com/10807096/diff/2001/lib/i18n/date_time_... lib/i18n/date_time_patterns.dart:3966: On 2012/07/31 05:52:53, Emily Fortuna wrote: > extra whitespace Done. https://chromiumcodereview.appspot.com/10807096/diff/2001/lib/i18n/intl.dart File lib/i18n/intl.dart (right): https://chromiumcodereview.appspot.com/10807096/diff/2001/lib/i18n/intl.dart#... lib/i18n/intl.dart:27: * will be set later, otherwise the format is not useful. On 2012/07/31 05:52:53, Emily Fortuna wrote: > why make pattern optional? OK, made it required. https://chromiumcodereview.appspot.com/10807096/diff/2001/lib/i18n/intl.dart#... lib/i18n/intl.dart:28: * If [locale] is not specified, then we default to the locale specified On 2012/07/31 05:52:53, Emily Fortuna wrote: > desiredLocale Done. https://chromiumcodereview.appspot.com/10807096/diff/2001/lib/i18n/intl.dart#... lib/i18n/intl.dart:29: * in this instance. Note that this is different from using the static On 2012/07/31 05:52:53, Emily Fortuna wrote: > "locale specified in this instance" please clarify. Done. https://chromiumcodereview.appspot.com/10807096/diff/2001/lib/i18n/intl.dart#... lib/i18n/intl.dart:30: * version of this, in which case we will default to the overall default On 2012/07/31 05:52:53, Emily Fortuna wrote: > please rephrase this last sentence. "this" is a bit unclear unless you re-read > the sentence carefully. Moot, since I removed the static version as per a later comment. https://chromiumcodereview.appspot.com/10807096/diff/2001/lib/i18n/intl.dart#... lib/i18n/intl.dart:47: static DateFormat Date([pattern, desiredLocale]) { On 2012/07/31 05:52:53, Emily Fortuna wrote: > types for parameters Method removed. https://chromiumcodereview.appspot.com/10807096/diff/2001/lib/i18n/intl.dart#... lib/i18n/intl.dart:50: // to do this both statically and on an instance. On 2012/07/31 05:52:53, Emily Fortuna wrote: > if you're going to do this statically the user might as well just create a Date > object explicitly. Let's provide one way to use the Intl object, and you have to > construct the DateFormatting objects explicitly if you want to do "on the fly" > locale detection versus a specific locale. Done. https://chromiumcodereview.appspot.com/10807096/diff/2001/tests/lib/i18n/date... File tests/lib/i18n/date_time_format_test.dart (left): https://chromiumcodereview.appspot.com/10807096/diff/2001/tests/lib/i18n/date... tests/lib/i18n/date_time_format_test.dart:8: On 2012/07/31 05:52:53, Emily Fortuna wrote: > keep this in. OK. Why do we define libraries for things that don't provide anything useful to something that imports them? https://chromiumcodereview.appspot.com/10807096/diff/2001/tests/lib/i18n/date... File tests/lib/i18n/date_time_format_test.dart (right): https://chromiumcodereview.appspot.com/10807096/diff/2001/tests/lib/i18n/date... tests/lib/i18n/date_time_format_test.dart:20: const [ On 2012/07/31 05:52:53, Emily Fortuna wrote: > move this up to the previous line Done. https://chromiumcodereview.appspot.com/10807096/diff/2001/tests/lib/i18n/date... tests/lib/i18n/date_time_format_test.dart:47: //TODO(alanknight): CLDR and ICU appear to disagree on these for Japanese On 2012/07/31 05:52:53, Emily Fortuna wrote: > indentation Done. https://chromiumcodereview.appspot.com/10807096/diff/2001/tests/lib/i18n/date... tests/lib/i18n/date_time_format_test.dart:128: On 2012/07/31 05:52:53, Emily Fortuna wrote: > extra whitespace Done. https://chromiumcodereview.appspot.com/10807096/diff/2001/tests/lib/i18n/date... tests/lib/i18n/date_time_format_test.dart:130: var date_format = new DateFormat(); On 2012/07/31 05:52:53, Emily Fortuna wrote: > indentation. please! Done. https://chromiumcodereview.appspot.com/10807096/diff/2001/tests/lib/i18n/date... tests/lib/i18n/date_time_format_test.dart:133: (date_format.parsePattern("hh:mm:ss")).map((x)=>x.string), On 2012/07/31 05:52:53, Emily Fortuna wrote: > extra paren here starting Done. https://chromiumcodereview.appspot.com/10807096/diff/2001/tests/lib/i18n/date... tests/lib/i18n/date_time_format_test.dart:140: orderedEquals(types)); On 2012/07/31 05:52:53, Emily Fortuna wrote: > test some other cases for parsing? Possible, although this functionality is also well-exercised by the round-trip tests and the explicit format tests. https://chromiumcodereview.appspot.com/10807096/diff/2001/tests/lib/i18n/date... tests/lib/i18n/date_time_format_test.dart:244: * locale. Expects to be given a Map from ICU constant names to the On 2012/07/31 05:52:53, Emily Fortuna wrote: > info about the parameters? nit: Can this function be up at the top before it's > used? Done. https://chromiumcodereview.appspot.com/10807096/diff/2001/tests/lib/i18n/date... tests/lib/i18n/date_time_format_test.dart:249: for(int i=0; i<formatsToTest.length;i++) { On 2012/07/31 05:52:53, Emily Fortuna wrote: > spacing. i < formatsToTest.length; i++ here and other places Done. https://chromiumcodereview.appspot.com/10807096/diff/2001/tests/lib/i18n/date... tests/lib/i18n/date_time_format_test.dart:254: // print("Expect: ${expectedResults[formatName]} from format $formatName " On 2012/07/31 05:52:53, Emily Fortuna wrote: > cut commented out code :-/ Done. https://chromiumcodereview.appspot.com/10807096/diff/2001/tests/lib/i18n/date... tests/lib/i18n/date_time_format_test.dart:259: expect(expectedResults[icuName],equals(actualResult)); On 2012/07/31 05:52:53, Emily Fortuna wrote: > spacing after , Done. https://chromiumcodereview.appspot.com/10807096/diff/2001/tests/lib/i18n/date... tests/lib/i18n/date_time_format_test.dart:281: var format = new DateFormat(skeleton,localeName); On 2012/07/31 05:52:53, Emily Fortuna wrote: > space after , Done. https://chromiumcodereview.appspot.com/10807096/diff/2001/tests/lib/i18n/date... tests/lib/i18n/date_time_format_test.dart:284: if (!badPatterns.some((x) => x == format.pattern)) { On 2012/07/31 05:52:53, Emily Fortuna wrote: > ? If this format's pattern is not in the list of patterns that we can't easily do a round-trip test on because they lose too much information, then do a round-trip test on this format. Basically a verbose way of writing badPatterns.contains(format.pattern) since we don't have an contains() method. https://chromiumcodereview.appspot.com/10807096/diff/2001/tests/lib/i18n/date... tests/lib/i18n/date_time_format_test.dart:288: expect(thenPrintAgain,equals(actualResult)); On 2012/07/31 05:52:53, Emily Fortuna wrote: > space after , . Done. https://chromiumcodereview.appspot.com/10807096/diff/2001/tests/lib/i18n/date... File tests/lib/i18n/date_time_format_test_data.dart (right): https://chromiumcodereview.appspot.com/10807096/diff/2001/tests/lib/i18n/date... tests/lib/i18n/date_time_format_test_data.dart:1: var English = const { On 2012/07/31 05:52:53, Emily Fortuna wrote: > copyright notice? some comment explanation of what this file is? > an explanation of why this set of locales? Done. At least as much of an explanation as I have of the choice of locales. https://chromiumcodereview.appspot.com/10807096/diff/2001/tests/lib/i18n/date... tests/lib/i18n/date_time_format_test_data.dart:2: "DAY" : "27", On 2012/07/31 05:52:53, Emily Fortuna wrote: > indentation Done. https://chromiumcodereview.appspot.com/10807096/diff/2001/tests/lib/i18n/date... tests/lib/i18n/date_time_format_test_data.dart:46: "YEAR_MONTH_WEEKDAY_DAY + HOUR_MINUTE_SECOND + GENERIC_TZ" : "Friday, January 27, 2012 8:58:59 PM Pacific Time", On 2012/07/31 05:52:53, Emily Fortuna wrote: > 80 char Done. https://chromiumcodereview.appspot.com/10807096/diff/2001/tests/lib/i18n/date... tests/lib/i18n/date_time_format_test_data.dart:100: On 2012/07/31 05:52:53, Emily Fortuna wrote: > extra whitespace Done. https://chromiumcodereview.appspot.com/10807096/diff/2001/tests/lib/i18n/date... tests/lib/i18n/date_time_format_test_data.dart:301: On 2012/07/31 05:52:53, Emily Fortuna wrote: > extra whitespace Done. https://chromiumcodereview.appspot.com/10807096/diff/2001/tests/lib/i18n/intl... File tests/lib/i18n/intl_message_test.dart (right): https://chromiumcodereview.appspot.com/10807096/diff/2001/tests/lib/i18n/intl... tests/lib/i18n/intl_message_test.dart:82: expect(de.locale,equals('de_DE')); On 2012/07/31 05:52:53, Emily Fortuna wrote: > space after , > why put this in intl_message_test? Done. Moved to a separate intl_test.dart.
https://chromiumcodereview.appspot.com/10807096/diff/2001/lib/i18n/date_forma... File lib/i18n/date_format.dart (right): https://chromiumcodereview.appspot.com/10807096/diff/2001/lib/i18n/date_forma... lib/i18n/date_format.dart:455: bool _localeExists(localeName) { On 2012/08/03 23:02:15, Alan Knight wrote: > On 2012/07/31 05:52:53, Emily Fortuna wrote: > > -1 space indentation. Please check that your code is properly formatted prior > to > > submitting for review. I'm a very expensive linter. > > Sorry, I did try to find formatting issues, but am both expensive and > error-prone as a linter. Is there any kind of automated mechanism? Last I heard the editor team was going to be working on something like this. You might follow up with them to see how that's going (maybe it's completed?), and at the very least indicate that there is serious interest for this and motivate them to make more progress on this. https://chromiumcodereview.appspot.com/10807096/diff/2001/lib/i18n/date_forma... lib/i18n/date_format.dart:492: if (aLocale.length != 5) return aLocale; On 2012/08/03 23:02:15, Alan Knight wrote: > On 2012/07/31 05:52:53, Emily Fortuna wrote: > > what other acceptable locales are there? > > if it's just two letters like "en" (are there one letter locales?), then seems > > like we could just test for the presence of '-' or '_'. If there are one > letter > > locales, then what if the locale is e-fr (in which case we still need to do > > canonicalization) > > Locales should be of the form either 'en' or 'en_US', with the apparent > exception of 'en_ISO' which is manually added to the generated list. There are > also some things that use hyphens instead of underscores. Closure also accepts > lowercase countries. More recent things allow passing longer strings that > indicate what locale to use for particular aspects, e.g. English locale, but > with Portugese date formats and German phone book collation. But we're not > handling those right now. > > What are you suggesting to do here? We could test for the presence anywhere in > the string of '-' or '_'. That's probably slower than testing length first and > looking in the known required position, but might be shorter to write. > This is fine. I just wanted to understand the cases where this is used. See my other comment on the new version of this function. https://chromiumcodereview.appspot.com/10807096/diff/2001/lib/i18n/date_forma... lib/i18n/date_format.dart:926: void setYear(x) {year = x;} On 2012/08/03 23:02:15, Alan Knight wrote: > On 2012/07/31 05:52:53, Emily Fortuna wrote: > > void setYear(x) => year = x > > or > > void setYear(x) { year = x; } > > Done. I'd prefer the => form, but using it with a void function caused issues, > since => means to return the value. Found a VM bug that way, but was told the > code was incorrect and should be changed. Oh.. weird. I thought that it was okay to have => for void functions. I've learned something new! https://chromiumcodereview.appspot.com/10807096/diff/2001/lib/i18n/date_forma... lib/i18n/date_format.dart:1026: digits.add(next().charCodeAt(0)); On 2012/08/03 23:02:15, Alan Knight wrote: > On 2012/07/31 05:52:53, Emily Fortuna wrote: > > this could be done faster with a regular expression... > > It's not obvious to me how to do that without complicating things, or at least > changing them significantly. Doing it in the same way, testing a single > character at a time, is actually slower using a regular expression. It might be > possible to do something with a regular expression to match a whole number at > once, but that's tricky to do as a local change, especially without a regular > expression method to start matching part-way through the string, so we'd need to > copy the string and match from that. It doesn't seem to be a bottleneck anyway. > The most obvious performance optimization not in here is to cache previously > used formats for a particular skeleton. Alrighty. Nevermind then. Seemed to make more sense at the time. https://chromiumcodereview.appspot.com/10807096/diff/11001/lib/i18n/_date_for... File lib/i18n/_date_format_field.dart (right): https://chromiumcodereview.appspot.com/10807096/diff/11001/lib/i18n/_date_for... lib/i18n/_date_format_field.dart:11: class _DateFormatField{ space between _DateFormatField and { https://chromiumcodereview.appspot.com/10807096/diff/11001/lib/i18n/_date_for... lib/i18n/_date_format_field.dart:31: String format(Date date) { String format(Date date) => pattern; (?) seems like this is unimplemented... Should we add a TODO? https://chromiumcodereview.appspot.com/10807096/diff/11001/lib/i18n/_date_for... lib/i18n/_date_format_field.dart:40: if (found != pattern) throw if your "if" statement is going to span more than one line, use {}. Don't line break + 4 spaces. https://chromiumcodereview.appspot.com/10807096/diff/11001/lib/i18n/_date_for... lib/i18n/_date_format_field.dart:50: class _DateFormatLiteralField extends _DateFormatField{ space before { and elsewhere https://chromiumcodereview.appspot.com/10807096/diff/11001/lib/i18n/_date_for... lib/i18n/_date_format_field.dart:52: _DateFormatLiteralField(pattern,parent): super(pattern,parent); give these variables some space to breathe! space after the , https://chromiumcodereview.appspot.com/10807096/diff/11001/lib/i18n/_date_for... lib/i18n/_date_format_field.dart:83: var twoEscapedQuotes = new RegExp("\'\'"); nit: could also be written as new RegExp(@"''"); https://chromiumcodereview.appspot.com/10807096/diff/11001/lib/i18n/_date_for... lib/i18n/_date_format_field.dart:96: _DateFormatPatternField(pattern,parent): super(pattern,parent); space between parameters: "pattern, parent" https://chromiumcodereview.appspot.com/10807096/diff/11001/lib/i18n/_date_for... lib/i18n/_date_format_field.dart:196: _Stream input, curious indentation https://chromiumcodereview.appspot.com/10807096/diff/11001/lib/i18n/_date_for... File lib/i18n/_date_format_helpers.dart (right): https://chromiumcodereview.appspot.com/10807096/diff/11001/lib/i18n/_date_for... lib/i18n/_date_format_helpers.dart:1: /** copyright notice at top https://chromiumcodereview.appspot.com/10807096/diff/11001/lib/i18n/_date_for... lib/i18n/_date_format_helpers.dart:16: // Functions that exist just be closurized so we can pass them to a general missing word in here "exist just be closurized" https://chromiumcodereview.appspot.com/10807096/diff/11001/lib/i18n/_date_for... lib/i18n/_date_format_helpers.dart:41: year, indent 4 spaces here https://chromiumcodereview.appspot.com/10807096/diff/11001/lib/i18n/_date_for... lib/i18n/_date_format_helpers.dart:85: index, 4 spaces indent if it's after an opening ( https://chromiumcodereview.appspot.com/10807096/diff/11001/lib/i18n/_date_for... lib/i18n/_date_format_helpers.dart:97: /** Find the index of the first element for which [f] returns true.*/ mention this has the side effect of advancing the steam's index https://chromiumcodereview.appspot.com/10807096/diff/11001/lib/i18n/_date_for... lib/i18n/_date_format_helpers.dart:135: class FormatException implements Exception { We now have a FormatException as a subclass of the main Exception class! So we can get rid of this. (Hooray!) https://chromiumcodereview.appspot.com/10807096/diff/11001/lib/i18n/date_form... File lib/i18n/date_format.dart (right): https://chromiumcodereview.appspot.com/10807096/diff/11001/lib/i18n/date_form... lib/i18n/date_format.dart:71: * "yMd" ->07/10/1996 nit: do you want the -> to be immediately adjacent to the result? https://chromiumcodereview.appspot.com/10807096/diff/11001/lib/i18n/date_form... lib/i18n/date_format.dart:83: * y year (Number) 1996 add back 1 space here to replace the character removed https://chromiumcodereview.appspot.com/10807096/diff/11001/lib/i18n/date_form... lib/i18n/date_format.dart:173: #source('_date_format_field.dart'); Suggestion: Let's try following the pub scheme here. Public files that people use are in this top level i18n directory, and then things that are private to the library (like _date_format_field.dart) are one level deeper in a lib directory (and don't name the file starting with _). We can change this back to how you have it if this ends up feeling strange, but that's the current plan. https://chromiumcodereview.appspot.com/10807096/diff/11001/lib/i18n/date_form... lib/i18n/date_format.dart:180: * in the [locale] or in a default if none is specified, and the corresponding "in a default" locale? https://chromiumcodereview.appspot.com/10807096/diff/11001/lib/i18n/date_form... lib/i18n/date_format.dart:183: * For example, in an en_US locale, specifying the skeleton new "paragraph"? so: * of the supported skeleton forms then it is used as a format directly. * * For example, in an en_US local etc https://chromiumcodereview.appspot.com/10807096/diff/11001/lib/i18n/date_form... lib/i18n/date_format.dart:184: * new DateFormat('yMEd'); Use markdown code indicators: http://daringfireball.net/projects/markdown/syntax/#code https://chromiumcodereview.appspot.com/10807096/diff/11001/lib/i18n/date_form... lib/i18n/date_format.dart:191: * IllegalArgumentException is thrown. [IllegalArgumentException] also... Bob is in the process of renaming these exceptions. I believe this one is going to be ArgumentException. Whatever it is, just make sure you touch base with him before committing/are in sync with the repo. https://chromiumcodereview.appspot.com/10807096/diff/11001/lib/i18n/date_form... lib/i18n/date_format.dart:212: _formatFields.forEach( nit: can line 212 and 213 all be one line? https://chromiumcodereview.appspot.com/10807096/diff/11001/lib/i18n/date_form... lib/i18n/date_format.dart:259: get locale() => _locale; I know you can look up the type of _locale, but listing the type here is probably nice, too. https://chromiumcodereview.appspot.com/10807096/diff/11001/lib/i18n/date_form... lib/i18n/date_format.dart:424: set pattern(String inputPattern) { I'd prefer to make this private unless we have a compelling reason otherwise. https://chromiumcodereview.appspot.com/10807096/diff/11001/lib/i18n/date_form... lib/i18n/date_format.dart:437: get pattern() => _pattern; again, in theory the user should already know this, because they just called a particular constructor with a pattern. https://chromiumcodereview.appspot.com/10807096/diff/11001/lib/i18n/date_form... lib/i18n/date_format.dart:465: * A series of regular expression used to parse a format string into its expression => expressions https://chromiumcodereview.appspot.com/10807096/diff/11001/lib/i18n/date_form... lib/i18n/date_format.dart:469: // Quoted String - anything between single quotes, with escaping too much indentation? https://chromiumcodereview.appspot.com/10807096/diff/11001/lib/i18n/date_form... lib/i18n/date_format.dart:514: return _fieldConstructors[i](match.group(0),this); space after , https://chromiumcodereview.appspot.com/10807096/diff/11001/lib/i18n/date_form... lib/i18n/date_format.dart:514: return _fieldConstructors[i](match.group(0),this); space after , https://chromiumcodereview.appspot.com/10807096/diff/11001/lib/i18n/date_form... lib/i18n/date_format.dart:519: /** Polyfill for missing library function. Issue 2804 */ Thanks for adding the issue number. Adding a comment on the bug that we're reimplementing this in i18n may help provide additional motivation to move forward on that, too. Perhaps make the issue num a TODO? // TODO(alanknight): Use standardized list reverse when implemented. See Issue 2804. https://chromiumcodereview.appspot.com/10807096/diff/11001/lib/i18n/date_symb... File lib/i18n/date_symbol_data.dart (right): https://chromiumcodereview.appspot.com/10807096/diff/11001/lib/i18n/date_symb... lib/i18n/date_symbol_data.dart:69: ERANAMES: const [ 'voor Christus', 'na Christus'], extra whitespace? [ 'voor Christus', 'na Christus'] (there are two spaces between the list items.) https://chromiumcodereview.appspot.com/10807096/diff/11001/lib/i18n/date_symb... File lib/i18n/date_symbols.dart (right): https://chromiumcodereview.appspot.com/10807096/diff/11001/lib/i18n/date_symb... lib/i18n/date_symbols.dart:25: this.ERAS, indentation? https://chromiumcodereview.appspot.com/10807096/diff/11001/lib/i18n/date_symb... lib/i18n/date_symbols.dart:51: extra newline https://chromiumcodereview.appspot.com/10807096/diff/11001/lib/i18n/date_time... File lib/i18n/date_time_patterns.dart (right): https://chromiumcodereview.appspot.com/10807096/diff/11001/lib/i18n/date_time... lib/i18n/date_time_patterns.dart:9: * generate_datetime_pattern_dart.cc (Google internal) is there/can you make a "go-link" for this? https://chromiumcodereview.appspot.com/10807096/diff/11001/lib/i18n/date_time... lib/i18n/date_time_patterns.dart:15: /** You're going to love this... I told you wrong about this file. It was correctly indented the first time you sent it out. I talked to Bob, and it is acceptable for list and map initialization spanning multiple lines to only indent 2 spaces. Same with maps. However in cases like line 25 here: https://chromiumcodereview.appspot.com/10807096/diff/11001/lib/i18n/date_symb... you should indent the 4 spaces. (as you have it correctly already). I'm sorry I told you incorrectly about this file. Elsewhere you should be okay. https://chromiumcodereview.appspot.com/10807096/diff/11001/lib/i18n/intl.dart File lib/i18n/intl.dart (right): https://chromiumcodereview.appspot.com/10807096/diff/11001/lib/i18n/intl.dart... lib/i18n/intl.dart:10: #library('intl'); personal preference, but I preferred the documentation for this class to be immediately above the class (Intl), without the #library and #source in between. https://chromiumcodereview.appspot.com/10807096/diff/11001/lib/i18n/intl.dart... lib/i18n/intl.dart:34: return new DateFormat(pattern,actualLocale); space between parameters https://chromiumcodereview.appspot.com/10807096/diff/11001/lib/i18n/intl.dart... lib/i18n/intl.dart:44: _locale = (a_locale == null) ? _getDefaultLocale() : a_locale; do we want to call "verifiedLocale" to canonicalize the locale being set? https://chromiumcodereview.appspot.com/10807096/diff/11001/lib/i18n/intl.dart... lib/i18n/intl.dart:72: static String defaultLocale = 'en_US'; let's move this down to the _getDefaultLocale function so we don't have duplicate functionality in two different ways. https://chromiumcodereview.appspot.com/10807096/diff/11001/lib/i18n/intl.dart... lib/i18n/intl.dart:79: if (_locale == null) { Since we do this check when Intl is created, do we really need this? We could just make _locale public so anyone can read/change as they like... https://chromiumcodereview.appspot.com/10807096/diff/11001/lib/i18n/intl.dart... lib/i18n/intl.dart:104: if (newLocale == null) return defaultLocale; call _getDefaultLocale... https://chromiumcodereview.appspot.com/10807096/diff/11001/lib/i18n/intl.dart... lib/i18n/intl.dart:127: if (aLocale.length != 5) return aLocale; Consider rewriting. It feels a little misleading to me to not throw the error here but instead wait for _localeExists to validate that it's an actual locale, but if you prefer writing this this way, this clearly does work as well. :-) https://chromiumcodereview.appspot.com/10807096/diff/11001/tests/lib/i18n/dat... File tests/lib/i18n/date_time_format_test.dart (right): https://chromiumcodereview.appspot.com/10807096/diff/11001/tests/lib/i18n/dat... tests/lib/i18n/date_time_format_test.dart:76: "DAY", consistent indentation (line 75 and 76 should be the same). You can indent 2 spaces (see my comment here: https://chromiumcodereview.appspot.com/10807096/diff/11001/lib/i18n/date_time...) here and in lines 23-70. https://chromiumcodereview.appspot.com/10807096/diff/11001/tests/lib/i18n/dat... tests/lib/i18n/date_time_format_test.dart:135: for(int i=0; i<formatsToTest.length;i++) { spacing: i < formatsToTest.length; i++ https://chromiumcodereview.appspot.com/10807096/diff/11001/tests/lib/i18n/dat... tests/lib/i18n/date_time_format_test.dart:140: expect(expectedResults[icuName],equals(actualResult)); add a space after the "," https://chromiumcodereview.appspot.com/10807096/diff/11001/tests/lib/i18n/dat... tests/lib/i18n/date_time_format_test.dart:152: DateFormat.ABBR_WEEKDAY, I think the preferred indentation is: [DateFormat.ABBR_WEEKDAY, DateFormat.WEEKDAY, DateFormat.QUARTER, etc (indenting 4 spaces) OR badSkeletons = [ DateFormat.ABBR_WEEKDAY, DateFormat.WEEKDAY etc OR badSkeletons = [DateFormat.ABBR_WEEKDAY, DateFormat.WEEKDAY, etc]; Technically, the formatting you have here is acceptable (yes, it is indented at least 4 spaces), but it strikes me a bit odd and inconsistent with the rest of the code. https://chromiumcodereview.appspot.com/10807096/diff/11001/tests/lib/i18n/dat... tests/lib/i18n/date_time_format_test.dart:179: date_format.parsePattern("hh:mm:ss").map((x)=>x.pattern), space around the => https://chromiumcodereview.appspot.com/10807096/diff/11001/tests/lib/i18n/dat... tests/lib/i18n/date_time_format_test.dart:187: var aDate = new Date(2012,1,27,20,58,59,0,false); just indent 2 spaces here (preferred). Or format the others like this. Just be consistent. https://chromiumcodereview.appspot.com/10807096/diff/11001/tests/lib/i18n/dat... tests/lib/i18n/date_time_format_test.dart:215: var compare = (a,b) => a.compareTo(b); space after , here and below https://chromiumcodereview.appspot.com/10807096/diff/11001/tests/lib/i18n/dat... tests/lib/i18n/date_time_format_test.dart:256: usWithWords.format(aDate), 4 spaces indent https://chromiumcodereview.appspot.com/10807096/diff/11001/tests/lib/i18n/dat... File tests/lib/i18n/date_time_format_test_data.dart (right): https://chromiumcodereview.appspot.com/10807096/diff/11001/tests/lib/i18n/dat... tests/lib/i18n/date_time_format_test_data.dart:6: * Test data for one particular date formatted for a small number of locales. why use this set to test? You mention briefly in date_time_format_test.dart why Austrian vs regular German, and you might elaborate on that here. Why should we feel reasonably confident that we have reasonable test coverage with these locales? Are these the "extremes" in Date formatting? https://chromiumcodereview.appspot.com/10807096/diff/11001/tests/lib/i18n/dat... tests/lib/i18n/date_time_format_test_data.dart:12: "DAY" : "27", indent two more spaces
https://chromiumcodereview.appspot.com/10807096/diff/11001/lib/i18n/_date_for... File lib/i18n/_date_format_field.dart (right): https://chromiumcodereview.appspot.com/10807096/diff/11001/lib/i18n/_date_for... lib/i18n/_date_format_field.dart:11: class _DateFormatField{ On 2012/08/06 22:43:00, Emily Fortuna wrote: > space between _DateFormatField and { Done. https://chromiumcodereview.appspot.com/10807096/diff/11001/lib/i18n/_date_for... lib/i18n/_date_format_field.dart:31: String format(Date date) { On 2012/08/06 22:43:00, Emily Fortuna wrote: > String format(Date date) => pattern; (?) > seems like this is unimplemented... Should we add a TODO? It's a default implementation in the superclass that works for the two literal pattern subclasses. The field pattern subclass overrides. Added a comment. Can push it down into the two subclasses if you think that's clearer. https://chromiumcodereview.appspot.com/10807096/diff/11001/lib/i18n/_date_for... lib/i18n/_date_format_field.dart:40: if (found != pattern) throw On 2012/08/06 22:43:00, Emily Fortuna wrote: > if your "if" statement is going to span more than one line, use {}. Don't line > break + 4 spaces. Done. https://chromiumcodereview.appspot.com/10807096/diff/11001/lib/i18n/_date_for... lib/i18n/_date_format_field.dart:50: class _DateFormatLiteralField extends _DateFormatField{ On 2012/08/06 22:43:00, Emily Fortuna wrote: > space before { and elsewhere Done. https://chromiumcodereview.appspot.com/10807096/diff/11001/lib/i18n/_date_for... lib/i18n/_date_format_field.dart:52: _DateFormatLiteralField(pattern,parent): super(pattern,parent); On 2012/08/06 22:43:00, Emily Fortuna wrote: > give these variables some space to breathe! space after the , Done. https://chromiumcodereview.appspot.com/10807096/diff/11001/lib/i18n/_date_for... lib/i18n/_date_format_field.dart:83: var twoEscapedQuotes = new RegExp("\'\'"); On 2012/08/06 22:43:00, Emily Fortuna wrote: > nit: could also be written as new RegExp(@"''"); Done. https://chromiumcodereview.appspot.com/10807096/diff/11001/lib/i18n/_date_for... lib/i18n/_date_format_field.dart:96: _DateFormatPatternField(pattern,parent): super(pattern,parent); On 2012/08/06 22:43:00, Emily Fortuna wrote: > space between parameters: > "pattern, parent" Done. https://chromiumcodereview.appspot.com/10807096/diff/11001/lib/i18n/_date_for... lib/i18n/_date_format_field.dart:196: _Stream input, On 2012/08/06 22:43:00, Emily Fortuna wrote: > curious indentation Done. It fits into 80 characters now, so no need to break at all. https://chromiumcodereview.appspot.com/10807096/diff/11001/lib/i18n/_date_for... File lib/i18n/_date_format_helpers.dart (right): https://chromiumcodereview.appspot.com/10807096/diff/11001/lib/i18n/_date_for... lib/i18n/_date_format_helpers.dart:1: /** On 2012/08/06 22:43:00, Emily Fortuna wrote: > copyright notice at top Oops. Done. https://chromiumcodereview.appspot.com/10807096/diff/11001/lib/i18n/_date_for... lib/i18n/_date_format_helpers.dart:16: // Functions that exist just be closurized so we can pass them to a general On 2012/08/06 22:43:00, Emily Fortuna wrote: > missing word in here "exist just be closurized" Done. https://chromiumcodereview.appspot.com/10807096/diff/11001/lib/i18n/_date_for... lib/i18n/_date_format_helpers.dart:41: year, On 2012/08/06 22:43:00, Emily Fortuna wrote: > indent 4 spaces here Done. https://chromiumcodereview.appspot.com/10807096/diff/11001/lib/i18n/_date_for... lib/i18n/_date_format_helpers.dart:85: index, On 2012/08/06 22:43:00, Emily Fortuna wrote: > 4 spaces indent if it's after an opening ( Done. https://chromiumcodereview.appspot.com/10807096/diff/11001/lib/i18n/_date_for... lib/i18n/_date_format_helpers.dart:97: /** Find the index of the first element for which [f] returns true.*/ On 2012/08/06 22:43:00, Emily Fortuna wrote: > mention this has the side effect of advancing the steam's index Done. https://chromiumcodereview.appspot.com/10807096/diff/11001/lib/i18n/_date_for... lib/i18n/_date_format_helpers.dart:135: class FormatException implements Exception { On 2012/08/06 22:43:00, Emily Fortuna wrote: > We now have a FormatException as a subclass of the main Exception class! So we > can get rid of this. (Hooray!) Done. https://chromiumcodereview.appspot.com/10807096/diff/11001/lib/i18n/date_form... File lib/i18n/date_format.dart (right): https://chromiumcodereview.appspot.com/10807096/diff/11001/lib/i18n/date_form... lib/i18n/date_format.dart:71: * "yMd" ->07/10/1996 On 2012/08/06 22:43:00, Emily Fortuna wrote: > nit: do you want the -> to be immediately adjacent to the result? Done. https://chromiumcodereview.appspot.com/10807096/diff/11001/lib/i18n/date_form... lib/i18n/date_format.dart:83: * y year (Number) 1996 On 2012/08/06 22:43:00, Emily Fortuna wrote: > add back 1 space here to replace the character removed Done. https://chromiumcodereview.appspot.com/10807096/diff/11001/lib/i18n/date_form... lib/i18n/date_format.dart:173: #source('_date_format_field.dart'); On 2012/08/06 22:43:00, Emily Fortuna wrote: > Suggestion: Let's try following the pub scheme here. Public files that people > use are in this top level i18n directory, and then things that are private to > the library (like _date_format_field.dart) are one level deeper in a lib > directory (and don't name the file starting with _). We can change this back to > how you have it if this ends up feeling strange, but that's the current plan. Yes, I like that much better. Done. https://chromiumcodereview.appspot.com/10807096/diff/11001/lib/i18n/date_form... lib/i18n/date_format.dart:180: * in the [locale] or in a default if none is specified, and the corresponding On 2012/08/06 22:43:00, Emily Fortuna wrote: > "in a default" locale? Done. https://chromiumcodereview.appspot.com/10807096/diff/11001/lib/i18n/date_form... lib/i18n/date_format.dart:183: * For example, in an en_US locale, specifying the skeleton On 2012/08/06 22:43:00, Emily Fortuna wrote: > new "paragraph"? so: > * of the supported skeleton forms then it is used as a format directly. > * > * For example, in an en_US local > etc Done. https://chromiumcodereview.appspot.com/10807096/diff/11001/lib/i18n/date_form... lib/i18n/date_format.dart:184: * new DateFormat('yMEd'); On 2012/08/06 22:43:00, Emily Fortuna wrote: > Use markdown code indicators: > http://daringfireball.net/projects/markdown/syntax/#code Done. https://chromiumcodereview.appspot.com/10807096/diff/11001/lib/i18n/date_form... lib/i18n/date_format.dart:191: * IllegalArgumentException is thrown. On 2012/08/06 22:43:00, Emily Fortuna wrote: > [IllegalArgumentException] also... Bob is in the process of renaming these > exceptions. I believe this one is going to be ArgumentException. Whatever it is, > just make sure you touch base with him before committing/are in sync with the > repo. Done. https://chromiumcodereview.appspot.com/10807096/diff/11001/lib/i18n/date_form... lib/i18n/date_format.dart:212: _formatFields.forEach( On 2012/08/06 22:43:00, Emily Fortuna wrote: > nit: can line 212 and 213 all be one line? Done. https://chromiumcodereview.appspot.com/10807096/diff/11001/lib/i18n/date_form... lib/i18n/date_format.dart:259: get locale() => _locale; On 2012/08/06 22:43:00, Emily Fortuna wrote: > I know you can look up the type of _locale, but listing the type here is > probably nice, too. I had an idea that the style guide said not to put types on getters since they were redundant with the field. But I can't find it, so added a type. https://chromiumcodereview.appspot.com/10807096/diff/11001/lib/i18n/date_form... lib/i18n/date_format.dart:424: set pattern(String inputPattern) { On 2012/08/06 22:43:00, Emily Fortuna wrote: > I'd prefer to make this private unless we have a compelling reason otherwise. Done. https://chromiumcodereview.appspot.com/10807096/diff/11001/lib/i18n/date_form... lib/i18n/date_format.dart:437: get pattern() => _pattern; On 2012/08/06 22:43:00, Emily Fortuna wrote: > again, in theory the user should already know this, because they just called a > particular constructor with a pattern. Somebody probably at some point called a constructor, but that doesn't mean they should have to remember it separately. But more importantly, they probably called it with a constant or a skeleton, but that gets looked up and it's the looked up value that's stored in pattern. This is also used in the tests. https://chromiumcodereview.appspot.com/10807096/diff/11001/lib/i18n/date_form... lib/i18n/date_format.dart:465: * A series of regular expression used to parse a format string into its On 2012/08/06 22:43:00, Emily Fortuna wrote: > expression => expressions Done. https://chromiumcodereview.appspot.com/10807096/diff/11001/lib/i18n/date_form... lib/i18n/date_format.dart:469: // Quoted String - anything between single quotes, with escaping On 2012/08/06 22:43:00, Emily Fortuna wrote: > too much indentation? Done. https://chromiumcodereview.appspot.com/10807096/diff/11001/lib/i18n/date_form... lib/i18n/date_format.dart:514: return _fieldConstructors[i](match.group(0),this); On 2012/08/06 22:43:00, Emily Fortuna wrote: > space after , Done. https://chromiumcodereview.appspot.com/10807096/diff/11001/lib/i18n/date_form... lib/i18n/date_format.dart:514: return _fieldConstructors[i](match.group(0),this); On 2012/08/06 22:43:00, Emily Fortuna wrote: > space after , Done. https://chromiumcodereview.appspot.com/10807096/diff/11001/lib/i18n/date_form... lib/i18n/date_format.dart:519: /** Polyfill for missing library function. Issue 2804 */ On 2012/08/06 22:43:00, Emily Fortuna wrote: > Thanks for adding the issue number. Adding a comment on the bug that we're > reimplementing this in i18n may help provide additional motivation to move > forward on that, too. > Perhaps make the issue num a TODO? > // TODO(alanknight): Use standardized list reverse when implemented. See Issue > 2804. Done. https://chromiumcodereview.appspot.com/10807096/diff/11001/lib/i18n/date_symb... File lib/i18n/date_symbol_data.dart (right): https://chromiumcodereview.appspot.com/10807096/diff/11001/lib/i18n/date_symb... lib/i18n/date_symbol_data.dart:69: ERANAMES: const [ 'voor Christus', 'na Christus'], On 2012/08/06 22:43:00, Emily Fortuna wrote: > extra whitespace? > [ 'voor Christus', 'na Christus'] > (there are two spaces between the list items.) I've made this go away but I don't understand where the other space is coming from in the Python code, which makes me nervous that it might be affecting one of the other things that are generated from there. https://chromiumcodereview.appspot.com/10807096/diff/11001/lib/i18n/date_symb... File lib/i18n/date_symbols.dart (right): https://chromiumcodereview.appspot.com/10807096/diff/11001/lib/i18n/date_symb... lib/i18n/date_symbols.dart:25: this.ERAS, On 2012/08/06 22:43:00, Emily Fortuna wrote: > indentation? Done. https://chromiumcodereview.appspot.com/10807096/diff/11001/lib/i18n/date_symb... lib/i18n/date_symbols.dart:51: On 2012/08/06 22:43:00, Emily Fortuna wrote: > extra newline Done. https://chromiumcodereview.appspot.com/10807096/diff/11001/lib/i18n/date_time... File lib/i18n/date_time_patterns.dart (right): https://chromiumcodereview.appspot.com/10807096/diff/11001/lib/i18n/date_time... lib/i18n/date_time_patterns.dart:9: * generate_datetime_pattern_dart.cc (Google internal) On 2012/08/06 22:43:00, Emily Fortuna wrote: > is there/can you make a "go-link" for this? Done. https://chromiumcodereview.appspot.com/10807096/diff/11001/lib/i18n/date_time... lib/i18n/date_time_patterns.dart:15: /** On 2012/08/06 22:43:00, Emily Fortuna wrote: > You're going to love this... I told you wrong about this file. It was correctly > indented the first time you sent it out. I talked to Bob, and it is acceptable > for list and map initialization spanning multiple lines to only indent 2 spaces. > Same with maps. However in cases like line 25 here: > https://chromiumcodereview.appspot.com/10807096/diff/11001/lib/i18n/date_symb... > you should indent the 4 spaces. (as you have it correctly already). I'm sorry I > told you incorrectly about this file. Elsewhere you should be okay. Heh. Fortunately, that was one of the easy things to change. Done. https://chromiumcodereview.appspot.com/10807096/diff/11001/lib/i18n/intl.dart File lib/i18n/intl.dart (right): https://chromiumcodereview.appspot.com/10807096/diff/11001/lib/i18n/intl.dart... lib/i18n/intl.dart:10: #library('intl'); On 2012/08/06 22:43:00, Emily Fortuna wrote: > personal preference, but I preferred the documentation for this class to be > immediately above the class (Intl), without the #library and #source in between. Done. https://chromiumcodereview.appspot.com/10807096/diff/11001/lib/i18n/intl.dart... lib/i18n/intl.dart:34: return new DateFormat(pattern,actualLocale); On 2012/08/06 22:43:00, Emily Fortuna wrote: > space between parameters Done. https://chromiumcodereview.appspot.com/10807096/diff/11001/lib/i18n/intl.dart... lib/i18n/intl.dart:44: _locale = (a_locale == null) ? _getDefaultLocale() : a_locale; On 2012/08/06 22:43:00, Emily Fortuna wrote: > do we want to call "verifiedLocale" to canonicalize the locale being set? Good idea. Done. https://chromiumcodereview.appspot.com/10807096/diff/11001/lib/i18n/intl.dart... lib/i18n/intl.dart:127: if (aLocale.length != 5) return aLocale; On 2012/08/06 22:43:00, Emily Fortuna wrote: > Consider rewriting. It feels a little misleading to me to not throw the error > here but instead wait for _localeExists to validate that it's an actual locale, > but if you prefer writing this this way, this clearly does work as well. :-) I think I'd rather not throw the exception here because I'd rather separate making the name canonical from testing if we support it. If we don't have a canonical name that isn't necessarily an error. The usage is that we canonicalize the locale and then see if we have data for it. And if we don't have it we also try the short form. That doesn't mean it doesn't exist. e.g. we don't have en_CA, but what we want to do is just fall back to en, not throw an exception. I don't think I'd want to throw an exception and catch it as part of normal control flow when we're only one level down in the stack and can just check it directly. https://chromiumcodereview.appspot.com/10807096/diff/11001/tests/lib/i18n/dat... File tests/lib/i18n/date_time_format_test.dart (right): https://chromiumcodereview.appspot.com/10807096/diff/11001/tests/lib/i18n/dat... tests/lib/i18n/date_time_format_test.dart:76: "DAY", On 2012/08/06 22:43:00, Emily Fortuna wrote: > consistent indentation (line 75 and 76 should be the same). You can indent 2 > spaces (see my comment here: > https://chromiumcodereview.appspot.com/10807096/diff/11001/lib/i18n/date_time...) > here and in lines 23-70. Done. https://chromiumcodereview.appspot.com/10807096/diff/11001/tests/lib/i18n/dat... tests/lib/i18n/date_time_format_test.dart:135: for(int i=0; i<formatsToTest.length;i++) { On 2012/08/06 22:43:00, Emily Fortuna wrote: > spacing: i < formatsToTest.length; i++ Done. https://chromiumcodereview.appspot.com/10807096/diff/11001/tests/lib/i18n/dat... tests/lib/i18n/date_time_format_test.dart:140: expect(expectedResults[icuName],equals(actualResult)); On 2012/08/06 22:43:00, Emily Fortuna wrote: > add a space after the "," Done. https://chromiumcodereview.appspot.com/10807096/diff/11001/tests/lib/i18n/dat... tests/lib/i18n/date_time_format_test.dart:152: DateFormat.ABBR_WEEKDAY, On 2012/08/06 22:43:00, Emily Fortuna wrote: > I think the preferred indentation is: > [DateFormat.ABBR_WEEKDAY, DateFormat.WEEKDAY, > DateFormat.QUARTER, > etc (indenting 4 spaces) OR > > badSkeletons = [ > DateFormat.ABBR_WEEKDAY, > DateFormat.WEEKDAY > etc > > OR > badSkeletons = [DateFormat.ABBR_WEEKDAY, > DateFormat.WEEKDAY, etc]; > > Technically, the formatting you have here is acceptable (yes, it is indented at > least 4 spaces), but it strikes me a bit odd and inconsistent with the rest of > the code. Done. https://chromiumcodereview.appspot.com/10807096/diff/11001/tests/lib/i18n/dat... tests/lib/i18n/date_time_format_test.dart:179: date_format.parsePattern("hh:mm:ss").map((x)=>x.pattern), On 2012/08/06 22:43:00, Emily Fortuna wrote: > space around the => Done. https://chromiumcodereview.appspot.com/10807096/diff/11001/tests/lib/i18n/dat... tests/lib/i18n/date_time_format_test.dart:187: var aDate = new Date(2012,1,27,20,58,59,0,false); On 2012/08/06 22:43:00, Emily Fortuna wrote: > just indent 2 spaces here (preferred). Or format the others like this. Just be > consistent. Done. https://chromiumcodereview.appspot.com/10807096/diff/11001/tests/lib/i18n/dat... tests/lib/i18n/date_time_format_test.dart:215: var compare = (a,b) => a.compareTo(b); On 2012/08/06 22:43:00, Emily Fortuna wrote: > space after , here and below Done. https://chromiumcodereview.appspot.com/10807096/diff/11001/tests/lib/i18n/dat... tests/lib/i18n/date_time_format_test.dart:256: usWithWords.format(aDate), On 2012/08/06 22:43:00, Emily Fortuna wrote: > 4 spaces indent Done. https://chromiumcodereview.appspot.com/10807096/diff/11001/tests/lib/i18n/dat... File tests/lib/i18n/date_time_format_test_data.dart (right): https://chromiumcodereview.appspot.com/10807096/diff/11001/tests/lib/i18n/dat... tests/lib/i18n/date_time_format_test_data.dart:6: * Test data for one particular date formatted for a small number of locales. On 2012/08/06 22:43:00, Emily Fortuna wrote: > why use this set to test? > You mention briefly in date_time_format_test.dart why Austrian vs regular > German, and you might elaborate on that here. Why should we feel reasonably > confident that we have reasonable test coverage with these locales? Are these > the "extremes" in Date formatting? Do we really want to put "This is all the data I could get out of the internationalization people and even that was like pulling teeth?" into the comment? :-) If it were up to me I'd say test all the locales, as I don't know what would be a representative set. It might be worth putting in a TODO to write a C++ or Java program to generate data from ICU and test that. It's still a bit circular because our generation code is also derived from ICU, but better than nothing. https://chromiumcodereview.appspot.com/10807096/diff/11001/tests/lib/i18n/dat... tests/lib/i18n/date_time_format_test_data.dart:12: "DAY" : "27", On 2012/08/06 22:43:00, Emily Fortuna wrote: > indent two more spaces In one of the earlier files you said Maps only needed to indent two spaces.
really shaping up! I still need to more carefully review a few files, but in the interest of getting feedback quickly, here are my immediate comments: https://chromiumcodereview.appspot.com/10807096/diff/11001/lib/i18n/date_symb... File lib/i18n/date_symbol_data.dart (right): https://chromiumcodereview.appspot.com/10807096/diff/11001/lib/i18n/date_symb... lib/i18n/date_symbol_data.dart:69: ERANAMES: const [ 'voor Christus', 'na Christus'], On 2012/08/08 00:47:44, Alan Knight wrote: > On 2012/08/06 22:43:00, Emily Fortuna wrote: > > extra whitespace? > > [ 'voor Christus', 'na Christus'] > > (there are two spaces between the list items.) > I've made this go away but I don't understand where the other space is coming > from in the Python code, which makes me nervous that it might be affecting one > of the other things that are generated from there. > Hm. Be sure to ask Shanjian when he reviews that. https://chromiumcodereview.appspot.com/10807096/diff/11001/lib/i18n/intl.dart File lib/i18n/intl.dart (right): https://chromiumcodereview.appspot.com/10807096/diff/11001/lib/i18n/intl.dart... lib/i18n/intl.dart:127: if (aLocale.length != 5) return aLocale; On 2012/08/08 00:47:44, Alan Knight wrote: > On 2012/08/06 22:43:00, Emily Fortuna wrote: > > Consider rewriting. It feels a little misleading to me to not throw the error > > here but instead wait for _localeExists to validate that it's an actual > locale, > > but if you prefer writing this this way, this clearly does work as well. :-) > > I think I'd rather not throw the exception here because I'd rather separate > making the name canonical from testing if we support it. If we don't have a > canonical name that isn't necessarily an error. The usage is that we > canonicalize the locale and then see if we have data for it. And if we don't > have it we also try the short form. That doesn't mean it doesn't exist. e.g. we > don't have en_CA, but what we want to do is just fall back to en, not throw an > exception. I don't think I'd want to throw an exception and catch it as part of > normal control flow when we're only one level down in the stack and can just > check it directly. Okay. Can you just put a comment maybe to the effect that locales that aren't length 2 or 5 are invalid? https://chromiumcodereview.appspot.com/10807096/diff/11001/tests/lib/i18n/dat... File tests/lib/i18n/date_time_format_test_data.dart (right): https://chromiumcodereview.appspot.com/10807096/diff/11001/tests/lib/i18n/dat... tests/lib/i18n/date_time_format_test_data.dart:6: * Test data for one particular date formatted for a small number of locales. On 2012/08/08 00:47:44, Alan Knight wrote: > On 2012/08/06 22:43:00, Emily Fortuna wrote: > > why use this set to test? > > You mention briefly in date_time_format_test.dart why Austrian vs regular > > German, and you might elaborate on that here. Why should we feel reasonably > > confident that we have reasonable test coverage with these locales? Are these > > the "extremes" in Date formatting? > Do we really want to put "This is all the data I could get out of the > internationalization people and even that was like pulling teeth?" into the > comment? :-) If it were up to me I'd say test all the locales, as I don't know > what would be a representative set. It might be worth putting in a TODO to write > a C++ or Java program to generate data from ICU and test that. It's still a bit > circular because our generation code is also derived from ICU, but better than > nothing. Ah, I see. I didn't understand that was why we had this set. Maybe just have a TODO to get more test data for other locales at some point then. https://chromiumcodereview.appspot.com/10807096/diff/11001/tests/lib/i18n/dat... tests/lib/i18n/date_time_format_test_data.dart:12: "DAY" : "27", On 2012/08/08 00:47:44, Alan Knight wrote: > On 2012/08/06 22:43:00, Emily Fortuna wrote: > > indent two more spaces > In one of the earlier files you said Maps only needed to indent two spaces. Right. Ignore! https://chromiumcodereview.appspot.com/10807096/diff/16001/lib/i18n/date_form... File lib/i18n/date_format.dart (right): https://chromiumcodereview.appspot.com/10807096/diff/16001/lib/i18n/date_form... lib/i18n/date_format.dart:185: * new DateFormat('yMEd'); You'll need to either add an empty line before and after this to make it a code block: * * new DateFormat('yMEd'); * http://daringfireball.net/projects/markdown/syntax/#precode or use backticks for code span (probably better in this case) http://daringfireball.net/projects/markdown/syntax/#code https://chromiumcodereview.appspot.com/10807096/diff/16001/lib/i18n/date_form... lib/i18n/date_format.dart:438: get pattern() => _pattern; TODO: look at alan's comment here https://chromiumcodereview.appspot.com/10807096/diff/16001/lib/i18n/date_time... File lib/i18n/date_time_patterns.dart (right): https://chromiumcodereview.appspot.com/10807096/diff/16001/lib/i18n/date_time... lib/i18n/date_time_patterns.dart:9: * 'http://go/generate_datetime_pattern_dart.cc' (Google internal) go links don't have http just say go/generate_datetime_pattern_dart.cc https://chromiumcodereview.appspot.com/10807096/diff/16001/lib/i18n/date_time... lib/i18n/date_time_patterns.dart:15: /** you can also -2 here if you want for consistency of "map formatting" throughout this file https://chromiumcodereview.appspot.com/10807096/diff/16001/lib/i18n/lib/date_... File lib/i18n/lib/date_format_field.dart (right): https://chromiumcodereview.appspot.com/10807096/diff/16001/lib/i18n/lib/date_... lib/i18n/lib/date_format_field.dart:133: case 'H': handleNumericField(input,builder.setHour); break; // hour 0-23 whitespace after a comma!! grep to find these cases?
Also modified the Google-internal files that do the generation slightly (indentation). https://chromiumcodereview.appspot.com/10807096/diff/11001/lib/i18n/intl.dart File lib/i18n/intl.dart (right): https://chromiumcodereview.appspot.com/10807096/diff/11001/lib/i18n/intl.dart... lib/i18n/intl.dart:127: if (aLocale.length != 5) return aLocale; On 2012/08/08 01:35:54, Emily Fortuna wrote: > On 2012/08/08 00:47:44, Alan Knight wrote: > > On 2012/08/06 22:43:00, Emily Fortuna wrote: > > > Consider rewriting. It feels a little misleading to me to not throw the > error > > > here but instead wait for _localeExists to validate that it's an actual > > locale, > > > but if you prefer writing this this way, this clearly does work as well. :-) > > > > I think I'd rather not throw the exception here because I'd rather separate > > making the name canonical from testing if we support it. If we don't have a > > canonical name that isn't necessarily an error. The usage is that we > > canonicalize the locale and then see if we have data for it. And if we don't > > have it we also try the short form. That doesn't mean it doesn't exist. e.g. > we > > don't have en_CA, but what we want to do is just fall back to en, not throw an > > exception. I don't think I'd want to throw an exception and catch it as part > of > > normal control flow when we're only one level down in the stack and can just > > check it directly. > > Okay. Can you just put a comment maybe to the effect that locales that aren't > length 2 or 5 are invalid? Done. The es_419 case also points out that length 6 is possible, so made the check test for <5 or >6. https://chromiumcodereview.appspot.com/10807096/diff/11001/tests/lib/i18n/dat... File tests/lib/i18n/date_time_format_test_data.dart (right): https://chromiumcodereview.appspot.com/10807096/diff/11001/tests/lib/i18n/dat... tests/lib/i18n/date_time_format_test_data.dart:6: * Test data for one particular date formatted for a small number of locales. On 2012/08/08 01:35:54, Emily Fortuna wrote: > On 2012/08/08 00:47:44, Alan Knight wrote: > > On 2012/08/06 22:43:00, Emily Fortuna wrote: > > > why use this set to test? > > > You mention briefly in date_time_format_test.dart why Austrian vs regular > > > German, and you might elaborate on that here. Why should we feel reasonably > > > confident that we have reasonable test coverage with these locales? Are > these > > > the "extremes" in Date formatting? > > Do we really want to put "This is all the data I could get out of the > > internationalization people and even that was like pulling teeth?" into the > > comment? :-) If it were up to me I'd say test all the locales, as I don't know > > what would be a representative set. It might be worth putting in a TODO to > write > > a C++ or Java program to generate data from ICU and test that. It's still a > bit > > circular because our generation code is also derived from ICU, but better than > > nothing. > > Ah, I see. I didn't understand that was why we had this set. Maybe just have a > TODO to get more test data for other locales at some point then. Done. https://chromiumcodereview.appspot.com/10807096/diff/11001/tests/lib/i18n/dat... tests/lib/i18n/date_time_format_test_data.dart:12: "DAY" : "27", On 2012/08/08 01:35:54, Emily Fortuna wrote: > On 2012/08/08 00:47:44, Alan Knight wrote: > > On 2012/08/06 22:43:00, Emily Fortuna wrote: > > > indent two more spaces > > In one of the earlier files you said Maps only needed to indent two spaces. > > Right. Ignore! Done. https://chromiumcodereview.appspot.com/10807096/diff/16001/lib/i18n/date_form... File lib/i18n/date_format.dart (right): https://chromiumcodereview.appspot.com/10807096/diff/16001/lib/i18n/date_form... lib/i18n/date_format.dart:185: * new DateFormat('yMEd'); On 2012/08/08 01:35:54, Emily Fortuna wrote: > You'll need to either add an empty line before and after this to make it a code > block: > * > * new DateFormat('yMEd'); > * > http://daringfireball.net/projects/markdown/syntax/#precode > > or use backticks for code span (probably better in this case) > http://daringfireball.net/projects/markdown/syntax/#code Done. https://chromiumcodereview.appspot.com/10807096/diff/16001/lib/i18n/date_time... File lib/i18n/date_time_patterns.dart (right): https://chromiumcodereview.appspot.com/10807096/diff/16001/lib/i18n/date_time... lib/i18n/date_time_patterns.dart:9: * 'http://go/generate_datetime_pattern_dart.cc' (Google internal) On 2012/08/08 01:35:54, Emily Fortuna wrote: > go links don't have http > just say go/generate_datetime_pattern_dart.cc I can change it pretty easily, but it's in the http: form for all the other things that it generates for, so I'm a bit reluctant to be inconsistent with them. https://chromiumcodereview.appspot.com/10807096/diff/16001/lib/i18n/date_time... lib/i18n/date_time_patterns.dart:15: /** On 2012/08/08 01:35:54, Emily Fortuna wrote: > you can also -2 here if you want for consistency of "map formatting" throughout > this file Done. https://chromiumcodereview.appspot.com/10807096/diff/16001/lib/i18n/lib/date_... File lib/i18n/lib/date_format_field.dart (right): https://chromiumcodereview.appspot.com/10807096/diff/16001/lib/i18n/lib/date_... lib/i18n/lib/date_format_field.dart:133: case 'H': handleNumericField(input,builder.setHour); break; // hour 0-23 On 2012/08/08 01:35:54, Emily Fortuna wrote: > whitespace after a comma!! > grep to find these cases? Done.
Can you upload your latest round of changes? With those, and the comments, I think you should be good to go. https://chromiumcodereview.appspot.com/10807096/diff/16001/lib/i18n/date_form... File lib/i18n/date_format.dart (right): https://chromiumcodereview.appspot.com/10807096/diff/16001/lib/i18n/date_form... lib/i18n/date_format.dart:438: get pattern() => _pattern; On 2012/08/08 01:35:54, Emily Fortuna wrote: > TODO: look at alan's comment here Done. https://chromiumcodereview.appspot.com/10807096/diff/16001/lib/i18n/date_form... lib/i18n/date_format.dart:469: static var _matchers = const [ move the static field up before the method declarations, with the rest of the fields? https://chromiumcodereview.appspot.com/10807096/diff/16001/lib/i18n/date_time... File lib/i18n/date_time_patterns.dart (right): https://chromiumcodereview.appspot.com/10807096/diff/16001/lib/i18n/date_time... lib/i18n/date_time_patterns.dart:62: 'ZZZZ': 'ZZZZ' // ABBR_UTC_TZ some funny business here with indentation on the last line of each of these.. https://chromiumcodereview.appspot.com/10807096/diff/16001/lib/i18n/intl.dart File lib/i18n/intl.dart (right): https://chromiumcodereview.appspot.com/10807096/diff/16001/lib/i18n/intl.dart... lib/i18n/intl.dart:45: ? _getDefaultLocale() : verifiedLocale(a_locale); If the ? : spans more than one line, switch to if/else (or the two line version like it was previously) https://chromiumcodereview.appspot.com/10807096/diff/16001/lib/i18n/lib/date_... File lib/i18n/lib/date_format_field.dart (right): https://chromiumcodereview.appspot.com/10807096/diff/16001/lib/i18n/lib/date_... lib/i18n/lib/date_format_field.dart:133: case 'H': handleNumericField(input,builder.setHour); break; // hour 0-23 On 2012/08/08 18:21:25, Alan Knight wrote: > On 2012/08/08 01:35:54, Emily Fortuna wrote: > > whitespace after a comma!! > > grep to find these cases? > > Done. Can you upload these changes, please?
https://chromiumcodereview.appspot.com/10807096/diff/16001/lib/i18n/date_form... File lib/i18n/date_format.dart (right): https://chromiumcodereview.appspot.com/10807096/diff/16001/lib/i18n/date_form... lib/i18n/date_format.dart:469: static var _matchers = const [ On 2012/08/08 19:27:36, Emily Fortuna wrote: > move the static field up before the method declarations, with the rest of the > fields? Done. https://chromiumcodereview.appspot.com/10807096/diff/16001/lib/i18n/date_time... File lib/i18n/date_time_patterns.dart (right): https://chromiumcodereview.appspot.com/10807096/diff/16001/lib/i18n/date_time... lib/i18n/date_time_patterns.dart:62: 'ZZZZ': 'ZZZZ' // ABBR_UTC_TZ On 2012/08/08 19:27:36, Emily Fortuna wrote: > some funny business here with indentation on the last line of each of these.. Done. I'd fixed that in google3 but forgot to regenerate. https://chromiumcodereview.appspot.com/10807096/diff/16001/lib/i18n/intl.dart File lib/i18n/intl.dart (right): https://chromiumcodereview.appspot.com/10807096/diff/16001/lib/i18n/intl.dart... lib/i18n/intl.dart:45: ? _getDefaultLocale() : verifiedLocale(a_locale); On 2012/08/08 19:27:36, Emily Fortuna wrote: > If the ? : spans more than one line, switch to if/else (or the two line version > like it was previously) Done.
lgtm! |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
