-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Add toLocalDate to CalendarUtils #725
base: master
Are you sure you want to change the base?
Conversation
The failing tests appear to be from other code, not the new code in the pull request. |
Would you please rebase on master? |
Done. |
@asgh |
I don't have mvn on this computer, I hope my last commit fixed things, if not I'll have to work on it a little later. |
src/test/java/org/apache/commons/lang3/time/CalendarUtilsTest.java
Outdated
Show resolved
Hide resolved
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 not sure the implementation is correct. The following fails with this PR:
@ParameterizedTest
@MethodSource("java.util.TimeZone#getAvailableIDs()")
public void testToLocalDate(final String id) {
final TimeZone timeZone = TimeZone.getTimeZone(id);
final Calendar calendar = new GregorianCalendar(timeZone);
// sanity check
assertEquals(LocalDateTime.ofInstant(calendar.toInstant(), timeZone.toZoneId()).toLocalDate(), new CalendarUtils(calendar).toLocalDate());
// actual test
calendar.setTimeInMillis(-27078001200000L);
assertEquals(LocalDate.of(1111, Month.DECEMBER, 1), new CalendarUtils(calendar).toLocalDate());
calendar.setTimeInMillis(1614700215000L);
assertEquals(LocalDate.of(2021, Month.MARCH, 2), new CalendarUtils(calendar).toLocalDate());
}
@asgh |
Calendar uses 0-11 and LocalDate uses 1-12, so use constants to avoid confusion.
Calendar uses 0-11 and LocalDate uses 1-12, so use constants to avoid confusion.
That's exactly why I added this! Dealing with local dates that cross timezones is a mess, LocalDate specifically has no timezone, the other way also does not correctly handle early dates. It's the other code that is incorrect. If you feel it's really important I can try to dive into the debugger and find where your pr goes wrong, but I already basically know where it will fail. You have to be careful about the timezone and the time you pick if you want to add such a test - if your local timezone, and GMT, have different dates for the time you picked (I assume current time), your test will fail. The point of this function is to create a LocalDate specifically using the current timezone. |
@@ -128,5 +131,13 @@ public void testToZonedDateTime(final String id) { | |||
final ZonedDateTime zdt1 = ZonedDateTime.of(1, 2, 3, 4, 5, 6, 0, zoneId); | |||
calendar.setTimeInMillis(zdt1.toInstant().toEpochMilli()); | |||
assertEquals(ZonedDateTime.ofInstant(zdt1.toInstant(), calendar.getTimeZone().toZoneId()), new CalendarUtils(calendar).toZonedDateTime()); | |||
|
|||
@Test |
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.
Hello @asgh,
Thank you for your reply and update.
I don't think the implementation is correct because it does not take into account the Calendar's time zone.
You should be able to see this if you test with time zones and adjust the test as in my example test using:
@ParameterizedTest
@MethodSource(TimeZonesTest.TIME_ZONE_GET_AVAILABLE_IDS)
See also the other tests in this class.
TY!
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 really not following you. For this to be wrong this.getDayOfMonth() would have to be wrong. Are you saying that function returns the wrong day of the month?
And the entire point is that LocalDate doesn't have a timezone. I'm only setting the timezone in the test so that it correctly converts the UnixTime into a Month and Day (otherwise it could vary +- a day depending on what timezone you pick).
If you look at the actual function in the code there is no timezone there are all.
I will run your assert with some sample data to try to explain.
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.
Hello @asgh
One of the reasons for confusion perhaps is that the test data does not look right.
How did you get from -27078001200000
to LocalDate.of(1111, Month.DECEMBER, 1)
when:
public void testToLocalDate() {
final Calendar calendar = new GregorianCalendar(TimeZone.getTimeZone(TimeZones.GMT_ID));
calendar.setTimeInMillis(-27078001200000L);
assertEquals("1111-12-08T05:00:00Z", calendar.toInstant().toString());
assertEquals(LocalDate.of(1111, Month.DECEMBER, 8), new CalendarUtils(calendar).toLocalDate());
calendar.setTimeInMillis(1614700215000L);
assertEquals(LocalDate.of(2021, Month.MARCH, 2), new CalendarUtils(calendar).toLocalDate());
}
Using an Instance to create a LocalDate from a Calendar does not work on dates before the Gregorian/Julian cutover.
So use this code instead, and add tests for years before the cutover and after.