Issue 13080044: stricter adherence to RFC3339 standards for date/time parsing (Closed)
Description1. Restricted milliseconds to 3 digits which wasn't consistent with RFC3339 standards, e.g. "1937-01-01T12:00:27.87+00:20" was not parsed as it should have been.
2. Wasn't strict enough with formatting:
A. Negative values allowed, e.g. "1937--3-55T12:00:27+00:20" was parsed with a month value of -3.
B. Didn't throw exceptions when it could parse a portion but not all of the user's input, e.g. "2013-01-01 09:00:02" was parsed as "2013-01-01" (date only).
Patch Set 1 #Patch Set 2 : fixed a little formatting #
Total comments: 12
Patch Set 3 : fixed formatting based on Eyal's comments #
Total comments: 20
Patch Set 4 : fixed Yaniv's comments and title/description #Patch Set 5 : changed javadoc on parseRfc3339() to reference throwing an IllegalArgumentException instead #
Total comments: 8
Patch Set 6 : fixed Yaniv's 2nd set of comments #
Total comments: 6
Patch Set 7 : changes based on Nick's comments #
Total comments: 32
Patch Set 8 : changed javadoc/comments according to nick's suggestion #Patch Set 9 : fixed Yaniv's comments #
Total comments: 10
Patch Set 10 : removed TODO #Patch Set 11 : fixed Yaniv's final comments #
MessagesTotal messages: 16
First review... I didn't check the logic, I did check only the style. Feel free to ping me if one of those comments isn't clear. Thanks, Eyal https://codereview.appspot.com/13080044/diff/3001/google-http-client/src/main... File google-http-client/src/main/java/com/google/api/client/util/DateTime.java (right): https://codereview.appspot.com/13080044/diff/3001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/util/DateTime.java:250: * @author mnrubin Parses an RFC 3339 date/time value using regex groups. Please remove author tag. no need that tag here. You are just changing fixing a bug, if it was a new file \ method maybe it was appropriate, but when you fix a bug you shouldn't add that tag. https://codereview.appspot.com/13080044/diff/3001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/util/DateTime.java:252: * <p> check your indentation - there shouldn't be a change in this paragraph https://codereview.appspot.com/13080044/diff/3001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/util/DateTime.java:267: Pattern pattern = Pattern.compile(regex); I think it's better to have a static Pattern which you will compile only once. There is no need to compile it over and over again for each call https://codereview.appspot.com/13080044/diff/3001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/util/DateTime.java:287: milliseconds = Integer.parseInt(matcher.group(8).substring(1)); [optional] maybe it will be clearer if you save 8 and 9 as some consts. Right now it's hard to understand those "magic" numbers. There are other magic number that I see here (5,6,7 and later on 10, 12 and 13) https://codereview.appspot.com/13080044/diff/3001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/util/DateTime.java:300: tzShift = Integer.parseInt(matcher.group(12)) * 60 + Integer.parseInt(matcher.group(13)); ditto. 12, 13? what are those groups means? https://codereview.appspot.com/13080044/diff/3001/google-http-client/src/test... File google-http-client/src/test/java/com/google/api/client/util/DateTimeTest.java (right): https://codereview.appspot.com/13080044/diff/3001/google-http-client/src/test... google-http-client/src/test/java/com/google/api/client/util/DateTimeTest.java:125: remove empty line
Sign in to reply to this message.
Fixed re: Eyal's style comments https://codereview.appspot.com/13080044/diff/3001/google-http-client/src/main... File google-http-client/src/main/java/com/google/api/client/util/DateTime.java (right): https://codereview.appspot.com/13080044/diff/3001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/util/DateTime.java:250: * @author mnrubin Parses an RFC 3339 date/time value using regex groups. On 2013/08/24 17:04:41, peleyal wrote: > Please remove author tag. no need that tag here. You are just changing fixing a > bug, if it was a new file \ method maybe it was appropriate, but when you fix a > bug you shouldn't add that tag. Done. https://codereview.appspot.com/13080044/diff/3001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/util/DateTime.java:252: * <p> On 2013/08/24 17:04:41, peleyal wrote: > check your indentation - there shouldn't be a change in this paragraph Done. https://codereview.appspot.com/13080044/diff/3001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/util/DateTime.java:267: Pattern pattern = Pattern.compile(regex); On 2013/08/24 17:04:41, peleyal wrote: > I think it's better to have a static Pattern which you will compile only once. > There is no need to compile it over and over again for each call Done. https://codereview.appspot.com/13080044/diff/3001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/util/DateTime.java:287: milliseconds = Integer.parseInt(matcher.group(8).substring(1)); On 2013/08/24 17:04:41, peleyal wrote: > [optional] > maybe it will be clearer if you save 8 and 9 as some consts. Right now it's hard > to understand those "magic" numbers. > There are other magic number that I see here (5,6,7 and later on 10, 12 and 13) Done. https://codereview.appspot.com/13080044/diff/3001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/util/DateTime.java:300: tzShift = Integer.parseInt(matcher.group(12)) * 60 + Integer.parseInt(matcher.group(13)); On 2013/08/24 17:04:41, peleyal wrote: > ditto. 12, 13? what are those groups means? Done. https://codereview.appspot.com/13080044/diff/3001/google-http-client/src/test... File google-http-client/src/test/java/com/google/api/client/util/DateTimeTest.java (right): https://codereview.appspot.com/13080044/diff/3001/google-http-client/src/test... google-http-client/src/test/java/com/google/api/client/util/DateTimeTest.java:125: On 2013/08/24 17:04:41, peleyal wrote: > remove empty line Done.
Sign in to reply to this message.
Please fix the title and description of this changeset. Title should be a single short sentence that summarizes what is being fixed. The description should be detailed explanation. The unit tests should be able to fully cover at least the specific bugs being fixed. https://codereview.appspot.com/13080044/diff/9001/google-http-client/src/main... File google-http-client/src/main/java/com/google/api/client/util/DateTime.java (right): https://codereview.appspot.com/13080044/diff/9001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/util/DateTime.java:44: private static final String regex = "^([0-9]{4})-([0-9]{2})-([0-9]{2})" // yyyy-MM-dd constants need to be in ALL_CAPS also give it a more specific name like RFC3999_REGEX similarly for pattern https://codereview.appspot.com/13080044/diff/9001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/util/DateTime.java:44: private static final String regex = "^([0-9]{4})-([0-9]{2})-([0-9]{2})" // yyyy-MM-dd is there a reason why 'y' is lowercase but 'M' is uppercase? https://codereview.appspot.com/13080044/diff/9001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/util/DateTime.java:45: + "([Tt]([0-9]{2}):([0-9]{2}):([0-9]{2})(\\.[0-9]+)?)?" // HH:mm:ss.SSSSSS missing 'T' in comment https://codereview.appspot.com/13080044/diff/9001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/util/DateTime.java:46: + "([Zz]|([+-])([0-9]{2}):([0-9]{2}))?"; // Z or time zone shift HH:mm maybe better to put 'Z' in quotes to imply that it is a literal char https://codereview.appspot.com/13080044/diff/9001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/util/DateTime.java:257: * Parses an RFC 3339 date/time value using regex groups. See {@link Pattern} and {@link Matcher}. these are implementation details that don't belong in the JavaDoc comment. Instead I recommend you replace it with an "upgrade warning" that specifies that the parsing has become more strict starting with version 1.17 to enforce that only valid RFC3339 strings are entered, and if not throw a NumberFormatException. https://codereview.appspot.com/13080044/diff/9001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/util/DateTime.java:262: * </p> @throws NumberFormatException ... https://codereview.appspot.com/13080044/diff/9001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/util/DateTime.java:266: throw new NumberFormatException("Invalid date/time format: " + str); perhaps IllegalArgumentException is a better exception class to throw because it's not a number that is being entered if you agree, then please switch to using Preconditions.checkArgument() https://codereview.appspot.com/13080044/diff/9001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/util/DateTime.java:297: if (matcher.group(9) != null && matcher.group(9).length() != 0) { //Z or timezone shift HH:mm make a local variable for matcher.group(9) for readability since it is being accessed 3 times https://codereview.appspot.com/13080044/diff/9001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/util/DateTime.java:312: } catch (StringIndexOutOfBoundsException e) { I assume we don't need to catch this any more? stated another way: is it possible to construct a case where this gets hit even though it matched out Pattern? if so, we need to validate such a case with a unit test https://codereview.appspot.com/13080044/diff/9001/google-http-client/src/test... File google-http-client/src/test/java/com/google/api/client/util/DateTimeTest.java (right): https://codereview.appspot.com/13080044/diff/9001/google-http-client/src/test... google-http-client/src/test/java/com/google/api/client/util/DateTimeTest.java:99: expectExceptionForParseRfc3339("1937--3-55T12:00:27+00:20"); //invalid month single space after "//"
Sign in to reply to this message.
> Please fix the title and description of this changeset. > Title should be a single short sentence that summarizes > what is being fixed. The description should be detailed > explanation. The unit tests should be able to fully cover > at least the specific bugs being fixed. Changed the title/description. I have already included unit tests for the specific bugs that I am fixing. https://codereview.appspot.com/13080044/diff/9001/google-http-client/src/main... File google-http-client/src/main/java/com/google/api/client/util/DateTime.java (right): https://codereview.appspot.com/13080044/diff/9001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/util/DateTime.java:44: private static final String regex = "^([0-9]{4})-([0-9]{2})-([0-9]{2})" // yyyy-MM-dd On 2013/08/27 21:21:41, yanivi wrote: > is there a reason why 'y' is lowercase but 'M' is uppercase? I just used the pattern letters from the Java SimpleDateFormat class corresponding to the fields I was referencing ('m' means minute whereas 'M' means month, for example). https://codereview.appspot.com/13080044/diff/9001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/util/DateTime.java:44: private static final String regex = "^([0-9]{4})-([0-9]{2})-([0-9]{2})" // yyyy-MM-dd On 2013/08/27 21:21:41, yanivi wrote: > constants need to be in ALL_CAPS > > also give it a more specific name like RFC3999_REGEX > > similarly for pattern Done. I'll also update serialVersionUID accordingly https://codereview.appspot.com/13080044/diff/9001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/util/DateTime.java:45: + "([Tt]([0-9]{2}):([0-9]{2}):([0-9]{2})(\\.[0-9]+)?)?" // HH:mm:ss.SSSSSS On 2013/08/27 21:21:41, yanivi wrote: > missing 'T' in comment Done. https://codereview.appspot.com/13080044/diff/9001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/util/DateTime.java:46: + "([Zz]|([+-])([0-9]{2}):([0-9]{2}))?"; // Z or time zone shift HH:mm On 2013/08/27 21:21:41, yanivi wrote: > maybe better to put 'Z' in quotes to imply that it is a literal char Done. https://codereview.appspot.com/13080044/diff/9001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/util/DateTime.java:257: * Parses an RFC 3339 date/time value using regex groups. See {@link Pattern} and {@link Matcher}. On 2013/08/27 21:21:41, yanivi wrote: > these are implementation details that don't belong in the JavaDoc comment. > Instead I recommend you replace it with an "upgrade warning" that specifies that > the parsing has become more strict starting with version 1.17 to enforce that > only valid RFC3339 strings are entered, and if not throw a > NumberFormatException. Done. (IllegalArgumentException instead of NumberFormatException as below.) https://codereview.appspot.com/13080044/diff/9001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/util/DateTime.java:262: * </p> On 2013/08/27 21:21:41, yanivi wrote: > @throws NumberFormatException ... Done. Switched to Preconditions.checkArgument() (see below) so I'm using @throws IllegalArgumentException annotation. https://codereview.appspot.com/13080044/diff/9001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/util/DateTime.java:266: throw new NumberFormatException("Invalid date/time format: " + str); On 2013/08/27 21:21:41, yanivi wrote: > perhaps IllegalArgumentException is a better exception class to throw because > it's not a number that is being entered > > if you agree, then please switch to using Preconditions.checkArgument() Done. https://codereview.appspot.com/13080044/diff/9001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/util/DateTime.java:297: if (matcher.group(9) != null && matcher.group(9).length() != 0) { //Z or timezone shift HH:mm On 2013/08/27 21:21:41, yanivi wrote: > make a local variable for matcher.group(9) for readability since it is being > accessed 3 times Done. https://codereview.appspot.com/13080044/diff/9001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/util/DateTime.java:312: } catch (StringIndexOutOfBoundsException e) { On 2013/08/27 21:21:41, yanivi wrote: > I assume we don't need to catch this any more? > > stated another way: is it possible to construct a case where this gets hit even > though it matched out Pattern? if so, we need to validate such a case with a > unit test Done I removed it as I believe I preempted the possible StringIndexOutOfBoundsException`s. https://codereview.appspot.com/13080044/diff/9001/google-http-client/src/test... File google-http-client/src/test/java/com/google/api/client/util/DateTimeTest.java (right): https://codereview.appspot.com/13080044/diff/9001/google-http-client/src/test... google-http-client/src/test/java/com/google/api/client/util/DateTimeTest.java:99: expectExceptionForParseRfc3339("1937--3-55T12:00:27+00:20"); //invalid month On 2013/08/27 21:21:41, yanivi wrote: > single space after "//" Done.
Sign in to reply to this message.
I don't have time for a more thorough review before I go on vacation. I'll defer to Nick or Eyal to review while I'm gone, or wait until I return next week (though next week will be a short and busy week for me). https://codereview.appspot.com/13080044/diff/19001/google-http-client/src/mai... File google-http-client/src/main/java/com/google/api/client/util/DateTime.java (right): https://codereview.appspot.com/13080044/diff/19001/google-http-client/src/mai... google-http-client/src/main/java/com/google/api/client/util/DateTime.java:39: private static final long SERIAL_VERSION_UID = 1L; No, that one must be serialVersionUID by convention. For every rule, there is an exception :) https://codereview.appspot.com/13080044/diff/19001/google-http-client/src/mai... google-http-client/src/main/java/com/google/api/client/util/DateTime.java:257: * Parses an RFC 3339 date/time value. Upgrade warning: the parsing has become more strict move the upgrade warning to a separate paragraph and surround with <p> and </p> https://codereview.appspot.com/13080044/diff/19001/google-http-client/src/mai... google-http-client/src/main/java/com/google/api/client/util/DateTime.java:258: * starting with version 1.17 to enforce that only valid RFC3339 strings are entered, and if not, Probably too risky of a change to try to slip into 1.17 release this late into the release cycle. Let's put it into default 1.18 branch only. https://codereview.appspot.com/13080044/diff/19001/google-http-client/src/mai... google-http-client/src/main/java/com/google/api/client/util/DateTime.java:266: * @throws IllegalArgumentException should explanation of what scenario might throw such an exception
Sign in to reply to this message.
https://codereview.appspot.com/13080044/diff/19001/google-http-client/src/mai... File google-http-client/src/main/java/com/google/api/client/util/DateTime.java (right): https://codereview.appspot.com/13080044/diff/19001/google-http-client/src/mai... google-http-client/src/main/java/com/google/api/client/util/DateTime.java:39: private static final long SERIAL_VERSION_UID = 1L; On 2013/08/27 22:39:21, yanivi wrote: > No, that one must be serialVersionUID by convention. For every rule, there is > an exception :) Done. https://codereview.appspot.com/13080044/diff/19001/google-http-client/src/mai... google-http-client/src/main/java/com/google/api/client/util/DateTime.java:257: * Parses an RFC 3339 date/time value. Upgrade warning: the parsing has become more strict On 2013/08/27 22:39:21, yanivi wrote: > move the upgrade warning to a separate paragraph and surround with <p> and </p> Done. https://codereview.appspot.com/13080044/diff/19001/google-http-client/src/mai... google-http-client/src/main/java/com/google/api/client/util/DateTime.java:258: * starting with version 1.17 to enforce that only valid RFC3339 strings are entered, and if not, On 2013/08/27 22:39:21, yanivi wrote: > Probably too risky of a change to try to slip into 1.17 release this late into > the release cycle. Let's put it into default 1.18 branch only. Done. https://codereview.appspot.com/13080044/diff/19001/google-http-client/src/mai... google-http-client/src/main/java/com/google/api/client/util/DateTime.java:266: * @throws IllegalArgumentException On 2013/08/27 22:39:21, yanivi wrote: > should explanation of what scenario might throw such an exception Done.
Sign in to reply to this message.
Note the details on correctly running mvn tests. https://codereview.appspot.com/13080044/diff/24001/google-http-client/src/mai... File google-http-client/src/main/java/com/google/api/client/util/DateTime.java (right): https://codereview.appspot.com/13080044/diff/24001/google-http-client/src/mai... google-http-client/src/main/java/com/google/api/client/util/DateTime.java:273: public static DateTime parseRfc3339(String str) throws IllegalArgumentException { I don't think we need to make this a backwards-incompatible change. By widening the type of exception that is thrown from NumberFormatException to IllegalArgumentException, we are (as anyone who calls it now needs to change the exception they catch/throw, or it won't compile). Let's leave the exception type the same. Note that the tests currently fail because of this change, as they catch NumberFormatException. If you're running the tests solely through Eclipse, it won't work. Eclipse (for whatever reason) doesn't compile our tests, it just runs based on the last compiled class files. So if you replace your tests with AssertTrue(false), they'll still pass because it's running the old test files. Instead, you need to run it in maven first, then Eclipse will reflect it. So I usually do "mvn test", see if anything fails, and then if it does I'll run it again in Eclipse as it shows the stack trace better. https://codereview.appspot.com/13080044/diff/24001/google-http-client/src/mai... google-http-client/src/main/java/com/google/api/client/util/DateTime.java:280: int month = Integer.parseInt(matcher.group(2)) - 1; // MM Do we do any value checking? The spec has a section on "restrictions" in which it states the maximum reasonable values for month and date. https://codereview.appspot.com/13080044/diff/24001/google-http-client/src/mai... google-http-client/src/main/java/com/google/api/client/util/DateTime.java:283: boolean dateOnly = length <= 10 || Character.toUpperCase(str.charAt(10)) != 'T'; Do you really need the "or" here? Couldn't you just the length of the string, since we're now ensuring the format of the string using regex? If I'm reading the code correctly, you shouldn't be able to have a string longer than 10 characters without it having a 'T' follow it.
Sign in to reply to this message.
https://codereview.appspot.com/13080044/diff/24001/google-http-client/src/mai... File google-http-client/src/main/java/com/google/api/client/util/DateTime.java (right): https://codereview.appspot.com/13080044/diff/24001/google-http-client/src/mai... google-http-client/src/main/java/com/google/api/client/util/DateTime.java:273: public static DateTime parseRfc3339(String str) throws IllegalArgumentException { On 2013/08/29 15:08:28, ngmiceli wrote: > I don't think we need to make this a backwards-incompatible change. By widening > the type of exception that is thrown from NumberFormatException to > IllegalArgumentException, we are (as anyone who calls it now needs to change the > exception they catch/throw, or it won't compile). > Let's leave the exception type the same. > Note that the tests currently fail because of this change, as they catch > NumberFormatException. If you're running the tests solely through Eclipse, it > won't work. Eclipse (for whatever reason) doesn't compile our tests, it just > runs based on the last compiled class files. So if you replace your tests with > AssertTrue(false), they'll still pass because it's running the old test files. > Instead, you need to run it in maven first, then Eclipse will reflect it. > So I usually do "mvn test", see if anything fails, and then if it does I'll run > it again in Eclipse as it shows the stack trace better. Done. I changed it back, and ensured that all the tests (plus additional ones I added) pass. https://codereview.appspot.com/13080044/diff/24001/google-http-client/src/mai... google-http-client/src/main/java/com/google/api/client/util/DateTime.java:280: int month = Integer.parseInt(matcher.group(2)) - 1; // MM On 2013/08/29 15:08:28, ngmiceli wrote: > Do we do any value checking? The spec has a section on "restrictions" in which > it states the maximum reasonable values for month and date. As per our discussion, I'll leave this out for now and we'll focus on format checking rather than value checking. (It's easy to make sure the month value is between 1-12 for example, but what about day-of-month; it varies based on which month and whether or not it's a leap year.) https://codereview.appspot.com/13080044/diff/24001/google-http-client/src/mai... google-http-client/src/main/java/com/google/api/client/util/DateTime.java:283: boolean dateOnly = length <= 10 || Character.toUpperCase(str.charAt(10)) != 'T'; On 2013/08/29 15:08:28, ngmiceli wrote: > Do you really need the "or" here? Couldn't you just the length of the string, > since we're now ensuring the format of the string using regex? If I'm reading > the code correctly, you shouldn't be able to have a string longer than 10 > characters without it having a 'T' follow it. <Note: I am operating under the assumption that the rfc3339 standard does not allow a time zone shift to be specified unless a time is given too (as opposed to date-only). I am unable to find a precise yes or no answer to this question anywhere, including in the standard itself which I've read many times.> I actually ended up refactoring some of the method (and renamed some variables) to make more sense and to clear up some bugs. For example, the old code would have allowed a date followed by a time zone shift (a "Z", for example), which the standard only allows when a Time is given as well, and would have considered it a dateOnly (which is ambiguously named as it's a date plus a time zone shift). Were I to have removed the 2nd condition after the OR, then it also would have gone through the regex but set dateOnly to false, which doesn't make sense either. Now I throw an exception for cases like that as it's against the standard (I think) and so we don't guess what the user meant.
Sign in to reply to this message.
https://codereview.appspot.com/13080044/diff/30001/google-http-client/src/mai... File google-http-client/src/main/java/com/google/api/client/util/DateTime.java (right): https://codereview.appspot.com/13080044/diff/30001/google-http-client/src/mai... google-http-client/src/main/java/com/google/api/client/util/DateTime.java:260: * Upgrade warning: the parsing has become more strict Hate to nitpick, but consistency is important. Try to follow the pattern: Upgrade warning: in prior version 1.XX, this method <blah blah>, but starting with 1.XY it <blah de blah>. The nice thing about this aside from reading consistently is it makes it easy to search for upgrade warnings as of a specific version. [optional] Also, I'd add another sentence or rephrase to be a little more precise about what wasn't RFC3339 compliant before. That's the important thing to developers - what behavior that they used to expect is changing. Simply saying that it's changing without saying how isn't quite as useful, IMO https://codereview.appspot.com/13080044/diff/30001/google-http-client/src/mai... google-http-client/src/main/java/com/google/api/client/util/DateTime.java:322: value -= tzShift * 60000L; [optional] comment to explain this math. Subtract timezone shift in milliseconds to get to GMT (or something)
Sign in to reply to this message.
https://codereview.appspot.com/13080044/diff/30001/google-http-client/src/mai... File google-http-client/src/main/java/com/google/api/client/util/DateTime.java (right): https://codereview.appspot.com/13080044/diff/30001/google-http-client/src/mai... google-http-client/src/main/java/com/google/api/client/util/DateTime.java:260: * Upgrade warning: the parsing has become more strict On 2013/09/03 22:08:57, ngmiceli wrote: > Hate to nitpick, but consistency is important. Try to follow the pattern: > > Upgrade warning: in prior version 1.XX, this method <blah blah>, but starting > with 1.XY it <blah de blah>. > > The nice thing about this aside from reading consistently is it makes it easy to > search for upgrade warnings as of a specific version. > > [optional] Also, I'd add another sentence or rephrase to be a little more > precise about what wasn't RFC3339 compliant before. That's the important thing > to developers - what behavior that they used to expect is changing. Simply > saying that it's changing without saying how isn't quite as useful, IMO Done. https://codereview.appspot.com/13080044/diff/30001/google-http-client/src/mai... google-http-client/src/main/java/com/google/api/client/util/DateTime.java:322: value -= tzShift * 60000L; On 2013/09/03 22:08:57, ngmiceli wrote: > [optional] comment to explain this math. Subtract timezone shift in milliseconds > to get to GMT (or something) Done.
Sign in to reply to this message.
https://codereview.appspot.com/13080044/diff/30001/google-http-client/src/mai... File google-http-client/src/main/java/com/google/api/client/util/DateTime.java (right): https://codereview.appspot.com/13080044/diff/30001/google-http-client/src/mai... google-http-client/src/main/java/com/google/api/client/util/DateTime.java:43: /** Regular expression for parsing RFC3339 date/times */ google-http-client/src/main/java/com/google/api/client/util/DateTime.java:43: warning: First sentence should end with a period. https://codereview.appspot.com/13080044/diff/30001/google-http-client/src/mai... google-http-client/src/main/java/com/google/api/client/util/DateTime.java:44: private static final String RFC3339_REGEX = "^([0-9]{4})-([0-9]{2})-([0-9]{2})" // yyyy-MM-dd I wouldn't bother to declare the RFC3339_REGEX. we shouldn't be using it directly anywhere. https://codereview.appspot.com/13080044/diff/30001/google-http-client/src/mai... google-http-client/src/main/java/com/google/api/client/util/DateTime.java:44: private static final String RFC3339_REGEX = "^([0-9]{4})-([0-9]{2})-([0-9]{2})" // yyyy-MM-dd \\d instead of [0-9] https://codereview.appspot.com/13080044/diff/30001/google-http-client/src/mai... google-http-client/src/main/java/com/google/api/client/util/DateTime.java:48: private static final Pattern PATTERN = Pattern.compile(RFC3339_REGEX); RFC3339_PATTERN https://codereview.appspot.com/13080044/diff/30001/google-http-client/src/mai... google-http-client/src/main/java/com/google/api/client/util/DateTime.java:135: * @param value an <a href='http://tools.ietf.org/html/rfc3339'>RFC 3339</a> date/time value. upgrade warning also goes here https://codereview.appspot.com/13080044/diff/30001/google-http-client/src/mai... google-http-client/src/main/java/com/google/api/client/util/DateTime.java:139: // TODO(rmistry): Move the implementation of parseRfc3339 into this constructor. Implementation while we are here, please also take care of this TODO https://codereview.appspot.com/13080044/diff/30001/google-http-client/src/mai... google-http-client/src/main/java/com/google/api/client/util/DateTime.java:262: * it throws an NumberFormatException. {@link NumberFormatException} https://codereview.appspot.com/13080044/diff/30001/google-http-client/src/mai... google-http-client/src/main/java/com/google/api/client/util/DateTime.java:271: * @throws NumberFormatException if {@code str} doesn't match the RFC3339 standard format; please ctrl-shift-F in Eclipse to auto-format the code https://codereview.appspot.com/13080044/diff/30001/google-http-client/src/mai... google-http-client/src/main/java/com/google/api/client/util/DateTime.java:275: public static DateTime parseRfc3339(String str) throws NumberFormatException { can you please compare this method implementation with the implementation that we had for GData? https://code.google.com/p/gdata-java-client/source/browse/trunk/java/src/com/... https://codereview.appspot.com/13080044/diff/30001/google-http-client/src/mai... google-http-client/src/main/java/com/google/api/client/util/DateTime.java:276: if (!str.matches(RFC3339_REGEX)) { remove this line. see below... https://codereview.appspot.com/13080044/diff/30001/google-http-client/src/mai... google-http-client/src/main/java/com/google/api/client/util/DateTime.java:280: matcher.matches(); this should just be if(matcher.matches()) { throw new NumberFormatException(...);} https://codereview.appspot.com/13080044/diff/30001/google-http-client/src/mai... google-http-client/src/main/java/com/google/api/client/util/DateTime.java:294: if(isTzShiftGiven && !isTimeGiven) { google-http-client/src/main/java/com/google/api/client/util/DateTime.java:294:7: 'if' is not followed by whitespace. https://codereview.appspot.com/13080044/diff/30001/google-http-client/src/tes... File google-http-client/src/test/java/com/google/api/client/util/DateTimeTest.java (right): https://codereview.appspot.com/13080044/diff/30001/google-http-client/src/tes... google-http-client/src/test/java/com/google/api/client/util/DateTimeTest.java:37: TimeZone.setDefault(TimeZone.getTimeZone("GMT-4")); while you are here, it would be nice to add set up and tear down methods that get the current time zone and reset back to original time zone, respectively. https://codereview.appspot.com/13080044/diff/30001/google-http-client/src/tes... google-http-client/src/test/java/com/google/api/client/util/DateTimeTest.java:110: assertFalse(value.isDateOnly()); should we assert the value of getValue()?
Sign in to reply to this message.
(note: Yaniv submitted comments while I was fixing Nick's, so one may have to look back 2 versions prior to get a good sense of changes / how Yaniv's comments were addressed.) https://codereview.appspot.com/13080044/diff/30001/google-http-client/src/mai... File google-http-client/src/main/java/com/google/api/client/util/DateTime.java (right): https://codereview.appspot.com/13080044/diff/30001/google-http-client/src/mai... google-http-client/src/main/java/com/google/api/client/util/DateTime.java:43: /** Regular expression for parsing RFC3339 date/times */ On 2013/09/04 14:13:48, yanivi wrote: > google-http-client/src/main/java/com/google/api/client/util/DateTime.java:43: > warning: First sentence should end with a period. Done. https://codereview.appspot.com/13080044/diff/30001/google-http-client/src/mai... google-http-client/src/main/java/com/google/api/client/util/DateTime.java:44: private static final String RFC3339_REGEX = "^([0-9]{4})-([0-9]{2})-([0-9]{2})" // yyyy-MM-dd On 2013/09/04 14:13:48, yanivi wrote: > I wouldn't bother to declare the RFC3339_REGEX. we shouldn't be using it > directly anywhere. Done. https://codereview.appspot.com/13080044/diff/30001/google-http-client/src/mai... google-http-client/src/main/java/com/google/api/client/util/DateTime.java:44: private static final String RFC3339_REGEX = "^([0-9]{4})-([0-9]{2})-([0-9]{2})" // yyyy-MM-dd On 2013/09/04 14:13:48, yanivi wrote: > \\d instead of [0-9] Done. https://codereview.appspot.com/13080044/diff/30001/google-http-client/src/mai... google-http-client/src/main/java/com/google/api/client/util/DateTime.java:48: private static final Pattern PATTERN = Pattern.compile(RFC3339_REGEX); On 2013/09/04 14:13:48, yanivi wrote: > RFC3339_PATTERN Done. https://codereview.appspot.com/13080044/diff/30001/google-http-client/src/mai... google-http-client/src/main/java/com/google/api/client/util/DateTime.java:135: * @param value an <a href='http://tools.ietf.org/html/rfc3339'>RFC 3339</a> date/time value. On 2013/09/04 14:13:48, yanivi wrote: > upgrade warning also goes here Done. https://codereview.appspot.com/13080044/diff/30001/google-http-client/src/mai... google-http-client/src/main/java/com/google/api/client/util/DateTime.java:139: // TODO(rmistry): Move the implementation of parseRfc3339 into this constructor. Implementation On 2013/09/04 14:13:48, yanivi wrote: > while we are here, please also take care of this TODO Is it really better to have what's currently the body of the parseRfc3339() method in this constructor instead, and have the parseRfc3339() simply return the constructor? I would think that abstracting the parsing to the parseRfc3339() method while keeping the constructor minimal is better. https://codereview.appspot.com/13080044/diff/30001/google-http-client/src/mai... google-http-client/src/main/java/com/google/api/client/util/DateTime.java:262: * it throws an NumberFormatException. On 2013/09/04 14:13:48, yanivi wrote: > {@link NumberFormatException} Done. https://codereview.appspot.com/13080044/diff/30001/google-http-client/src/mai... google-http-client/src/main/java/com/google/api/client/util/DateTime.java:271: * @throws NumberFormatException if {@code str} doesn't match the RFC3339 standard format; On 2013/09/04 14:13:48, yanivi wrote: > please ctrl-shift-F in Eclipse to auto-format the code Done. https://codereview.appspot.com/13080044/diff/30001/google-http-client/src/mai... google-http-client/src/main/java/com/google/api/client/util/DateTime.java:275: public static DateTime parseRfc3339(String str) throws NumberFormatException { On 2013/09/04 14:13:48, yanivi wrote: > can you please compare this method implementation with the implementation that > we had for GData? > > https://code.google.com/p/gdata-java-client/source/browse/trunk/java/src/com/... Done. https://codereview.appspot.com/13080044/diff/30001/google-http-client/src/mai... google-http-client/src/main/java/com/google/api/client/util/DateTime.java:276: if (!str.matches(RFC3339_REGEX)) { On 2013/09/04 14:13:48, yanivi wrote: > remove this line. see below... Done. https://codereview.appspot.com/13080044/diff/30001/google-http-client/src/mai... google-http-client/src/main/java/com/google/api/client/util/DateTime.java:280: matcher.matches(); On 2013/09/04 14:13:48, yanivi wrote: > this should just be if(matcher.matches()) { throw new > NumberFormatException(...);} Done. https://codereview.appspot.com/13080044/diff/30001/google-http-client/src/mai... google-http-client/src/main/java/com/google/api/client/util/DateTime.java:294: if(isTzShiftGiven && !isTimeGiven) { On 2013/09/04 14:13:48, yanivi wrote: > google-http-client/src/main/java/com/google/api/client/util/DateTime.java:294:7: > 'if' is not followed by whitespace. Done. https://codereview.appspot.com/13080044/diff/30001/google-http-client/src/tes... File google-http-client/src/test/java/com/google/api/client/util/DateTimeTest.java (right): https://codereview.appspot.com/13080044/diff/30001/google-http-client/src/tes... google-http-client/src/test/java/com/google/api/client/util/DateTimeTest.java:37: TimeZone.setDefault(TimeZone.getTimeZone("GMT-4")); On 2013/09/04 14:13:48, yanivi wrote: > while you are here, it would be nice to add set up and tear down methods that > get the current time zone and reset back to original time zone, respectively. Done. https://codereview.appspot.com/13080044/diff/30001/google-http-client/src/tes... google-http-client/src/test/java/com/google/api/client/util/DateTimeTest.java:110: assertFalse(value.isDateOnly()); On 2013/09/04 14:13:48, yanivi wrote: > should we assert the value of getValue()? Done.
Sign in to reply to this message.
removed TODO
Sign in to reply to this message.
LGTM (but please fix the minor issues below first) https://codereview.appspot.com/13080044/diff/40001/google-http-client/src/mai... File google-http-client/src/main/java/com/google/api/client/util/DateTime.java (right): https://codereview.appspot.com/13080044/diff/40001/google-http-client/src/mai... google-http-client/src/main/java/com/google/api/client/util/DateTime.java:45: "^([\\d]{4})-([\\d]{2})-([\\d]{2})" // yyyy-MM-dd remove the brackets from "\\d", e.g.: "^(\\d{4})-(\\d{2})-(\\d{2})" // yyyy-MM-dd https://codereview.appspot.com/13080044/diff/40001/google-http-client/src/mai... google-http-client/src/main/java/com/google/api/client/util/DateTime.java:288: if (str == null || !matcher.matches()) { remove str == null part since NullPointerException is expected, which hopefully matches already does https://codereview.appspot.com/13080044/diff/40001/google-http-client/src/tes... File google-http-client/src/test/java/com/google/api/client/util/DateTimeTest.java (right): https://codereview.appspot.com/13080044/diff/40001/google-http-client/src/tes... google-http-client/src/test/java/com/google/api/client/util/DateTimeTest.java:29: TimeZone defaultTimeZone; use the word "original" to make it more clear, e.g. "orignalTimeZone" https://codereview.appspot.com/13080044/diff/40001/google-http-client/src/tes... google-http-client/src/test/java/com/google/api/client/util/DateTimeTest.java:29: TimeZone defaultTimeZone; private https://codereview.appspot.com/13080044/diff/40001/google-http-client/src/tes... google-http-client/src/test/java/com/google/api/client/util/DateTimeTest.java:38: /* (non-Javadoc) remove noisy and unhelpful lines 38-40 similarly below for tearDown
Sign in to reply to this message.
committing https://codereview.appspot.com/13080044/diff/40001/google-http-client/src/mai... File google-http-client/src/main/java/com/google/api/client/util/DateTime.java (right): https://codereview.appspot.com/13080044/diff/40001/google-http-client/src/mai... google-http-client/src/main/java/com/google/api/client/util/DateTime.java:45: "^([\\d]{4})-([\\d]{2})-([\\d]{2})" // yyyy-MM-dd On 2013/09/04 20:47:53, yanivi wrote: > remove the brackets from "\\d", e.g.: > "^(\\d{4})-(\\d{2})-(\\d{2})" // yyyy-MM-dd Done. https://codereview.appspot.com/13080044/diff/40001/google-http-client/src/mai... google-http-client/src/main/java/com/google/api/client/util/DateTime.java:288: if (str == null || !matcher.matches()) { On 2013/09/04 20:47:53, yanivi wrote: > remove str == null part since NullPointerException is expected, which hopefully > matches already does Done. https://codereview.appspot.com/13080044/diff/40001/google-http-client/src/tes... File google-http-client/src/test/java/com/google/api/client/util/DateTimeTest.java (right): https://codereview.appspot.com/13080044/diff/40001/google-http-client/src/tes... google-http-client/src/test/java/com/google/api/client/util/DateTimeTest.java:29: TimeZone defaultTimeZone; On 2013/09/04 20:47:53, yanivi wrote: > private Done. https://codereview.appspot.com/13080044/diff/40001/google-http-client/src/tes... google-http-client/src/test/java/com/google/api/client/util/DateTimeTest.java:29: TimeZone defaultTimeZone; On 2013/09/04 20:47:53, yanivi wrote: > use the word "original" to make it more clear, e.g. "orignalTimeZone" Done. https://codereview.appspot.com/13080044/diff/40001/google-http-client/src/tes... google-http-client/src/test/java/com/google/api/client/util/DateTimeTest.java:38: /* (non-Javadoc) On 2013/09/04 20:47:53, yanivi wrote: > remove noisy and unhelpful lines 38-40 > > similarly below for tearDown Done.
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
