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

fix: Don't accept invalid step for Period #122

Merged
merged 2 commits into from
Oct 29, 2024

Conversation

bombsimon
Copy link
Contributor

If the specifier is a period specifier, ensure that the interval is larger than 0 (0 would be every 0th) and less than the max value for the unit.

Resolves #59

@bombsimon bombsimon changed the title fix: Don't accept invalid intervals for periods fix: Don't accept invalid step for Period Jun 2, 2024
@bombsimon
Copy link
Contributor Author

There's no source cited but saw this on Wikipedia

Note that frequencies in general cannot be expressed; only step values which evenly divide their range express accurate frequencies (for minutes and seconds, that's /2, /3, /4, /5, /6, /10, /12, /15, /20 and /30 because 60 is evenly divisible by those numbers; for hours, that's /2, /3, /4, /6, /8 and /12); all other possible "steps" and all other fields yield inconsistent "short" periods at the end of the time-unit before it "resets" to the next minute, second, or day; for example, entering */5 for the day field sometimes executes after 1, 2, or 3 days, depending on the month and leap year; this is because cron is stateless (it does not remember the time of the last execution nor count the difference between it and now, required for accurate frequency counting—instead, cron is a mere pattern-matcher).

Do you think it's worth adding an extra check to ensure that the step value is evenly divisible? I guess it will be equally surprising to some users that */45 won't be every 45 minutes as */120 not being every 2 hours?

@zslayton
Copy link
Owner

Apologies for the delayed reply. If you're willing to rebase this PR, I'll take a look!

If the specifier is a period specifier, ensure that the interval is
larger than 0 (0 would be every 0th) and less than the max value for the
unit.
@bombsimon
Copy link
Contributor Author

bombsimon commented Oct 28, 2024

Apologies for the delayed reply. If you're willing to rebase this PR, I'll take a look!

No worries! Rebased and update the PR, lmk if I can help make reviewing easier or change anything!

Copy link
Owner

@zslayton zslayton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fix!

@zslayton zslayton merged commit cabee0f into zslayton:master Oct 29, 2024
1 check passed
@bombsimon bombsimon deleted the disallow-invalid-period branch October 29, 2024 13:00
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.

reject invalid cron values
2 participants