Skip to content

Conversation

@ljanyst
Copy link

@ljanyst ljanyst commented Nov 23, 2017

As a user, I want to be able to specify an interval definition as a string. This pull request implements this functionality. It parses the input string and calls the appropriate properties and functions. It slightly abuses asserts but you did not define any custom exceptions and I wanted to follow your style.

This is how it works:

schedule.when('every wednesday at 13:15').do(job)
schedule.when('every 15 seconds').do(job)

@coveralls
Copy link

coveralls commented Nov 23, 2017

Coverage Status

Coverage decreased (-1.09%) to 98.905% when pulling ac33fd2 on ljanyst:interval-definitions into b4ac8ca on dbader:master.

@ljanyst ljanyst force-pushed the interval-definitions branch from ac33fd2 to 4aa3b77 Compare November 23, 2017 15:26
@coveralls
Copy link

coveralls commented Nov 23, 2017

Coverage Status

Coverage decreased (-0.4%) to 99.635% when pulling 4aa3b77 on ljanyst:interval-definitions into b4ac8ca on dbader:master.

@ljanyst ljanyst force-pushed the interval-definitions branch from 4aa3b77 to 10a1edb Compare November 23, 2017 15:32
@coveralls
Copy link

coveralls commented Nov 23, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling 10a1edb on ljanyst:interval-definitions into b4ac8ca on dbader:master.

@ljanyst
Copy link
Author

ljanyst commented Nov 23, 2017

Perhaps it would be better to use the Scheduler.__call__ method instead of when and a ValueError exceptions instead of these assertions. Let me know what you think and I will update the code.

@ljanyst ljanyst force-pushed the interval-definitions branch from 10a1edb to ac6db85 Compare November 23, 2017 19:16
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.4%) to 99.638% when pulling ac6db85 on ljanyst:interval-definitions into b4ac8ca on dbader:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.4%) to 99.638% when pulling ac6db85 on ljanyst:interval-definitions into b4ac8ca on dbader:master.

@ljanyst ljanyst force-pushed the interval-definitions branch from ac6db85 to 0d08f59 Compare November 23, 2017 19:26
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.7%) to 99.275% when pulling 0d08f59 on ljanyst:interval-definitions into b4ac8ca on dbader:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 0d08f59 on ljanyst:interval-definitions into b4ac8ca on dbader:master.

@ljanyst
Copy link
Author

ljanyst commented Dec 5, 2017

Are there any chances of getting this merged? I would very much appreciate your feedback. Thanks!

@jcnaud
Copy link

jcnaud commented Mar 2, 2018

Great feature. I actually use it but i also wait this merge.

@ljanyst
Copy link
Author

ljanyst commented Mar 2, 2018

Yeah, I know. ;) I needed it for this: https://github.com/ljanyst/scrapy-do but pretty useful in general.

antwal added a commit to antwal/schedule that referenced this pull request Nov 25, 2018
@SijmenHuizenga
Copy link
Collaborator

I'm very hesitant to go forward with this.

You are proposing to add a new syntax to define a job interval as a string. This already exists: cron. If you want to encode a scheduling interval as a string (could be in a file, via an api or somewhere else) I would recommend to use the cron syntax. It is widely supported and interpretes exist in many languages (like python).

Schedule is a simple Python job scheduler. It does not support advanced features like job persistence or string parsing because other tools already do this a lot better. This library is for projects that do not need all the advanced features that APSchedule offers. The feature you are proposing is one of those advanced features that APSchedule supports, and this library won't. See CronTrigger.from_crontab().

There are for sure projects out there where encoding a job definition as a more human readable string is required, and the code you wrote seems like a good implementation. I encourage you to publish it in your own repository, add an readme with proper documentation. You could do this as a fork of this project, or even better, create a standalone library that parses strings into schedule Job instances that can be used with this scheduler. If you go this route we can definitely add a FAQ item to this library's docs pointing to your library.

@ljanyst
Copy link
Author

ljanyst commented Feb 1, 2021

That's certainly your choice. If I wanted cron I would have used cron.

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