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

Implement functions to generate latest and list of times #53

Merged
merged 1 commit into from
Aug 19, 2020

Conversation

owenfarrell
Copy link
Member

This change adds 2 new functions to the TimeLord type:

  1. Latest - generates the time.Time for which no greater value will also satisfy the configuration. This function may return a zero time.Time in certain circumstances, such as:
  • no PreviousTime and not in Start/Stop range
  • PreviousTime is after reference
  1. List - generate all time.Time instances which satisfy the configuration between the PreviousTime and the specified reference (inclusive). This function may return an empty list in certain circumstances, such as:
  • PreviousTime is not rounded to the nearest interval/start-of-range
  • PreviousTime is after reference

This PR is marked as a draft until #52 is merged.

@owenfarrell owenfarrell changed the title Implement functions to generate latest and list of times WIP: Implement functions to generate latest and list of times Jun 29, 2020
@vito vito assigned vito and izabelacg and unassigned vito Jun 30, 2020
@owenfarrell owenfarrell marked this pull request as ready for review July 21, 2020 22:10
@owenfarrell owenfarrell changed the title WIP: Implement functions to generate latest and list of times Implement functions to generate latest and list of times Jul 21, 2020
Copy link
Member

@vito vito left a comment

Choose a reason for hiding this comment

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

Phew! Sorry for the wait on this, lots of life stuff getting in the way lately. Also time related PRs are so tricky to review, haha. (As exemplified by my many notes to self.)

I think this is good to go - the tests look great and make sense to me, and at worst it won't break anything as it's not yet integrated. Merging!

}),
Entry("between the start and stop time but the stop time is before the start time, spanning half a day", testCase{
start: "8:00 PM +0000",
stop: "8:00 AM +0000",
now: "1:00 AM +0000",
result: true,
latest: expectedTime{hour: 20, weekday: time.Saturday},
Copy link
Member

Choose a reason for hiding this comment

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

Note to self: this, along with the other examples, expects Saturday because nowDay defaults to 0, i.e. Sunday, and we're returning 8PM on the previous day, i.e. Saturday.

}),

Entry("between the start and stop time but the compare time is in a different timezone", testCase{
start: "2:00 AM -0600",
stop: "6:00 AM -0600",
now: "1:00 AM -0700",
result: true,
latest: expectedTime{hour: 8},
Copy link
Member

Choose a reason for hiding this comment

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

Note to self: 8 because it's normalized to UTC, and 2AM + negating the -6 hour offset = 8.

@@ -294,6 +349,11 @@ var _ = DescribeTable("A range with a location and a previous time", (testCase).
nowDay: time.Thursday,

result: true,
latest: expectedTime{hour: 13, weekday: time.Thursday},
list: []expectedTime{
{hour: 13, weekday: time.Wednesday},
Copy link
Member

Choose a reason for hiding this comment

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

Note to self: 13 = 1PM in America/Indiana/Indianapolis = 6PM +0000.

@@ -342,6 +410,11 @@ var _ = DescribeTable("A range with an interval and a previous time", (testCase)
nowDay: time.Thursday,

result: true,
latest: expectedTime{hour: 13, weekday: time.Thursday},
list: []expectedTime{
{hour: 14, minute: 58, weekday: time.Wednesday},
Copy link
Member

Choose a reason for hiding this comment

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

Note to self: stop is exclusive, so it is correct for this to be the last version.

@vito vito merged commit 23cbebb into concourse:master Aug 19, 2020
@owenfarrell owenfarrell deleted the lord_calculator branch August 20, 2020 14:07
@owenfarrell
Copy link
Member Author

@vito - Thanks for the follow up. And don't sweat the turnaround time, I know there's been lots of activity going on with core capabilities and recognize this isn't as urgent as that work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants