Skip a job #320
Skip a job #320
Conversation
There was a problem hiding this comment.
Thanks for submitting this pr!
Just a few small comments left before we can merge 🚀.
Would you also be able to add an example to the docs?
| @@ -73,6 +73,13 @@ class CancelJob(object): | |||
| pass | |||
|
|
|||
|
|
|||
| class SkipJob(object): | |||
| """ | |||
| Can be returned from a job to skip a run. | |||
There was a problem hiding this comment.
Let's elaborate a bit more what it means to skip a job. We could mention that the job scheduling is not changed and that the job will run the next time run_pending is called (unless the job's deadline set by until is reached).
| @@ -483,7 +490,8 @@ def run(self): | |||
| """ | |||
| logger.info('Running job %s', self) | |||
| ret = self.job_func() | |||
| self.last_run = datetime.datetime.now() | |||
| if not isinstance(ret, SkipJob) and ret is not SkipJob: | |||
| self.last_run = datetime.datetime.now() | |||
| self._schedule_next_run() | |||
There was a problem hiding this comment.
Should self._schedule_next_run() also be included in the if statements body?
| if not isinstance(ret, SkipJob) and ret is not SkipJob: | ||
| self.last_run = datetime.datetime.now() |
There was a problem hiding this comment.
Personally I would invert the if statement like so:
| if not isinstance(ret, SkipJob) and ret is not SkipJob: | |
| self.last_run = datetime.datetime.now() | |
| if isinstance(ret, SkipJob) or ret is SkipJob: | |
| return ret | |
| self.last_run = datetime.datetime.now() |
(untested suggestion)
That would make it more clear that returning SkipJob is a special flow. And the normal flow would continue on the same level.
| if skip: | ||
| return schedule.SkipJob | ||
| else: | ||
| return None |
There was a problem hiding this comment.
Nice tests! I like them 👍
As mentioned in #319, add the ability to skip a job without cancel it.