mvniekerk / tokio-cron-scheduler

Schedule tasks on Tokio using cron-like annotation
Apache License 2.0
529 stars 59 forks source link

Allow cron::Schedules as function arguments #11

Closed hgzimmerman closed 2 years ago

hgzimmerman commented 2 years ago

This PR depends on this change in the cron crate making its way to crates.io and should not be merged before then. The minimum version is anticipated to be either 0.9.1 or 0.10.0.

This PR should allow taking schedule arguments as either &str or Schedule types. This is preferable over taking plain &strs because it allows testing that the cron strings are valid before passing them to your library, lowering the chance of a programmer error causing a scheduled task to not run when ignoring errors returned from constructing Jobs

mvniekerk commented 2 years ago

Hi @hgzimmerman Thank you for the PR. I take it that the it is marked WIP up until cron has been updated. Looking forward 👍🏻

hgzimmerman commented 2 years ago

Yup, I just renamed the PR to indicate it is a WIP. It may take a bit for the change in cron to be released, given the last release was a year ago, so this may just sit here for a while.

mvniekerk commented 2 years ago

Hey @hgzimmerman Kindly check - I've merged a huge code change into main for 0.6-beta.1. There's an error enum JobSchedulerError that also may be of interest for you.

hgzimmerman commented 2 years ago

I'm finding the places where support for taking a TryInto<Schedule, E> would be added return an error that can only come from failing to parse the &str to a Schedule. Instead, if duplicate functions that only took Schedule were added, they wouldn't need to return a Result. So you would have something like:

    pub fn new_infallible<T>(schedule: Schedule, run: T) -> Self
    where
        T: 'static,
        T: FnMut(Uuid, JobsSchedulerLocked) + Send + Sync,
    {
        let schedule: Schedule = schedule.try_into()?;
        let job_id = Uuid::new_v4();
        Self(Arc::new(RwLock::new(Box::new(CronJob {
            data: JobStoredData {
               // ...
            },
            run: Box::new(run),
            run_async: Box::new(nop_async),
            async_job: false,
        }))))
    }
    pub fn new<T>(schedule: &str, run: T) -> Result<Self, Box<dyn std::error::Error>>
    where
        T: 'static,
        T: FnMut(Uuid, JobsSchedulerLocked) + Send + Sync,
    {
        let schedule = Schedule::from_str(schedule)?
        Ok(Self::new_infallible(schedule, run))
    }

This could be done without waiting for cron to release to crates.io, but would require adding functions with somewhat odd naming schemes that do the same thing as the current ones, but return Self instead of Result<Self, Box<dyn Error>>.

Do you have any thoughts regarding this versus what is currently in this PR?

BlackDex commented 2 years ago

@hgzimmerman looks like cron is updated :tada: https://github.com/zslayton/cron/pull/99

https://github.com/zslayton/cron/releases/tag/v0.10.0

mvniekerk commented 2 years ago

Hey @hgzimmerman & @BlackDex Looking forward getting this patch in. We'll chat after 0.6 beta 2 is released? I'm writing example code for the Nats integration then I'll be releasing it.

mvniekerk commented 2 years ago

Okay so 0.6 beta 2 is out. You'll see that job.rs moved to job/mod.rs If you can kindly update your PR?

mvniekerk commented 2 years ago

Thank you @hgzimmerman for your contribution. I'll merge the PR and have it part of the 0.6 release 🙌🏻