Hey Alex, Good job with this. Generally, looks good, with a bunch of nits (many related to doc). I've mentioned them on a per-file basis below. If you could take care of these nits (provided that you agree with them), go ahead and commit; there's no need to iterate again on this patch unless you make some significant code changes.
TimeZoneConstants.java 24: should read "..the actual data is defined in property files named TimeZoneConstants_xx.properties.". The following sentence about deferred binding is not needed. 28: should read "Time zone data has only been provided for the "en" locale. Due to the sheer size of the time zone data for each locale, we recommend that applications retrieve the necessary data (i.e. over RPC) for the user's locale at run time." TimeZoneConstants.properties Though I think it already is, ensure that this file is in UTF-8 format before committing it. I also wonder if we should be including the rest of the TimeZone data in this patch. We do the same for DateTimeConstants and NumberConstants; it is somewhat odd that we're not including all of the TimeZone data, even if we do not recommend that they pull it in via deferred binding. Maybe it would suffice to add a URL in TimeZoneConstants.java indicating where users can find the rest of the TimeZone data if need be? TimeZone.java 25-35: All instances of "initiated", "initiate", etc should be changed to "instantiate", "instantiated", and so on 29: "TimeZoneConstants" could make this a javadoc ref to the TimeZoneConstants class 38: Could make this comment more accurate - "constants to reference time zone names in the time zone names array" 39-42: These could all be private 47: "get such string from TimeZoneConstants class, or it can ask server to provide the string" --> "get such a string from the TimeZoneConstants class, or it can request the string from the server.." 48: "But either way, the application obtains the original string from the data provided in TimeZoneConstant.properties, which was carefully prepared from CLDR and Olson time zone database." ---> "Either way, the application obtains the original string from the data provided in the TimeZoneConstats.properties file, which was carefully prepared using data from CLDR and the Olson time zone database." 65:Cast to int is unnecessary here 94: "This factory method provide a decent..." --> "This factory method provides a decent.." 95-96: Add a line break here. 116: Seems like having "GMT" in the array is unnecessary; it can be concatenated to the end result after the transformation step. 131-143, 145-157: This logic is quite similar; can this be factored out in some way? Really, the only difference is the prefix (UTC, or Etc/GMT) 169: Not sure why this comment is here. 179: Maybe clarify this sentence: "Return the adjustment amount of time zone offset" --> "Return the amount to adjust the timezone offset by if daylight savings time is in effect on the given date". Also, give some indication of the units that the offset is expressed in. 181: Should be the "date" to check. 199: "The date for which time to retrieve GMT string." --> "The date from which the time information should be extracted" 200: "GMT String" --> "A GMT representation of the time given by the date" 181-182,198-199: Add a line break here. 207: "To get time zone id for this time zone. For time zone object initiated from time zone offset, POSIX time zone id will be returned." --> "Return time zone id for this time zone. For time zone objects that have been initiated from a time zone offset, the POSIX time zone id will be returned." 217: "To get long time zone name for given date." --> "Returns the long version of the time zone name for the given date.". Add a line break after this sentence as well. 218: "The date for which time to retrieve long time zone string." --> "The date to check to see if daylight savings time is in effect" 238: "To get RFC representation of certain time zone name for given date." --> "Returns the RFC representation of the time zone name for the given date." Add a line break after this line as well. 242: This method is very similar to the composeGMTString(int) method. Can you consolidate these two? 257: "To get short time zone name for given date." -> "Returns the short time zone name for a given date.". Add a line break after this line as well. 258: Add the word "name" before the period. Add a line break before this line as well. 266: "To get the standard time zone offset in minutes." --> "Returns the standard time zone offset in minutes." 268: Need @return tag in javadoc 273: "Check if the given time fall within daylight saving period." --> "Check to see if the given date and time falls within a daylight savings time period." Add a line break. 274: "time for which to check." --> "date and time to check" 275: "if daylight time in effect." --> "if daylight savings time is in effect" DateTimeFormat.java In the javadoc that was added for the public static methods, make sure there are line breaks between the method description and any param/return doc. 651: "hold" --> "holds" 653: "desired format" --> "format defined by this object" 672: Add a line break here 673: Formatting, can move some of the text up from the following line 909: "count" parameter is not needed (not part of your patch) 1346: You don't need the continue statement here, but it can be left if you feel it adds clarity. (not part of your patch) 1564: Add a newline. 1565: "pattern for this field" --> "pattern character for this field" 1568: Remove "hold" 1569: realDate param does not exist 1570,1571: "this date object holds"--> "a date object which holds" TimeZoneInfo.java 34: Add a line break here. 35: Missing description of "json" parameter, missing @return tag TimeZoneTest.java Needs a sort; some methods are out of order. 33: Not sure if we need to have this or not; other parts of the GWT code that use the Date class have deprecation warnings. If we want to make the warning settings consistent for at least this class, make sure you add this annotation to the testNames method as well. 44: Might want to add a comment as to why we decided to generate the dates this way, as opposed to setting the day/year/month/hours/mins/seconds on a date object. TimeZoneInfoTest.java Needs a sort; some methods are out of order. 28: This does not need to be af field; could be a local variable. I18NSuite.java LG. TimeZoneTest.gwt.xml Make sure that you set the svn:mime-type and svn:eol-style properties on this file; it does not look like you set them. 16: Don't need to inherit JSON module anymore. 18: Don't need to explicitly inherit JUnit module; it is inherited by default 19-20: Don't need to specify these; they are implied 21-22: My personal preference, put the <extend-.. declaration before the <set.. declaration Rajeev On Fri, Sep 12, 2008 at 6:35 PM, Alex Rudnick <[EMAIL PROTECTED]> wrote: > +gwtc > > Shanjian, > > Just to make sure I'm not missing something, you're referencing the > extra conversions that we do in TimeZoneInfo? About those, I think > you're probably right -- we wanted to return a nice List<Integer> or > List<String> instead of JsArray types, so we had to copy the names and > transitions over into a List. But the code is almost the same if we > eliminate the copy and just use the JsArrayInteger and JsArrayString > types, so let's do that! > > Here's the updated patch, which eliminates some array copying and > includes the new constants. > > On Thu, Sep 11, 2008 at 6:51 PM, Shanjian Li <[EMAIL PROTECTED]> wrote: > > Alex, > > > > This is the constant file for English. > > > > Btw, I read through your modification. We could remove the conversion of > > transition array and use it the way it is (as 1-dimensional array instead > of > > convert it to 2 arrays). I did that for the purpose of readability. But > such > > conversion suffer from performance, which is the purpose of the > introduction > > of JSO. > > -- > Alex Rudnick > swe, gwt, atl > --~--~---------~--~----~------------~-------~--~----~ http://groups.google.com/group/Google-Web-Toolkit-Contributors -~----------~----~----~----~------~----~------~--~---
