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

Calendar API changes so that methods accepting strings return strings #5029

Merged
merged 8 commits into from
Jan 12, 2024
Merged

Calendar API changes so that methods accepting strings return strings #5029

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
64076f9
Fix broken calendar unit tests over midnight.
chipkent Jan 10, 2024
a22292d
Spotless
chipkent Jan 10, 2024
8ba59b4
Addressed reviewer comments.
chipkent Jan 10, 2024
8cd97a4
Converted string input methods to return strings.
chipkent Jan 10, 2024
76164a1
TODOs that raise question.
chipkent Jan 10, 2024
18ed1fd
Merge remote-tracking branch 'origin/main' into calendar_string_to_st…
chipkent Jan 10, 2024
847acd6
Spotless
chipkent Jan 10, 2024
d7cee84
Remove todos.
chipkent Jan 11, 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
13 changes: 9 additions & 4 deletions engine/time/src/main/java/io/deephaven/time/DateTimeUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -433,8 +433,7 @@ private CachedCurrentDate(@NotNull final ZoneId timeZone) {
synchronized void update(final long currentTimeMillis) {
date = toLocalDate(epochMillisToInstant(currentTimeMillis), timeZone);
str = formatDate(date);
valueExpirationTimeMillis =
epochMillis(atMidnight(epochMillisToZonedDateTime(currentTimeMillis, timeZone)).plusDays(1));
valueExpirationTimeMillis = epochMillis(date.plusDays(1).atStartOfDay(timeZone));
}
}

