Hey Rajeev :) Thanks for the review! Responses inline. The upshot: OK, fixed, committed r3655.
On Tue, Sep 16, 2008 at 2:48 PM, Rajeev Dayal <[EMAIL PROTECTED]> wrote: > 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. "data" is sort of a contentious word. Let's use "The values of the constants constants are..." and avoid the data-as-plural vs. data-as-mass-noun issue. > 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." Done! > 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? Good catch! Made it be UTF-8. We can discuss how to make non-en locales available for the next pass; I don't think it would be so bad to include them in the distribution, although it'd make our download size rather bigger. > TimeZone.java > 25-35: All instances of "initiated", "initiate", etc should be changed to > "instantiate", "instantiated", and so on OK. > 29: "TimeZoneConstants" could make this a javadoc ref to the > TimeZoneConstants class OK > 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 OK, and OK. > 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.." OK > 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." OK > 65:Cast to int is unnecessary here OK > 94: "This factory method provide a decent..." --> "This factory method > provides a decent.." OK > 95-96: Add a line break here. OK > 116: Seems like having "GMT" in the array is unnecessary; it can be > concatenated to the end result after the transformation step. I tried rewriting composeGMTString with offsetDisplay, but doing this properly wants to use String.format so we can get nice padding. Honestly, I think it's pretty expressive as is. > 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) I think these are just different enough where we can leave them as is -- factoring something more general out of them would make it messier. > 169: Not sure why this comment is here. Removed! > 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. Refactored the sentence. > 181: Should be the "date" to check. OK > 199: "The date for which time to retrieve GMT string." --> "The date from > which the time information should be extracted" OK > 200: "GMT String" --> "A GMT representation of the time given by the date" OK > 181-182,198-199: Add a line break here. OK > 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." OK > 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. OK > 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" Refactored the text a bit. > 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. OK > 242: This method is very similar to the composeGMTString(int) method. Can > you consolidate these two? (see earlier note...) > 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. OK > 258: Add the word "name" before the period. Add a line break before this > line as well. OK > 266: "To get the standard time zone offset in minutes." --> "Returns the > standard time zone offset in minutes." OK > 268: Need @return tag in javadoc OK > 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" OK > 275: "if daylight time in effect." --> "if daylight savings time is in > effect" OK > 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. OK > 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 OK. > 909: "count" parameter is not needed (not part of your patch) Removed! > 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) It's some pretty hairy code here; I'd be inclined to keep it. > 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" OK > TimeZoneInfo.java > 34: Add a line break here. > 35: Missing description of "json" parameter, missing @return tag OK > TimeZoneTest.java > Needs a sort; some methods are out of order. Sorted! > 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. Took out the @SuppressWarnings. > 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. Added! > TimeZoneInfoTest.java > Needs a sort; some methods are out of order. OK > 28: This does not need to be af field; could be a local variable. OK > 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. Set to text/xml and native. > 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 OK Committed r3655. -- Alex Rudnick swe, gwt, atl --~--~---------~--~----~------------~-------~--~----~ http://groups.google.com/group/Google-Web-Toolkit-Contributors -~----------~----~----~----~------~----~------~--~---
