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

add WaitForSchedule(s) funcs #162

Merged
merged 4 commits into from
Apr 16, 2021

Conversation

JohnRoesler
Copy link
Contributor

What does this do?

  • adds WaitForSchedule and WaitForSchedules to allow setting per job or for all jobs to wait for the first scheduled run rather than running immediately
  • un-exports the job's NewJob method. There was no use for it to be exported as we don't accept a newly created job struct in any scheduler methods. alters the signature to handle the wait for interval logic

Which issue(s) does this PR fix/relate to?

#152

List any changes that modify/break current functionality

Have you included tests for your changes?

Did you document any new/modified functionality?

  • Updated example_test.go
  • Updated README.md

Notes

scheduler.go Outdated Show resolved Hide resolved
scheduler.go Outdated
// new jobs with the WaitForSchedule option as true.
// The jobs will not start immediately but rather will
// wait until their first scheduled interval.
func (s *Scheduler) WaitForSchedules() {
Copy link
Member

Choose a reason for hiding this comment

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

I think the users might think WaitForSchedules() and WaitForSchedule() have the same behavior. Because we have other methods like Second()-Seconds(), Minute()-Minutes() etc which in spite of being singular and plural have the same behavior.

The comments are clear enough, but maybe a different name for WaitForSchedules might be nice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure. Thoughts on the name?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

WaitForSchedulesAllJobs()

Copy link
Member

@arjunmahishi arjunmahishi Apr 15, 2021

Choose a reason for hiding this comment

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

Sure. Or if the word "Hold" makes sense, they can both be changed to HoldJobTillFirstSchedule and HoldAllJobsTillFirstSchedule

Copy link
Member

Choose a reason for hiding this comment

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

We could also skip the word "First" to make it shorter :P

Copy link
Contributor Author

Choose a reason for hiding this comment

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

how about WaitForScheduleAll i pushed that, let me know

Copy link
Member

Choose a reason for hiding this comment

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

Sure 👍🏽

JohnRoesler and others added 3 commits April 15, 2021 15:12
Co-authored-by: Arjun Mahishi <arjun.mahishi@gmail.com>
@JohnRoesler JohnRoesler merged commit c7671c0 into go-co-op:master Apr 16, 2021
@JohnRoesler JohnRoesler deleted the start-first-interal branch April 16, 2021 03:27
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.

None yet

2 participants