-
Notifications
You must be signed in to change notification settings - Fork 15
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 timer trigger #70
Conversation
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.
I'd love to see some tests! :)
executionContext.getHour().map(String::valueOf).orElse("*"), "*", "*", "*"); | ||
String cron = String.format("%s %s %s %s %s %s", executionContext.getSecond().map(String::valueOf).orElse("0"), | ||
executionContext.getMinute().map(String::valueOf).orElse("0"), | ||
executionContext.getHour().map(String::valueOf).orElse("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.
Why use %s with the last 3 static fields? * * *
?
I find the code reasier to read the fewer replacements there are.
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.
I wasn't sure if i should add flags for day, month, day-of-week to JRuleWhenTimeTrigger as well. This is the preparation for this. I could remove the "unused" %s flags. Shall I?
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.
@seaside1 wdyt?
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.
So, I guess it is easier to have fewer replacements, but then again if you plan on adding flags for day, month etc. Then maybe not worth changing back?
@querdenker2k could you rebase? |
Probably my fault since I renamed the TimerExecutor to JRuleTimeExecutor, but should be easy to fix. |
46b408f
to
817dc51
Compare
Oh sorry @querdenker2k you just pushed seconds before me ;) |
crons was generated false.