agronholm / apscheduler

Task scheduling library for Python
MIT License
6.22k stars 705 forks source link

Weekday numbers in crontab expressions correspond to wrong weekdays #286

Closed qli-aa closed 4 years ago

qli-aa commented 6 years ago

apscheduler/apscheduler/triggers/cron/expressions.py WEEKDAYS = ['mon', 'tue', 'wed', 'thu', 'fri', 'sat', 'sun'] will map mon ==> 0, sun ==> 6

actually should map mon ==>1 , sun ==> 0 or 7 WEEKDAYS = ['sun', 'mon', 'tue', 'wed', 'thu', 'fri', 'sat', 'sun']

from cron wiki: https://en.wikipedia.org/wiki/Cron

agronholm commented 6 years ago

How is this affecting you? I would also like to point out the notice here saying that Monday is always the first weekday (as per the ISO standard). The only place where this might have an effect is the newly added crontab expression support. If so then I will fix it there locally.

qli-aa commented 6 years ago

I migrated old cron task with 30 7 4 cron expr from linux crontab to apscheduler, I found it was scheduled at Fri, it was scheduled at Thur before.

agronholm commented 6 years ago

I see, that explains it. As a workaround, you can replace the weekday with the textual expression (thu). This is also supported by actual cron daemons.

agronholm commented 6 years ago

This is a bit more problematic than I thought at first. I cannot simply change the way the weekdays are indexed because it would break things for existing users of CronTrigger. On the other hand, APScheduler's weekday numbers do not correspond to cron's which causes confusion to people used to crontabs.

chomey commented 6 years ago

Yeap, we just ran into this as well :)

qli-aa commented 6 years ago

So it will be changed to crontab style ? or not ?

agronholm commented 6 years ago

CronTrigger.from_crontab() will be changed to respect crontab style weekday numbering. I will consider changing CronTrigger itself to use the same numbering in APScheduler 4.0, but for now I will keep backwards compatibility there.

myzhan commented 5 years ago

I just ran into the same issue, add this to work around, support limited expressions.

def crontab_compatible_weekday(expr):
    if expr == "*":
        return expr

    mapping = {
        "0": "sun",
        "1": "mon",
        "2": "tue",
        "3": "wed",
        "4": "thu",
        "5": "fri",
        "6": "sat",
        "7": "sun"
    }

    return "".join(map(lambda x: mapping.get(x, x), expr))
andrewchen5678 commented 3 years ago

when will 4.0 be released with this fix?

agronholm commented 3 years ago

Not very soon. In the meantime, I suggest you use weekday names instead and subscribe to updates on #465.