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

DST Fixes #4

Merged
merged 2 commits into from
Feb 14, 2023
Merged

DST Fixes #4

merged 2 commits into from
Feb 14, 2023

Conversation

naimulhaider
Copy link

@naimulhaider naimulhaider commented Feb 14, 2023

@MikeZLin
Copy link

Since we are cherry-picking fix strategies, let's make a comment/ description on the PR on where this came from.

cronexpr.go Outdated
i := sort.SearchInts(expr.yearList, v)
if i == len(expr.yearList) {
loc := fromTime.Location()
t := fromTime.Add(time.Second - time.Duration(fromTime.Nanosecond())*time.Nanosecond)

Choose a reason for hiding this comment

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

could truncate to time.Nanosecond here i guess

Copy link
Author

Choose a reason for hiding this comment

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

Well they are rounding up - so I've moved the logic to a method explaining it.
All hell breaks loose if I truncate to nanosec.

cronexpr.go Outdated

WRAP:

// let's find the next date that satisfies condition

Choose a reason for hiding this comment

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

probs worth keeping the comment string

Copy link
Author

Choose a reason for hiding this comment

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

cronexpr_test.go Outdated
require.Equal(t, expectedTm, resultTm, test)

//if expr.Next(from).UTC() != to.UTC() {
// t.Errorf("Expected Next time to be %s from %s in %s, when actually %s; UTC FROM %s, UTC TO:%s ", to, from, test.where, expr.Next(from), from.UTC(), expr.Next(from).UTC())

Choose a reason for hiding this comment

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

do we need this commented code?

Copy link
Author

Choose a reason for hiding this comment

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

Nope

{"2019-11-03 02:30:00", "2019-11-04T02:30:00-08:00", "America/Los_Angeles"},
{"2019-03-09 02:30:00", "2019-03-10T03:30:00-07:00", "US/Pacific"},
{"2019-03-09 02:30:00", "2019-03-11T03:30:00-06:00", "US/Pacific"}, // Note that 02:30am on the 10th doesn't exist due to DST

Choose a reason for hiding this comment

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

since san paulo was referenced in the code, lets make a test case for this

Copy link
Author

Choose a reason for hiding this comment

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

@MikeZLin MikeZLin merged commit 24cff38 into master Feb 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants