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

Make user can control when to stop a job. #15

Merged
merged 9 commits into from
Sep 2, 2013
Merged

Conversation

cfrco
Copy link
Contributor

@cfrco cfrco commented Aug 12, 2013

I think it's good for user can stop the job (just return False in job_func).

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling fd6c41d on cfrco:master into 05f0a07 on dbader:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 79f9a61 on cfrco:master into 05f0a07 on dbader:master.

@dbader
Copy link
Owner

dbader commented Aug 25, 2013

Thanks @cfrco for your contribution! I agree that it is a good idea to have a way to cancel recurring jobs in schedule. delete() looks good but for naming this I would prefer cancel_job(). We also need to provide this functionality at the module level, i.e. there should also be a schedule.cancel_job().

I'm not sure if a job function's return value should determine whether the job gets cancelled or not. I think this may be confusing because False is basically a magic constant in this case. Still, there is some value in the functionality because it gives a job a simple way to cancel itself. For clarity it may be better to designate cancel_job() as the only way to unschedule a job. What are your thoughts on this?

@cfrco
Copy link
Contributor Author

cfrco commented Aug 26, 2013

Hi,
I think cancel_job() is a good idea.

There is a problem if we only provide cancel_job to unschedule a job. That is we can't cancel a job in it's job_func. Because we can't get Job in job_func.

I have two ideas.
(1) Add Job.scheduler (link to Scheduler) . And we can add a method to Job ( may call Job.cancel() ).
Pass Job to job_func as the first parameter.
(but old version job_func can't work with this idea, it should be rewrite.)

(2) Use a new empty-class (may call CancelJob) instead of False.

@coveralls
Copy link

Coverage Status

Coverage decreased (-2.86%) when pulling 2c646ff on cfrco:master into 05f0a07 on dbader:master.

@dbader
Copy link
Owner

dbader commented Aug 30, 2013

Thanks @cfrco, it looks good now! Once you add a test case for removing and invalid job to bring the coverage back up it's good to merge :-)

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 185d462 on cfrco:master into 05f0a07 on dbader:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling bfe8813 on cfrco:master into 05f0a07 on dbader:master.

@dbader
Copy link
Owner

dbader commented Sep 2, 2013

Looks good now, thanks for your contribution @cfrco!

dbader added a commit that referenced this pull request Sep 2, 2013
Allow jobs to cancel repeated execution.
@dbader dbader merged commit 08acba9 into dbader:master Sep 2, 2013
@dbader
Copy link
Owner

dbader commented Nov 9, 2013

@cfrco 0.2.0 is out with your changes -- https://pypi.python.org/pypi/schedule Thanks again! :-)

@cfrco
Copy link
Contributor Author

cfrco commented Nov 10, 2013

: ) Thank you, too.

@PROJETFEG
Copy link

Hi

How using these function with parallel thread you are showing here:

https://github.com/dbader/schedule/blob/master/FAQ.rst

Thank you

@cfrco cfrco mentioned this pull request Jun 7, 2015
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.

4 participants