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 7 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
40 changes: 26 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 @@ -165,6 +165,7 @@ private YearData getYearData(final int year) {

// region Getters

// TODO: should there be a string equivalent?
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think there needs to be one

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree; people can convert these to strings on their own, or stick with java.time types (as we'd generally recommend).

I don't strongly oppose a firstValidDateStr(); I just wouldn't add it proactively. We can always add it later if people ask for it.

/**
* Returns the first valid date for the business calendar.
*
Expand All @@ -174,6 +175,7 @@ public LocalDate firstValidDate() {
return firstValidDate;
}

// TODO: should there be a string equivalent?
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think so.

/**
* Returns the last valid date for the business calendar.
*
Expand Down Expand Up @@ -1214,14 +1216,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 +1291,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 +1361,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 +1436,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 +1754,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 +1847,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 +1946,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 +2042,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 Expand Up @@ -2091,6 +2099,7 @@ public ZonedDateTime minusNonBusinessDays(final ZonedDateTime time, final int da
return plusNonBusinessDays(time, -days);
}

// TODO: should there be a string equivalent?
Copy link
Contributor

@jjbrosnan jjbrosnan Jan 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think so.

/**
* Adds a specified number of business days to the current date. Adding negative days is equivalent to subtracting
* days.
Expand All @@ -2108,6 +2117,7 @@ public LocalDate futureBusinessDate(final int days) {
return plusBusinessDays(calendarDate(), days);
}

// TODO: should there be a string equivalent?
Copy link
Contributor

@jjbrosnan jjbrosnan Jan 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think so.

/**
* Subtracts a specified number of business days from the current date. Subtracting negative days is equivalent to
* adding days.
Expand All @@ -2125,6 +2135,7 @@ public LocalDate pastBusinessDate(final int days) {
return minusBusinessDays(calendarDate(), days);
}

// TODO: should there be a string equivalent?
Copy link
Contributor

@jjbrosnan jjbrosnan Jan 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think so.

/**
* Adds a specified number of non-business days to the current date. Adding negative days is equivalent to
* subtracting days.
Expand All @@ -2142,6 +2153,7 @@ public LocalDate futureNonBusinessDate(final int days) {
return this.plusNonBusinessDays(calendarDate(), days);
}

// TODO: should there be a string equivalent?
/**
* Subtracts a specified number of non-business days to the current date. Subtracting negative days is equivalent to
* adding days.
Expand Down
25 changes: 17 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 @@ -89,6 +90,7 @@ public String toString() {

// region Now

// TODO: should there be a string equivalent?
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think so

/**
* The current date for the calendar.
*
Expand Down Expand Up @@ -117,6 +119,7 @@ public DayOfWeek dayOfWeek(final LocalDate date) {
return DateTimeUtils.dayOfWeek(date);
}

// TODO: should this return a String?
Copy link
Contributor

Choose a reason for hiding this comment

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

All of the overloads for this method return a DayOfWeek. The one that takes string as input doesn't have to be special. It should conform to the behavior the overloads have.

/**
* The current day of the week for the calendar.
*
Expand Down Expand Up @@ -258,12 +261,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 +343,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 @@ -392,6 +397,7 @@ public ZonedDateTime minusDays(final ZonedDateTime time, final int days) {
return plusDays(time, -days);
}

// TODO: should there be a string equivalent?
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think so.

/**
* Adds a specified number of days to the current date. Adding negative days is equivalent to subtracting days.
*
Expand All @@ -403,6 +409,7 @@ public LocalDate futureDate(final int days) {
return plusDays(calendarDate(), days);
}

// TODO: should there be a string equivalent?
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think so.

/**
* Subtracts a specified number of days from the current date. Subtracting negative days is equivalent to adding
* days.
Expand Down Expand Up @@ -457,14 +464,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 +537,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