Skip to content
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.

Sign up for GitHub

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

Fix java date parsing to be compatible with joda #36155

Merged
merged 3 commits into from
Dec 3, 2018
Merged

Fix java date parsing to be compatible with joda #36155

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0692975
Fix java date parsing to be compatible with joda
pgomulka Dec 3, 2018
88915f1
test cleanup
pgomulka Dec 3, 2018
2650241
unused import
pgomulka Dec 3, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion server/src/main/java/org/elasticsearch/common/time/DateFormatters.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,13 +85,13 @@ public class DateFormatters {
.optionalStart()
.appendFraction(MILLI_OF_SECOND, 3, 3, true)
.optionalEnd()
.optionalEnd()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is just moving the zoneId below the optional section of seconds/milliseconds. Hard to see with this formatting

.optionalStart()
.appendZoneOrOffsetId()
.optionalEnd()
.optionalEnd()
.optionalEnd()
.optionalEnd()
.optionalEnd()
.toFormatter(Locale.ROOT);

private static final DateTimeFormatter STRICT_DATE_OPTIONAL_TIME_FORMATTER_2 = new DateTimeFormatterBuilder()
Expand Down
23 changes: 23 additions & 0 deletions server/src/test/java/org/elasticsearch/common/joda/JavaJodaTimeDuellingTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
import java.util.Locale;

import static org.hamcrest.Matchers.containsString;
import static org.hamcrest.Matchers.equalTo;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unused import

import static org.hamcrest.Matchers.is;
import static org.hamcrest.Matchers.startsWith;

Expand Down Expand Up @@ -390,6 +391,28 @@ public void testDuelingStrictParsing() {

assertSameDate("2012-W31-5", "strict_weekyear_week_day");
assertParseException("2012-W1-1", "strict_weekyear_week_day");
//failing below
assertSameDate("2015-01-04T00:00Z", "strict_date_optional_time||epoch_millis");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you just use strict_date_optional_time and remove epoch_millis? Also, can yo move this to the other strict_date_optional_time checks?

Can you also move this strict_date_optional_time checks to the others in this method?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

absolutely - will clean this up

}
//Failing
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will delete these if the PR is ok, but it helps investigating

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for sure I'm fine with the PR in general! :-)

public void testFailingParseJavaTime(){
DateFormatter formatter = DateFormatters.forPattern("strict_date_optional_time||epoch_millis");
TemporalAccessor parse = formatter.parse("2015-01-04T00:00Z");
}
//Passing
public void testParsingJoda(){
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these two methods are not needed anymore, correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these are not needed. I only left them if you wanted to verify that quickly on your own

FormatDateTimeFormatter formatter = Joda.forPattern("strict_date_optional_time||epoch_millis");
DateTime dateTime = formatter.parser().parseDateTime("2015-01-04T00:00Z");
assertThat(dateTime,
equalTo(new DateTime()
.withZone(DateTimeZone.UTC)
.withYear(2015)
.withMonthOfYear(1)
.withDayOfMonth(4)
.withHourOfDay(0)
.withMinuteOfHour(0)
.withMillisOfDay(0)
));
}

public void testSamePrinterOutput() {
Expand Down