Fix scheduling issue #41 #42
Conversation
lib/cron.dart
Outdated
| @@ -47,6 +47,24 @@ class Schedule { | |||
| /// The weekdays a Task should be started. | |||
| final List<int>? weekdays; | |||
|
|
|||
| /// The second a Task last run. | |||
| int? lastSecond = -1; | |||
There was a problem hiding this comment.
I think these fields should be private (_lastSecond) and if you initialize them, they can be int instead of int?. e.g. int _lastSecond = -1;.
lib/cron.dart
Outdated
| final currentDay = (days == null) ? days : time.day; | ||
| final currentMonth = (months == null) ? months : time.month; | ||
| final currentWeekday = (weekdays == null) ? weekdays : time.weekday; | ||
| if (currentSecond == lastSecond && |
There was a problem hiding this comment.
Please make the condition expression a separate variable and add comment on what that means...
// comment
final xyz = currentSecond == lastSecond && .... ;
if (xyz) {
return false;
}
There was a problem hiding this comment.
if ((seconds == null || lastTime.second == time.second) &&
(minutes == null || lastTime.minute == time.minute) &&
(hours == null || lastTime.hour == time.hour) &&
(days == null || lastTime.day == time.day) &&
(months == null || lastTime.month == time.month) &&
(weekdays == null || lastTime.weekday == time.weekday)) {
return false;
}
I think there is no need to comment here, the condition means the whole logical. seconds equals null means any second can trigger cron, so no matter seconds equals null or second match last datatime means the condition for the task has been ran.
lib/cron.dart
Outdated
| currentDay == lastDay && | ||
| currentMonth == lastMonth && | ||
| currentWeekday == lastWeekday) return false; | ||
| lastSecond = currentSecond as int?; |
There was a problem hiding this comment.
I think the cast is not needed here.
|
I'm also wondering if we could just store the |
i have pushed new commit with datetime. |
lib/cron.dart
Outdated
| @@ -47,6 +47,9 @@ class Schedule { | |||
| /// The weekdays a Task should be started. | |||
| final List<int>? weekdays; | |||
|
|
|||
| /// The datetime a Task last run. | |||
| DateTime lastTime = DateTime(0, 0, 0, 0, 0, 0, 0); | |||
There was a problem hiding this comment.
Thinking about it more, I think this field shouldn't be here, rather it should be moved to _ScheduledTask.
The check should be in
void tick(DateTime now) {
if (_closed) return;
if (!schedule.shouldRunAt(now)) return;
// this is the new part:
if (<big-condition>) {
return;
}
_run();
}
The reason being: Schedule instances may be shared even between Cron instances, and we could get parallel updates. _ScheduledTask is private to the Cron instance and it is not shared, it would be the correct place to put this field in.
|
@isoos all done. |
|
I think this is almost ready to be merged, however, two things: the CI tests are failing ( Also, please add this to the top of the changelog: |
ccl0326
force-pushed
the
clchen_dev
branch
2 times, most recently
from
d01ed02
to
096a1a6
Compare
|
@isoos all done. |
|
there are still a few tests failing, but I'll fix them after merging |
|
Published as 0.5.1. |
No description provided.