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-18 : fix issue #18 - about cron with multiple hours #19

Closed
wants to merge 1 commit into from
Closed

FIX-18 : fix issue #18 - about cron with multiple hours #19

wants to merge 1 commit into from

Conversation

DaaN88
Copy link

@DaaN88 DaaN88 commented Jul 17, 2021

Check it out please! I quickly ran, everything seems to work and the error with this cron is gone :-)

@lorisleiva
Copy link
Owner

Hi there 👋

Sorry about the delay! Would you mind adding some tests to make sure the added logic create sentences that make sense? 🙂

@lorisleiva lorisleiva linked an issue Jul 27, 2021 that may be closed by this pull request
@DaaN88
Copy link
Author

DaaN88 commented Jul 27, 2021

Hello! :-)
There is no point in adding tests now, because my fix only partially solves the problem. At this point, you need to think about regular code breakouts. The bottom line is that cron will be valid if you transfer as many periods as you like in a block for hours, days, etc. That is, my fix works fine if there are two periods, but if you pass three or more, then again a parsing error ... And here's how to write a regular season to take into account an unlimited number of periods, I have not figured out yet.

@DaaN88
Copy link
Author

DaaN88 commented Jul 27, 2021

That is, the crons of the form * / 15 0-8,21-23 * * * or * / 15 1 2-3,3-4 * * are now recognized correctly (in my PR).
But the crons of the form * / 15 0-8,21-23.3-4 * * * or * / 15 1 2-3,3-4,5-8 * *, well, and more periods (separated by commas) will cause a parsing error

@DaaN88
Copy link
Author

DaaN88 commented Jul 27, 2021

if in this regular expression / ^ ([0-9] +) \ - ([0-9] +), ([0-9] +) \ - ([0-9] +) $ / add another block separated by commas ([0-9] +) \ - ([0-9] +), then three periods will be processed correctly. But there can be a lot of periods, as I already wrote. How many such blocks are needed? in fact, this regular expression should be modified to search for all periods listed with commas ...

@lorisleiva
Copy link
Owner

Got it! Yeah we need a more clever regex or a more clever parsed that keeps track of operator precedence.

I'll close this for now as it does not fix the issue at hand but feel free to try again on another PR. 🙂

@lorisleiva lorisleiva closed this Jul 27, 2021
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.

Problem with cron
2 participants