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

fix: offset logic calculated for east_opt not for west_opt #64

Merged
merged 1 commit into from
Apr 26, 2024

Conversation

rlienlaf
Copy link
Contributor

While testing Job::new_async_tz on different timezones with specific set hours next_tick_for_job() was giving wrong results. On further inspection, the next_tick for every job with given timezones was being calculated wrong because the offset logic wasn't working properly while using west_opt() when calculating the fixed offset, mainly because of how signs are treated on FixedOffset.
Changing west_opt() for east_opt() should suffice as a quick fix. Also, as an alternative solution, when initially calculating time_offset_seconds in JobLocked::new_async_tz, instead of local_minus_utc(), utc_minus_local() can be used in conjunction with the original west_opt().

As an example to replicate this problem, following the README, a job can be created like this

    let mut job = Job::new_async_tz("1/7 * 17 * * *", Santiago, |uuid, mut l| {
        Box::pin(async move {
            println!("I run async every 7 seconds");

            // Query the next execution time for this job
            let next_tick = l.next_tick_for_job(uuid).await;
            match next_tick {
                Ok(Some(ts)) => println!("Next time for 7s job is {:?}", ts),
                _ => println!("Could not get next tick for 7s job"),
            }
        })
    })?;

Pay attention to the value returned by next_tick_for_job().

@dgonzf1
Copy link

dgonzf1 commented Apr 22, 2024

Gran trabajo maquina

Copy link

@WeiNyn WeiNyn left a comment

Choose a reason for hiding this comment

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

Face the same problem, which cause my schedule wrong after first attempt. I believe this commit can fix problem

@mvniekerk mvniekerk merged commit 849422c into mvniekerk:main Apr 26, 2024
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

4 participants