-
Notifications
You must be signed in to change notification settings - Fork 54
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
Use zoned time when parse string #341
Use zoned time when parse string #341
Conversation
It would be better if it didn't depend on the hard-coded <server>.<database>.<table> template
…rse a time_string with an offset marker e.g. +03:00, -0130, Z. Also we can handle an exception when a dateTime string has invalid year e.g. in MSSQL "0000-00-01 00:00:00". Zero year value is invalid in java.
Maybe it can solve #215? |
if(parsingSuccesful == false) { | ||
|
||
if (parsingSuccesful) { | ||
if (setMinDate) { |
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.
If they are both true, why is there a need to have 2 variables?
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.
When we get invalid year exception we can handle it by setting the setMinDate flag. So we don't get a date by parsing, but use the setMinDate
flag to set it to minimum date. If this error occurs then we can safely assume that the date was not parsed due to zero year value
parsingSuccesful = true; | ||
break; | ||
} catch(Exception e) { | ||
if (e.getMessage().contains("Invalid value for YearOfEra")) { |
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.
Doesn't look like a good idea to check for exception using the message(Message could change in the future), isnt there a specific exception if parsing fails?
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.
OK, i will check.
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 have rewritten a bit this part. It's possible to use getCause()
method that inherited from Throwable
to define if it's either a DateTimeParseException
error or a DateTimeException
. If getCause()
returns null
- it's a parsing error, otherwise not.
Please add to this test case like this
|
if(splitName.length == 3) { | ||
tableName = splitName[2]; | ||
} | ||
tableName = splitName[splitName.length - 1]; |
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.
please remove these changes for topic.prefix
LocalDateTime zd = LocalDateTime.parse((String) value, formatter); | ||
result = zd.format(destFormatter); | ||
//result = removeTrailingZeros(result); | ||
parsedDT = ZonedDateTime.parse((String) value, formatter.withZone(ZoneId.of("UTC"))); | ||
parsingSuccesful = true; |
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.
nit, parsingSuccessful
This reverts commit e8cef17.
This was implemented as part of other PR's , closing this for now. |
I suggest to use ZonedDateTime instead of LocalDateTime. So we can parse a time_string with an offset marker e.g. +03:00, -0130, Z. Also we can handle an exception when a dateTime string has invalid year e.g. in MSSQL "0000-00-01 00:00:00". Zero year value is invalid in java.