Conversation
SijmenHuizenga
left a comment
SijmenHuizenga
left a comment
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?
|
|
||
| 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).
| 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 👍
3 participants
As mentioned in #319, add the ability to skip a job without cancel it.