adhocore / gronx

Lightweight, fast and dependency-free Cron expression parser (due checker, next/prev due date finder), task runner, job scheduler and/or daemon for Golang (tested on v1.13+) and standalone usage. If you are bold, use it to replace crontab entirely.
https://github.com/adhocore/gronx
MIT License
422 stars 25 forks source link

`IsValid` does not work properly #15

Closed sananguliyev closed 1 year ago

sananguliyev commented 1 year ago

IsValid only checks whether cron expression has 5 or 6 parts. E.g. * * 30 * * * this should be invalid since there is no hour 30 but gronx returns true for it.

adhocore commented 1 year ago

btw * * 30 * * * maybe valid because 30 in 3rd segment is month day 1-31. also IsValid already does check a lot more than number of segments ;) but again that is just wrong example, and this issue will be resolved in #16 then * 30 * * * will no longer be valid

adhocore commented 1 year ago

released https://github.com/adhocore/gronx/releases/tag/v1.2.0

sananguliyev commented 1 year ago

btw * * 30 * * * maybe valid because 30 in 3rd segment is month day 1-31. also IsValid already does check a lot more than number of segments ;) but again that is just wrong example, and this issue will be resolved in #16 then * 30 * * * will no longer be valid

3rd segment is not day since the example has 6 segments and 3rd segment is hours. We need to find the count of segments, and then we can check whether the 3rd segment is day or hour.

adhocore commented 1 year ago

with 6 segments the only change is last segment is year, rest are all the same isn't it?

minute (0-59) | hour (0 - 23) | day of the month (1 - 31) | month (1 - 12) | day of the week (0 - 6) | year

https://res-1.cloudinary.com/hv4xf2jgd/image/upload/q_auto/v1/images/cron_expression_syntax.png

adhocore commented 1 year ago

if that's not the case then i will have to drop support for year altogether from gronx and just have

minute (0-59) | hour (0 - 23) | day of the month (1 - 31) | month (1 - 12) | day of the week (0 - 6)

than have confusions.

sananguliyev commented 1 year ago

AFAIK, when it's 6 segments, the first one is seconds. The year case included only when there are 7 segments.

adhocore commented 1 year ago

hmm can i have some clear refs please? thanks i need to standardize the gronx library whichever is the convention (or just support minimal 5 segments instead of refactoring it heavily)

sananguliyev commented 1 year ago

Covering all cases, at least 6 would be nice. Actually, 6 segments are mandatory. Only year segment is optional. Nowadays most of the people skip seconds part, but in the end most of the systems just prepend 0 in case it's 5 segments.

This is the first result popped up. Probably, all the docs are saying the same thing. https://help.deltek.com/product/CostpointSFT/2.0/GA/cron_expressions.htm#:~:text=A%20cron%20expression%20is%20a,The%20year%20is%20not%20mandatory.

adhocore commented 1 year ago

atm i am testing this feature to support 1st segment as seconds

adhocore commented 1 year ago

can you create a new ticket? this ticket #15 would be used as value range validation eg: 0-59 for minutes, 0-23 for hours etc. and a new would be for Seconds support

sananguliyev commented 1 year ago

@adhocore I have created new issue https://github.com/adhocore/gronx/issues/17

But this needs to be communicated in the documentation clearly since this is breaking change.