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.

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

Make time of day methods handle daylight savings time (DST) better #5245

Merged
merged 9 commits into from
Mar 19, 2024

Conversation

chipkent
Copy link
Member

@chipkent chipkent commented Mar 12, 2024

Change time of day methods to handle daylight savings time better.

The old method signatures are deprecated and need to be removed in a version or two.

Resolves #5244

@chipkent chipkent added this to the 1. March 2024 milestone Mar 12, 2024
@chipkent chipkent requested a review from kosak March 12, 2024 18:21
@chipkent chipkent self-assigned this Mar 12, 2024
@chipkent chipkent added the devrel-watch DevRel team is watching label Mar 12, 2024
Copy link
Contributor

@kosak kosak left a comment

Choose a reason for hiding this comment

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

just some small comments

@ScriptApi
@Deprecated
public static int millisOfDay(@Nullable final Instant instant, @Nullable final ZoneId timeZone) {
return millisOfDay(instant, timeZone, false);
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 know what the culture is here, but I'm feeling a little sad that we have to provide all these ${UNIT}OfDay methods. Would it be ok just to provide nanosOfDay and let our user call nanosTo${UNIT} e.g. nanosToSeconds or whatever?

Just an observation, I'm not deeply opinionated on this.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is part of an intentional decision made long ago to target concise syntax for command line users. The proposed change results in more verbose query strings.

@chipkent chipkent merged commit fa44ebd into deephaven:main Mar 19, 2024
13 of 14 checks passed
@chipkent chipkent deleted the dst_fix branch March 19, 2024 15:37
@github-actions github-actions bot locked and limited conversation to collaborators Mar 19, 2024
@deephaven-internal
Copy link
Contributor

Labels indicate documentation is required. Issues for documentation have been opened:

Community: deephaven/deephaven-docs-community#172

@chipkent chipkent removed the devrel-watch DevRel team is watching label May 6, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

minuteOfDay returns surprising results on DST days
3 participants