Skip to content

Allow 5-field expressions#136

Open
Arshia001 wants to merge 2 commits intozslayton:masterfrom
Arshia001:optional-seconds-field
Open

Allow 5-field expressions#136
Arshia001 wants to merge 2 commits intozslayton:masterfrom
Arshia001:optional-seconds-field

Conversation

@Arshia001
Copy link
Copy Markdown

This is my proposed implementation for #13. This allows 5-field expressions to be parsed without any change in the existing behavior. There's certainly more that can be done in this area, but I feel this is a useful step in the right direction.

@zslayton
Copy link
Copy Markdown
Owner

When I started this, I was following the Quartz scheduler's cron syntax spec, which doesn't recognize 5-field expressions. However, there's no cron standard, only lots of different conventions of varying popularity. Is there one out there that accepts 5-field expressions? If so, I think we can merged this.

@Arshia001
Copy link
Copy Markdown
Author

Is is my understanding that the cron implementation in linux only supports 5 fields. It is also my understanding that this is the more ubiquitous format; for example, saffron (from cloudflare) only supports 5-field expressions: https://github.com/cloudflare/saffron/blob/bb9bb16e112f9ab69161d55181ffc259708824ae/saffron/src/parse.rs#L632

I do need to fix the CI though, just noticed it's red.

@zslayton
Copy link
Copy Markdown
Owner

Cool, thanks. Can you add a unit test that iterates through the output of a couple of example 5-field schedules to make sure they work as expected?

@dufferzafar
Copy link
Copy Markdown

Just to add a note that wikipedia page on Cron also considers 5 fields to be standard: https://en.wikipedia.org/wiki/Cron

Comment thread src/parsing.rs
terminated(fields, eof)
.map(|(minutes, hours, days_of_month, months, days_of_week)| {
ScheduleFields::new(
Seconds::all(),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this should be zero right? You wouldn't want a 5 part to run every second.

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