Fixing _getNextDateFrom function logic #359
Conversation
lib/cron.js
Outdated
| var date; | ||
| if(zone){ | ||
| date=moment(start).tz(zone); | ||
| } |
There was a problem hiding this comment.
The formatting here seems to be messed up. I'm going to add a eslint+prettier at some point in the near future. In the meantime can you change this to } else { and then being the last } in line? Thanks!
There was a problem hiding this comment.
So sorry about that 👍
| date.add(1, 'M'); | ||
| date.date(1); | ||
| date.hours(0); | ||
| date.minutes(0); | ||
| date.seconds(0); | ||
| continue; | ||
| } | ||
|
|
||
| if (!(date.date() in this.dayOfMonth)) { | ||
| if (!(date.date() in this.dayOfMonth) && Object.keys(this.dayOfMonth).length!=31) { |
There was a problem hiding this comment.
Can you add some testing to ensure that this wont break months that have less than 31 days?
There was a problem hiding this comment.
@ncb000gt The logic behind this condition Object.keys(this.dayOfMonth).length!=31 is to know whether the dayOfMonth is set to * in the cronString or not. This has nothing to do with the current Date date.date()
lib/cron.js
Outdated
| break; | ||
| } | ||
|
|
||
| if(origDate.valueOf()==start){ |
There was a problem hiding this comment.
Why would we want to go back a second here?
There was a problem hiding this comment.
@ncb000gt removed in commit 98dad71
|
Thanks for putting up this pr! |
|
@ncb000gt Can you please review again, i added new tests and made a couple of changes. Let me know what your thoughts and suggestions are. |
|
Landed. Thanks @jodevsa! I'll try to get a new version out today. |
|
@ncb000gt That would be awesome. Thanks. |
Hi @ncb000gt
As Discussed in the previous PR, this fix will cover the following cases:
extracting the next day from the following cron strings
and many others where * is used in minute/hour/day/week
Thanks alot.