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

cron job trigger on timezone based, daylightsaving taken care #40

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

laxmanvallandas
Copy link

No description provided.

gocron.go Outdated
@@ -128,27 +132,28 @@ func (j *Job) Do(jobFun interface{}, params ...interface{}) {
func (j *Job) At(t string) *Job {
hour := int((t[0]-'0')*10 + (t[1] - '0'))
min := int((t[3]-'0')*10 + (t[4] - '0'))
if hour < 0 || hour > 23 || min < 0 || min > 59 {
sec := int((t[6]-'0')*10 + (t[7] - '0'))
if hour < 0 || hour > 23 || min < 0 || min > 59 || sec < 0 || sec > 59 {

Choose a reason for hiding this comment

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

This does not handle leap seconds.

Copy link
Author

Choose a reason for hiding this comment

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

Can you elaborate a bit? I suppose leap second is beyond scope of gocron lib as the job gets triggered when at specified time in seconds. After the leap second, since time resets back and job is already triggered, so no issues.
Apologies for responding after years. :)

Copy link

Choose a reason for hiding this comment

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

The library doesn't support winter/summer time changes that happen twice per year causing trigger to be missed completely once per year or triggered at wrong hour, leap seconds is surely something much less important than that.

README.md Outdated
@@ -46,9 +50,9 @@ func main() {
gocron.Every(1).Monday().Do(task)
gocron.Every(1).Thursday().Do(task)

// function At() take a string like 'hour:min'
gocron.Every(1).Day().At("10:30").Do(task)

Choose a reason for hiding this comment

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

I think it is still valuable to have this line here as an example of what you can use.
It isn't always clear what you do need and don't need when you're learning a new language.

Copy link
Author

Choose a reason for hiding this comment

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

I agree. It can be retained.

@sergiught
Copy link

Hello @laxmanvallandas :) this is a feature I find myself in need of with this package. Do you think you can rebase and clean this up a bit so it can be considered to be merged into master?

@laxmanvallandas
Copy link
Author

laxmanvallandas commented Aug 2, 2019 via email

@sergiught
Copy link

Thank you @laxmanvallandas ! If you need some help please let me know :)

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.

5 participants