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: parsing of day of week when # used #782

Closed
wants to merge 1 commit into from

Conversation

shohag121
Copy link

Currently, we only can use 1 to 5 as a "day of week" when we use # sign with it.

In this PR, Day of week range check is set to 0 to 7.
it fixes the parsing of string with # sign. eg. 0 12 ? 1/1 SUN#2 *

fix: #781

@shohag121
Copy link
Author

Hi @barryhughes,

Would you kindly review this PR and let me know if any additional change is needed?

Thanks.

@barryhughes
Copy link
Member

Sorry for the delay here, @shohag121. Testing this out:

Expression Before fix After fix
⛔️ 0 12 ? 1/1 SUN#2 * Must be a value between 1 and 5 Impossible CRON expression
⛔️ 0 12 * * SUN#2 Must be a value between 1 and 5 Impossible CRON expression
0 12 * * SAT#2 Must be a value between 1 and 5 (Works!)
0 12 * * MON#2 (Works!) (Works!)

It looks like:

  • In the first two cases (using Sunday), the exception you noted is solved but we hit a different problem (impossible CRON expression)
  • In the third case (using Saturday), the exception you noted is solved and there is no further issue
  • In the final case (using Monday - Friday), there is no change

Do you find the same? It does look like this yields an incremental improvement—as demonstrated by the third scenario—but some further work is needed to fully smooth things out. Ideally, we would also add some further test cases to ActionScheduler_CronSchedule_Test (and we could help with that if needed).

@barryhughes
Copy link
Member

📝 Note to self: so far as I can tell, this lib is a custom fork that lives directly within our repo (ie, it is likely fine to adjust directly).

@rrennick
Copy link
Collaborator

so far as I can tell, this lib is a custom fork that lives directly within our repo (ie, it is likely fine to adjust directly).

It is to both.

@barryhughes barryhughes changed the base branch from master to trunk March 9, 2022 00:17
@barryhughes
Copy link
Member

Closing/stale.

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.

Limitation of as_schedule_cron_action()
3 participants