-
Notifications
You must be signed in to change notification settings - Fork 79
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
Calendar API cleanup #4201
Calendar API cleanup #4201
Conversation
* Exceptions
* Business Schedule
* Business Day
* Business Time
* Arithmetic
* BusinessCalendarParser
* BusinessCalendarParser
* DefaultBusinessCalendar
* Calendars
* DateStringUtils
* DefaultNoHolidayBusinessCalendar
* Differences
* Plus/Minus
* Calendar
* Calendar * BusinessCalendar
* Calendar * BusinessCalendar
* Calendar * BusinessCalendar
* @return current date | ||
*/ | ||
public LocalDate calendarDate() { | ||
return DateTimeUtils.todayLocalDate(timeZone()); |
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.
It's important context, but we are depending on the clock
state in DateTimeUtils
. I'm not the biggest fan of the static state in DateTimeUtils
in-and-of itself, but I wonder if we should be persisting that stateful dependency here? Should clock
be a member of Calendar
?
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 fully following this set of comments.
DateTimeUtils
is the foundational layer. Calendar
is built on top of DateTimeUtils
. BusinessCalendar
is built on top of Calendar
. If Calendar
has its own clock, separate from DateTimeUtils
, it is just going to lead to user nightmare scenarios since we can't tell the user to just change the clock in a single place. Instead, they will end up with an inconsistent view of the world where they need to know multiple places they need to update to achieve consistency.
engine/time/src/main/java/io/deephaven/time/calendar/Calendar.java
Outdated
Show resolved
Hide resolved
engine/time/src/main/java/io/deephaven/time/calendar/BusinessCalendar.java
Outdated
Show resolved
Hide resolved
engine/time/src/main/java/io/deephaven/time/calendar/BusinessCalendarXMLParser.java
Outdated
Show resolved
Hide resolved
…minusDays for long args.
…ors package so that they can be converted to interfaces in the future.
engine/time/src/main/java/io/deephaven/time/calendar/Calendar.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.
The Python changes LGTM.
Labels indicate documentation is required. Issues for documentation have been opened: Community: https://github.com/deephaven/deephaven.io/issues/3610 |
The calendar API is a key part of the time library that was never modernized. This PR cleans up the user-facing calendar libraries:
Many of the above changes are breaking. The prior library in DHC was not popularized because this API rework had not been completed. Hopefully, this means the number of impacted users is small.
The underlying library has >90% test coverage.
The Calendar XML configuration format has been changed to: