issue-740: expand oneTimeJob to support multiple times

[rbroggi]

What does this do?

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

Resolves #740

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

[rbroggi]

While implementing this functionality I found unexpected that the NextRun will not return time.Time{} upon exhausted runs.

I debugged the implementation and saw that the next method implemented by the new job works as expected - returning time.Time{} upon input lastRun that are after the sortedTimes.

This could be a potential bug also for the cronJob schedule - I wonder what it returns upon schedule exhaustion.

I have issued a PR to demonstrate this concern for the OneTimeJob: #742

[JohnRoesler]

hey @rbroggi thanks for all your contributions lately!

For the implementation on this one, I think it would be nice and possible to utilize the existing OneTimeJob but expand it to allow multiple runs.

Something like (incomplete code...)

var _ JobDefinition = (*oneTimeJobDefinition)(nil)

type oneTimeJobDefinition struct {
	startAt OneTimeJobStartAtOption
}

func (o oneTimeJobDefinition) setup(j *internalJob, _ *time.Location, now time.Time) error {
	var oneTimeJobSchedule oneTimeJob
	atTimes, err := o.startAt(j)
	if err != nil {
		return err
	}
	parsedAtTimes := make([]time.Time, 0)
	for _, at := range atTimes {
		if at.After(now) {
			parsedAtTimes = append(parsedAtTimes, at)
		}
	}
	slices.SortStableFunc(parsedAtTimes, func(a, b time.Time) int {
		return a.Compare(b)
	})
	if len(parsedAtTimes) != 0 {
		j.startTime = parsedAtTimes[0]
		oneTimeJobSchedule.atTimes = parsedAtTimes
	}
	j.jobSchedule = oneTimeJobSchedule
	
	// in case we are not in the `startImmediately` case, our start-date must be in
	// the future according to the scheduler clock
	if !j.startImmediately && (j.startTime.IsZero() || j.startTime.Before(now)) {
		return ErrOneTimeJobStartDateTimePast
	}
	return nil
}

// OneTimeJobStartAtOption defines when the one time job is run
type OneTimeJobStartAtOption func(*internalJob) ([]time.Time, error)

// OneTimeJobStartImmediately tells the scheduler to run the one time job immediately.
func OneTimeJobStartImmediately() OneTimeJobStartAtOption {
	return func(j *internalJob) ([]time.Time, error) {
		j.startImmediately = true
		return nil, nil
	}
}

// OneTimeJobStartDateTime sets the date & time at which the job should run.
// This datetime must be in the future (according to the scheduler clock).
func OneTimeJobStartDateTime(start time.Time) OneTimeJobStartAtOption {
	return func(j *internalJob) ([]time.Time, error) {
		j.startTime = start
		return nil, nil
	}
}

// OneTimeJobStartDateTimes sets the date & times at which the job should run.
// At least one of the date/times must be in the future (according to the scheduler clock).
func OneTimeJobStartDateTimes(times []time.Time) OneTimeJobStartAtOption {
	return func(j *internalJob) ([]time.Time, error) {
		if len(times) == 0 {
			return nil, fmt.Errorf("TODO")
		}
		return times, nil
	}
}

// OneTimeJob is to run a job once at a specified time and not on
// any regular schedule.
func OneTimeJob(startAt OneTimeJobStartAtOption) JobDefinition {
	return oneTimeJobDefinition{
		startAt: startAt,
	}
}





var _ jobSchedule = (*oneTimeJob)(nil)

type oneTimeJob struct {
	atTimes []time.Time
}

func (o oneTimeJob) next(lastRun time.Time) time.Time {
	if len(o.atTimes) != 0 {
		next := o.atTimes[0]
		o.atTimes = o.atTimes[1:]
		if !next.After(lastRun) {
			return time.Time{}
		}
		return next
	}
	return time.Time{}
}

What do you think about that? I haven't tested or anything, but I think it should work.

[rbroggi]

I think it should work as well, it sounds a bit misleading to me to have a OneTimeJob actually implementing a ManyTimesJob
but that's only my opinion, happy to accommodate your suggestion if you think it aligns better with the lib design.

[JohnRoesler]

@rbroggi I do agree, the name is a little misleading, but I think updating the description of the OneTimeJob will suffice. It's really more like "limited set of job(s)". I like how it fits vs. adding an entirely new job type. 🤝

[rbroggi]

@rbroggi I do agree, the name is a little misleading, but I think updating the description of the OneTimeJob will suffice. It's really more like "limited set of job(s)". I like how it fits vs. adding an entirely new job type. 🤝

Awesome, will followup! thank you for the fast feedback ❤️

[rbroggi]

I had to change the signature of this exported type - I tried to think about situations where this would not be considered a backwards-compatible change but I could not think of one.

[JohnRoesler]

I've used this in other places, Before vs. Compare. SortStable, which isn't super important here, just keeps the order if items are duplicates

slices.SortStableFunc(j.nextScheduled, func(a, b time.Time) int {
			return a.Compare(b)
		})

[JohnRoesler]

This looks good 👍 I tried to think of any edge cases with input, but you handle if they pass an empty slice, or times in the past