-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
iso 8601 date format #552
iso 8601 date format #552
Conversation
…MM-dd|yyyyMMdd][T(hh:mm[:ss[.sss]]|hhmm[ss[.sss]])]?[Z|[+-]hh:mm]]
} | ||
|
||
// extract timezone | ||
String timezoneId; | ||
if (date.length() <= offset) { | ||
throw new IndexOutOfBoundsException("No time zone indicator "); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it'd be best to use some more refined exception, like ParseException of IllegalArgumentException. I know it's behavioral change, but IndexOutOfBounds is sort of low-level errror.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm agree with you.
But it's not an IndexOutOfBounds that client see, it's a ParseException because of line 217/218.
I can replace the line 177 by:
throw new ParseException("Failed to parse date ["' + date + "']: No time zone indicator", pos.getIndex());
But it break a little the logic of this function,
Let me known what your prefer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah. Ok, so it is to keep handling working the way it did -- got it. I can see if I can tweak it after merge, but if it gets translated to different exception by code that is fine.
Sounds good to me; thank you for the contribution! One thing I will have to ask for before merging this is a filled Contributor License Agreement (CLA), from: https://github.com/FasterXML/jackson/blob/master/contributor-agreement.pdf so that we know it is safe to distribute code. We have to ask one of these from all contributors to Jackson project; but fortunately only once before the first contribution. If you could print, fill-in+sign and scan it, and send resulting image (pdf, png, jpeg whatever) to Apologies for this, but it is one (and only!) formality we need here. :) |
You're welcome, |
Cla send :) |
I think this change is incorrect for ISO dates without a time component. In the examples above, "20070813Z" is not a valid date-only value. There should be no time zone in a date-only value, since the value has no time component at all. Logically, it's that day in any time zone, and if it's being converted to a date/time instance, the conversion would always happen in the local time zone. |
@newyankeecodeshop I am not sure which way to go with this: while I can see your argument, it is also true that since in Java date/time values are typically stored as "milliseconds-since-epoch" notation, and thereby value of a date-only value may well change depending on timezone information. But the point about ISO-8601 spec and RFC would be valid; if they suggest use of timezone offset / 'Z' is not legal then it probably was not a good idea to allow it. And then again, if such usage is wide-spread, we have to balance inter-operability (of being more liberal in accepting values, conservative in what we output) and standards-compliancy. What do you think would be the right thing to do, considering that this behavior is included in 2.5.0? |
@cowtowncoder You're right that Java Date objects represent an instant in time. In my experience, to make date-only values work in that environment, they always have to be parsed in the local time, so that the Date instance will be midnight in the local time zone. Otherwise, when the date is formatted for display, it can possibly show as being in a different day. For example, since the American Eastern time zone is GMT-5:00 (or -4:00 without daylight savings), if a date-only value is parsed and turned that day at midnight GMT, when shown to the user in their time zone, it will be the previous day. I agree there is a semantic issue because a date-only value is being stored in a type that stores date+time values. This is probably a reason why we should encourage folks to use Joda LocalDate instead of j.u.Date in all cases, since it removes the uncertainty. At any rate, I think if Jackson has this feature, it should allow the 'Z' to be optional and treat the value as midnight in the local time zone. |
@newyankeecodeshop Fair enough. If missing |
Hello,
After reading several post about iso 8601 date format,( like this one http://www.w3.org/TR/NOTE-datetime for example), I think that it could be easier to parse iso 8601 date if we could use both "strict standard" and short format.
We could parse format according this:
[yyyy-MM-dd|yyyyMMdd][T(hh:mm[:ss[.sss]]|hhmm[ss[.sss]])]?[Z|[+-]hh:mm]]
For example:
2007-08-13T19:51:23.2Z
20070813T19:51:23Z
2007-08-13T195123Z
2007-08-13T19:51Z
20070813T19:51Z
20070813Z
I hope it can help a little,
Thanks for you're job, time
Best Regards
Jérôme