-
Notifications
You must be signed in to change notification settings - Fork 68
U/krall/croniter #1108
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
base: master
Are you sure you want to change the base?
U/krall/croniter #1108
Changes from all commits
23e3aa5
2d4ad81
06efbf3
14edb55
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,10 +6,12 @@ | |
| import re | ||
| from collections import namedtuple | ||
|
|
||
| from croniter import croniter | ||
| from croniter.croniter import CroniterBadDateError | ||
|
|
||
| from tron.config import config_utils | ||
| from tron.config import ConfigError | ||
| from tron.config import schema | ||
| from tron.utils import crontab | ||
|
|
||
| ConfigGenericSchedule = schema.config_object_factory( | ||
| "ConfigGenericSchedule", | ||
|
|
@@ -24,7 +26,7 @@ | |
|
|
||
| ConfigCronScheduler = namedtuple( | ||
| "ConfigCronScheduler", | ||
| "original minutes hours monthdays months weekdays ordinals jitter", | ||
| "original jitter", | ||
| ) | ||
|
|
||
| ConfigDailyScheduler = namedtuple( | ||
|
|
@@ -289,18 +291,19 @@ def parse_groc_expression(config, config_context): | |
|
|
||
| def valid_cron_scheduler(config, config_context): | ||
| """Parse a cron schedule.""" | ||
| try: | ||
| crontab_kwargs = crontab.parse_crontab(config.value) | ||
| if crontab_kwargs["monthdays"] is not None and crontab_kwargs["weekdays"] is not None: | ||
| raise ValueError("cannot supply both monthdays and weekdays") | ||
| return ConfigCronScheduler( | ||
| original=config.value, | ||
| jitter=config.jitter, | ||
| **crontab_kwargs, | ||
| ) | ||
| except ValueError as e: | ||
| expression = re.sub(r"\s*,\s*", ",", config.value.strip()) | ||
| if not croniter.is_valid(expression, hash_id="validation_placeholder"): | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this use the actual hash_id instead of a placeholder? If we have someone doing
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Interesting point. |
||
| msg = "Invalid cron scheduler %s: %s" | ||
| raise ConfigError(msg % (config_context.path, e)) | ||
| raise ConfigError(msg % (config_context.path, expression)) | ||
| try: | ||
| croniter(expression, hash_id="validation_placeholder").get_next() | ||
| except CroniterBadDateError: | ||
| msg = "Cron expression %s at %s will never match a valid date" | ||
| raise ConfigError(msg % (expression, config_context.path)) | ||
| return ConfigCronScheduler( | ||
| original=expression, | ||
| jitter=config.jitter, | ||
| ) | ||
|
|
||
|
|
||
| schedulers = { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thoughts on repurposing some of these GeneralScheduler tests for CronScheduler?
I think some of these should be carried forward. E.g.
test_fall_backshould look at CronScheduler("30 1 * * *")test_spring_forwardshould look at CronScheduler("30 2 * * *") to ensure we shift nonexistent times forwardtest_handles_unsetting_the_time_zoneandtest_handles_changing_the_time_zoneboth test behaviour we should maintainThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Jeez,
test_handles_unsetting_the_time_zoneis just incorrect in its current form. I like the spirit of it, but imo it shouldn't start with UTC and it shouldn't assert hour == 0.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
JEEZ,
test_handles_changing_the_time_zoneis also funky. I don't think this should look at datetime.now and I very much disagree with asserting on just the hour.