Expand All @@ -450,6 +449,10 @@ public ZoneId getKey(final CACHED_DATE_TYPE cachedDate) {
private static final KeyedObjectHashMap<ZoneId, CachedCurrentDate> cachedCurrentDates =
new KeyedObjectHashMap<>(new CachedDateKey<>());

private static CachedCurrentDate getCachedCurrentDate(@NotNull final ZoneId timeZone) {
return cachedCurrentDates.putIfAbsent(timeZone, CachedCurrentDate::new);
}

/**
* Provides the current date string according to the {@link #currentClock() current clock}. Under most
* circumstances, this method will return the date according to current system time, but during replay simulations,
Expand All @@ -468,7 +471,7 @@ public static String today(@Nullable final ZoneId timeZone) {
return null;
}

return cachedCurrentDates.putIfAbsent(timeZone, CachedCurrentDate::new).getStr();
return getCachedCurrentDate(timeZone).getStr();
}

/**
Expand All @@ -485,6 +488,7 @@ public static String today(@Nullable final ZoneId timeZone) {
@ScriptApi
@NotNull
public static String today() {
// noinspection ConstantConditions
return today(DateTimeUtils.timeZone());
}

Expand All @@ -506,7 +510,7 @@ public static LocalDate todayLocalDate(@Nullable final ZoneId timeZone) {
return null;
}

return cachedCurrentDates.putIfAbsent(timeZone, CachedCurrentDate::new).getLocalDate();
return getCachedCurrentDate(timeZone).getLocalDate();
}

/**
Expand All @@ -523,6 +527,7 @@ public static LocalDate todayLocalDate(@Nullable final ZoneId timeZone) {
@ScriptApi
@NotNull
public static LocalDate todayLocalDate() {
// noinspection ConstantConditions
return todayLocalDate(DateTimeUtils.timeZone());
}

Expand Down
34 changes: 20 additions & 14 deletions engine/time/src/main/java/io/deephaven/time/calendar/BusinessCalendar.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1214,14 +1214,16 @@ public LocalDate[] businessDates(final LocalDate start, final LocalDate end, fin
* @throws InvalidDateException if the dates are not in the valid range
* @throws DateTimeUtils.DateTimeParseException if the string cannot be parsed
*/
public LocalDate[] businessDates(final String start, final String end, final boolean startInclusive,
public String[] businessDates(final String start, final String end, final boolean startInclusive,
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment as on the other overload.

Copy link
Member Author

Choose a reason for hiding this comment

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

I assumed that String in implies String out. If someone is working in strings, do you expect that they would not want this result in strings as well?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think String in -> String out, LocalDate in -> LocalDate out is the most user-friendly

final boolean endInclusive) {
if (start == null || end == null) {
return null;
}

return businessDates(DateTimeUtils.parseLocalDate(start), DateTimeUtils.parseLocalDate(end), startInclusive,
endInclusive);
final LocalDate[] dates =
businessDates(DateTimeUtils.parseLocalDate(start), DateTimeUtils.parseLocalDate(end), startInclusive,
endInclusive);
return dates == null ? null : Arrays.stream(dates).map(DateTimeUtils::formatDate).toArray(String[]::new);
}

/**
Expand Down Expand Up @@ -1287,7 +1289,7 @@ public LocalDate[] businessDates(final LocalDate start, final LocalDate end) {
* @throws InvalidDateException if the dates are not in the valid range
* @throws DateTimeUtils.DateTimeParseException if the string cannot be parsed
*/
public LocalDate[] businessDates(final String start, final String end) {
public String[] businessDates(final String start, final String end) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Every other businessDate overload returns a LocalDate[]. I don't think the one that takes string inputs needs to be special.

Copy link
Member Author

Choose a reason for hiding this comment

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

I assumed that String in implies String out. If someone is working in strings, do you expect that they would not want this result in strings as well?

return businessDates(start, end, true, true);
}

Expand Down Expand Up @@ -1357,14 +1359,16 @@ public LocalDate[] nonBusinessDates(final LocalDate start, final LocalDate end,
* @throws InvalidDateException if the dates are not in the valid range
* @throws DateTimeUtils.DateTimeParseException if the string cannot be parsed
*/
public LocalDate[] nonBusinessDates(final String start, final String end, final boolean startInclusive,
public String[] nonBusinessDates(final String start, final String end, final boolean startInclusive,
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment as the one on businessDates

Copy link
Member Author

Choose a reason for hiding this comment

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

I assumed that String in implies String out. If someone is working in strings, do you expect that they would not want this result in strings as well?

final boolean endInclusive) {
if (start == null || end == null) {
return null;
}

return nonBusinessDates(DateTimeUtils.parseLocalDate(start), DateTimeUtils.parseLocalDate(end), startInclusive,
endInclusive);
final LocalDate[] dates =
nonBusinessDates(DateTimeUtils.parseLocalDate(start), DateTimeUtils.parseLocalDate(end), startInclusive,
endInclusive);
return dates == null ? null : Arrays.stream(dates).map(DateTimeUtils::formatDate).toArray(String[]::new);
}

/**
Expand Down Expand Up @@ -1430,7 +1434,7 @@ public LocalDate[] nonBusinessDates(final LocalDate start, final LocalDate end)
* @throws InvalidDateException if the dates are not in the valid range
* @throws DateTimeUtils.DateTimeParseException if the string cannot be parsed
*/
public LocalDate[] nonBusinessDates(final String start, final String end) {
public String[] nonBusinessDates(final String start, final String end) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment as the one I made on businessDates

Copy link
Member Author

Choose a reason for hiding this comment

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

I assumed that String in implies String out. If someone is working in strings, do you expect that they would not want this result in strings as well?

return nonBusinessDates(start, end, true, true);
}

Expand Down Expand Up @@ -1748,12 +1752,13 @@ public LocalDate plusBusinessDays(final LocalDate date, final int days) {
* @throws InvalidDateException if the date is not in the valid range
* @throws DateTimeUtils.DateTimeParseException if the string cannot be parsed
*/
public LocalDate plusBusinessDays(final String date, final int days) {
public String plusBusinessDays(final String date, final int days) {
if (date == null || days == NULL_INT) {
return null;
}

return plusBusinessDays(DateTimeUtils.parseLocalDate(date), days);
final LocalDate d = plusBusinessDays(DateTimeUtils.parseLocalDate(date), days);
return d == null ? null : d.toString();
}

/**
Expand Down Expand Up @@ -1840,7 +1845,7 @@ public LocalDate minusBusinessDays(final LocalDate date, final int days) {
* @throws InvalidDateException if the date is not in the valid range
* @throws DateTimeUtils.DateTimeParseException if the string cannot be parsed
*/
public LocalDate minusBusinessDays(final String date, final int days) {
public String minusBusinessDays(final String date, final int days) {
if (date == null || days == NULL_INT) {
return null;
}
Expand Down Expand Up @@ -1939,12 +1944,13 @@ public LocalDate plusNonBusinessDays(final LocalDate date, final int days) {
* @throws InvalidDateException if the date is not in the valid range
* @throws DateTimeUtils.DateTimeParseException if the string cannot be parsed
*/
public LocalDate plusNonBusinessDays(final String date, final int days) {
public String plusNonBusinessDays(final String date, final int days) {
if (date == null || days == NULL_INT) {
return null;
}

return this.plusNonBusinessDays(DateTimeUtils.parseLocalDate(date), days);
final LocalDate d = this.plusNonBusinessDays(DateTimeUtils.parseLocalDate(date), days);
return d == null ? null : d.toString();
}

/**
Expand Down Expand Up @@ -2034,7 +2040,7 @@ public LocalDate minusNonBusinessDays(final LocalDate date, final int days) {
* @throws InvalidDateException if the date is not in the valid range
* @throws DateTimeUtils.DateTimeParseException if the string cannot be parsed
*/
public LocalDate minusNonBusinessDays(final String date, final int days) {
public String minusNonBusinessDays(final String date, final int days) {
if (date == null || days == NULL_INT) {
return null;
}
Expand Down
21 changes: 13 additions & 8 deletions engine/time/src/main/java/io/deephaven/time/calendar/Calendar.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
import java.time.*;
import java.time.temporal.ChronoUnit;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.List;

import static io.deephaven.util.QueryConstants.NULL_INT;
Expand Down Expand Up @@ -258,12 +259,13 @@ public LocalDate plusDays(final LocalDate date, final int days) {
* {@link io.deephaven.util.QueryConstants#NULL_INT}.
* @throws DateTimeUtils.DateTimeParseException if the string cannot be parsed
*/
public LocalDate plusDays(final String date, final int days) {
public String plusDays(final String date, final int days) {
if (date == null || days == NULL_INT) {
return null;
}

return plusDays(DateTimeUtils.parseLocalDate(date), days);
final LocalDate d = plusDays(DateTimeUtils.parseLocalDate(date), days);
return d == null ? null : d.toString();
}

/**
Expand Down Expand Up @@ -339,12 +341,13 @@ public LocalDate minusDays(final LocalDate date, final int days) {
* {@link io.deephaven.util.QueryConstants#NULL_INT}.
* @throws DateTimeUtils.DateTimeParseException if the string cannot be parsed
*/
public LocalDate minusDays(final String date, final int days) {
public String minusDays(final String date, final int days) {
if (date == null || days == NULL_INT) {
return null;
}

return minusDays(DateTimeUtils.parseLocalDate(date), days);
final LocalDate d = minusDays(DateTimeUtils.parseLocalDate(date), days);
return d == null ? null : d.toString();
}

/**
Expand Down Expand Up @@ -457,14 +460,16 @@ public LocalDate[] calendarDates(final LocalDate start, final LocalDate end, fin
* @return dates between {@code start} and {@code end}, or {@code null} if any input is {@code null}.
* @throws DateTimeUtils.DateTimeParseException if the string cannot be parsed
*/
public LocalDate[] calendarDates(final String start, final String end, final boolean startInclusive,
public String[] calendarDates(final String start, final String end, final boolean startInclusive,
final boolean endInclusive) {
if (start == null || end == null) {
return null;
}

return calendarDates(DateTimeUtils.parseLocalDate(start), DateTimeUtils.parseLocalDate(end), startInclusive,
endInclusive);
final LocalDate[] dates =
calendarDates(DateTimeUtils.parseLocalDate(start), DateTimeUtils.parseLocalDate(end), startInclusive,
endInclusive);
return dates == null ? null : Arrays.stream(dates).map(DateTimeUtils::formatDate).toArray(String[]::new);
}

/**
Expand Down Expand Up @@ -528,7 +533,7 @@ public LocalDate[] calendarDates(final LocalDate start, final LocalDate end) {
* any input is {@code null}.
* @throws DateTimeUtils.DateTimeParseException if the string cannot be parsed
*/
public LocalDate[] calendarDates(final String start, final String end) {
public String[] calendarDates(final String start, final String end) {
return calendarDates(start, end, true, true);
}

Expand Down
Loading
Loading