Core: Fix Java Time DateFormatter printers #32592
Conversation
A bug in the test suite prevented to properly check that all date formatters printed the date the same way like joda time does. This fixes the test and thus also a fair share of formats, that basically use the strict parser for printing. Also, now all date parsers are using UTC as their default time zone.
spinscale
added
review
:Core/Infra/Core
|
Pinging @elastic/es-core-infra |
| return new CompoundDateTimeFormatterBuilder(); | ||
| } | ||
|
|
||
| static final class CompoundDateTimeFormatterBuilder { |
There was a problem hiding this comment.
Instead of creating another class, could the functionality here just be a couple private static methods on DateFormatters? If I understand these timezone formats correctly, the only reason we have any of this complexity is because of +HHmm, which is a non standard format. The other two would be handled by appendZoneOrOffsetId()? I think it would be good to keep this local to DateFormatters, so that adding more cases like this does not leak outside of that class.
There was a problem hiding this comment.
This looks ok, but it is extremely hard to read because of the tons of formatters/printers reused sometimes and not others. Could you please add a comment to the top of the class indicating what naming conventions are used? For example, with basic time, BASIC_TIME_PRINTER does not have time zone info, but BASIC_TIME_NO_MILLIS does. Additionally, as a general suggestion for things that would make changes here easier to review in the future, some comments within the file grouping the formatters would also be nice. For example, having all the "time" formatters grouped together with some large comment blocks. And even more helpful would be having a comment on each formatter with the string format equivalent.
| .append(OPTIONAL_TIME_ZONE_FORMATTER) | ||
| .optionalStart().appendZoneId().optionalEnd() | ||
| .optionalStart().appendOffset("+HHmm", "Z").optionalEnd() | ||
| .optionalStart().appendOffset("+HH:mm", "Z").optionalEnd() |
There was a problem hiding this comment.
Can you use appendZoneOrOffsetId() above, which includes this?
|
I removed the methods from the I also removed making all the formatters UTC by default, as there seem to be differences between java8 and 10 (java10 works as expected, java8 fails the tests in CI and local). |
|
I also added a small comment for each of the compound date time formatters now |
A bug in the test suite prevented to properly check that all date formatters printed the date the same way like joda time does. This fixes the test and thus also a fair share of formats, that now use the strict parser for printing.
A bug in the test suite prevented to properly check that all date formatters printed the date the same way like joda time does. This fixes the test and thus also a fair share of formats, that now use the strict parser for printing.
A bug in the test suite prevented to properly check that all date
formatters printed the date the same way like joda time does.
This fixes the test and thus also a fair share of formats, that
now use the strict parser for printing.
Also, now all date parsers are using UTC as their default time zone.