Fix CronTrigger.NextFireTime() implementation #52
Conversation
|
Hey @reugn, will this be considered soon? I'm currently considering using this wonderful library and this seems like a crucial bug |
|
@reugn, if you have no intention to maintain the project, could you kindly clarify your position? Perhaps we can transfer it to a community-owned repository. It would be a great shame to abandon such a valuable project. As a sidenote, the cron-related functionality could be moved to a separate library. After a quick search, it appears that the only other project that offers similar functionality is robfig/cron, which has been unmaintained since January 2021, with numerous pending pull requests and issues. |
Codecov Report
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more. @@ Coverage Diff @@
## master #52 +/- ##
==========================================
+ Coverage 85.39% 85.90% +0.50%
==========================================
Files 7 8 +1
Lines 678 532 -146
==========================================
- Hits 579 457 -122
+ Misses 74 53 -21
+ Partials 25 22 -3
|
There was a problem hiding this comment.
Hi @joaquinrovira,
Thanks for your time to look into the project and your contribution.
Since the PR is a voluminous rework, I had to postpone reviewing it.
Here are my initial comments before we can merge.
- Please add documentation for all new exported items (possibly make the
csmpackage internal) - Add a
doc.goto thecsmpackage with a brief description of the functionality - Address linter warnings
- Add an empty line between methods where it is missing
quartz/csm.go
Outdated
| CSM "github.com/reugn/go-quartz/quartz/csm" | ||
| ) | ||
|
|
||
| func MakeCSMFromFields(prev time.Time, fields []*cronField) *CSM.CronStateMachine { |
quartz/csm.go
Outdated
| csm := CSM.NewCronStateMachine(second, minute, hour, day, month, year) | ||
| return csm | ||
| } | ||
| func MakeCSMFromCronTrigger(prev time.Time, trigger *CronTrigger) *CSM.CronStateMachine { |
|
On it! Thank you for your time. |
|
Fixed the issues mentioned and added an explanation in |
|
@joaquinrovira, please run |
Done. |
Proposed fix for issue #51. I have also updated and validated the tests in
cron_test.gowith Cronmaker, an online website that uses the original Quartz library to return the next fire times of a given Cron expression.Hope this helps, and I welcome any discussion regarding the implementation.