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

Jump to bottom

Add toLocalDate to CalendarUtils #725

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Add toLocalDate to CalendarUtils #725

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
4b73a09
Add toLocalDate to CalendarUtils
Mar 2, 2021
beed197
typo
asgh Mar 2, 2021
961a9ff
use month names instead of numbers
asgh Mar 2, 2021
9b98f78
Add toLocalDate to CalendarUtils
Mar 2, 2021
d4400ed
typo
asgh Mar 2, 2021
acaa506
use month names instead of numbers
asgh Mar 2, 2021
c08d525
use existing constant
Aug 21, 2024
f28e526
Adjusted comment wording
Aug 21, 2024
89073a2
typo, and some of my earlier fixes somehow got lost, so put them back
Aug 21, 2024
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
9 changes: 9 additions & 0 deletions src/main/java/org/apache/commons/lang3/time/CalendarUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@

package org.apache.commons.lang3.time;

import java.time.LocalDate;
import java.time.LocalDateTime;
import java.time.OffsetDateTime;
import java.time.ZoneId;
Expand Down Expand Up @@ -223,4 +224,12 @@ public ZonedDateTime toZonedDateTime() {
return toZonedDateTime(calendar);
}

/** Converts this instance to a {@link LocalDate}.
*
* @return a LocalDate
* @since 3.17.0
*/
public LocalDate toLocalDate() {
return LocalDate.of(getYear(), getMonth() + 1, getDayOfMonth());
}
}
11 changes: 11 additions & 0 deletions src/test/java/org/apache/commons/lang3/time/CalendarUtilsTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,9 @@

import static org.junit.jupiter.api.Assertions.assertEquals;

import java.time.LocalDate;
import java.time.LocalDateTime;
import java.time.Month;
import java.time.OffsetDateTime;
import java.time.ZoneId;
import java.time.ZonedDateTime;
Expand All @@ -29,6 +31,7 @@
import java.util.TimeZone;

import org.apache.commons.lang3.AbstractLangTest;
import org.apache.commons.lang3.time.TimeZones;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.MethodSource;
Expand Down Expand Up @@ -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
Copy link
Member

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!

Copy link
Author

@asgh asgh Aug 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@garydgregory

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.

Copy link
Member

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());
    }

?
See CalendarUtilsTest.testToLocalDate().

public void testToLocalDate() {
Calendar calendar = new GregorianCalendar(TimeZone.getTimeZone(TimeZones.GMT_ID));
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());
}
}