Solution for new GetPreviousOccurence
#108
Conversation
|
Build appears to be failing due to an old .NET Standard library reference. /home/appveyor/projects/ncrontab/NCrontab/NCrontab.csproj : error NU3037: Package 'NETStandard.Library 2.0.3' from source 'https://api.nuget.org/v3/index.json': The repository primary signature validity period has expired. |
|
I needed this so I could use CRON to define working windows with a limit of operations between each window |
There was a problem hiding this comment.
Thanks for this contribution, but I'm afraid it still needs a lot of work before it can be merged. It adds considerable new and fundamental logic that's duplicated between TryGetNextOccurrence and TryGetPrevOccurrence, but with very few tests ensuring full coverage (e.g. TryGetNextOccurrence has 0 uncovered blocks but TryGetPrevOccurrence has 11). Not only it adds to maintenance, I'm not confident that it's bug-free. Since you may not always be around to help maintain and fix any reported bugs, I want to avoid that burden falling entirely on me. If you have the will and motivation to address the following points then I'd be happy to work with you to get this into a mergeable state:
- Consolidating as much of the code duplication as possible.
- Ensuring full test coverage.
- Exercising any edge cases you can think of.
PS Also, please bear in mind that issue #64 was never committed as a feature so there is a slight chance that it may not still make the cut. That said, I am open to seeing if this can be done with some reasonable effort.
atifaziz
changed the title
GetPreviousOccurence
|
@atifaziz, I understand your position and made this contribution because I want my company (Servant Software LLC) to go beyond just mere consumption of these open source projects, but to be an active contributor. When contributing this PR, we had some competing priorities and so you're right, for this work to get merged into master it needs more efforts. I'll personally make sure that these are dealt with and provide you with updates. Honestly, I thought that this repo had gone dormant and wanted to get this PR out there before forgetting to post it, so that others (like @SammyROCK) could at least get a fork of it, if they had such a specialized need, like myself. Very glad to see that you breathed new life into the repo today. Thank you! We plan to be here to help you as needed and totally understand if this feature doesn't make the cut. |
@DaveRMaltby You can imagine this is like music to any maintainer's ears and I really commend your attitude and will.
Understandable.
I time-share between many open source project (too many for my own good). There are periods when some get more love than others as I try to avoid moving in and out of different problem spaces so I can do each proper justice. On top of that, this particular library was always meant to do just one thing, so I consider it to be really feature complete (including stability and compatibility) and that too can lead to the impression of it being abandoned/dormant. The one (non-urgent) feature I've been meaning to add since a while is enabling unions of multiple schedules. This may be something to think about with respect to computing previous occurrences.
Looking forward. |
|
For reference, below is the difference between --- .\TryGetNextOccurrence.cs
+++ .\TryGetPreviousOccurrence.cs
@@ -1,4 +1,4 @@
- DateTime? TryGetNextOccurrence(DateTime baseTime, DateTime endTime)
+ DateTime? TryGetPrevOccurrence(DateTime baseTime, DateTime startTime)
{
const int nil = -1;
@@ -9,109 +9,118 @@
var baseMinute = baseTime.Minute;
var baseSecond = baseTime.Second;
- var endYear = endTime.Year;
- var endMonth = endTime.Month;
- var endDay = endTime.Day;
+ var startYear = startTime.Year;
+ var startMonth = startTime.Month;
+ var startDay = startTime.Day;
var year = baseYear;
var month = baseMonth;
var day = baseDay;
var hour = baseHour;
var minute = baseMinute;
- var second = baseSecond + 1;
+ var second = baseSecond - 1;
//
// Second
//
var seconds = _seconds ?? SecondZero;
- second = seconds.Next(second);
+ second = seconds.Prev(second);
if (second == nil)
{
- second = seconds.GetFirst();
- minute++;
+ second = seconds.GetLast();
+ minute--;
}
//
// Minute
//
- minute = _minutes.Next(minute);
+ minute = _minutes.Prev(minute);
if (minute == nil)
{
- second = seconds.GetFirst();
- minute = _minutes.GetFirst();
- hour++;
+ second = seconds.GetLast();
+ minute = _minutes.GetLast();
+ hour--;
}
- else if (minute > baseMinute)
+ else if (minute < baseMinute)
{
- second = seconds.GetFirst();
+ second = seconds.GetLast();
}
//
// Hour
//
- hour = _hours.Next(hour);
+ hour = _hours.Prev(hour);
+ // this variable for trick
+ // to keep day when it adjusted day 31 for
+ // "short" months
+ bool adjusting = false;
+
+ RetryDayMonth:
+
if (hour == nil)
{
- minute = _minutes.GetFirst();
- hour = _hours.GetFirst();
- day++;
+ minute = _minutes.GetLast();
+ hour = _hours.GetLast();
+ day--;
}
else if (hour > baseHour)
{
- second = seconds.GetFirst();
- minute = _minutes.GetFirst();
+ second = seconds.GetLast();
+ minute = _minutes.GetLast();
}
//
// Day
//
- day = _days.Next(day);
+
- RetryDayMonth:
+ day = _days.Prev(day);
+
if (day == nil)
{
- second = seconds.GetFirst();
- minute = _minutes.GetFirst();
- hour = _hours.GetFirst();
- day = _days.GetFirst();
- month++;
+ second = seconds.GetLast();
+ minute = _minutes.GetLast();
+ hour = _hours.GetLast();
+ day = _days.GetLast();
+ month--;
}
- else if (day > baseDay)
+ else if (day < baseDay)
{
- second = seconds.GetFirst();
- minute = _minutes.GetFirst();
- hour = _hours.GetFirst();
+ second = seconds.GetLast();
+ minute = _minutes.GetLast();
+ hour = _hours.GetLast();
}
//
// Month
//
- month = _months.Next(month);
+ month = _months.Prev(month);
if (month == nil)
{
- second = seconds.GetFirst();
- minute = _minutes.GetFirst();
- hour = _hours.GetFirst();
- day = _days.GetFirst();
- month = _months.GetFirst();
- year++;
+ second = seconds.GetLast();
+ minute = _minutes.GetLast();
+ hour = _hours.GetLast();
+ day = _days.GetLast();
+ month = _months.GetLast();
+ year--;
}
- else if (month > baseMonth)
+ else if (month < baseMonth)
{
- second = seconds.GetFirst();
- minute = _minutes.GetFirst();
- hour = _hours.GetFirst();
- day = _days.GetFirst();
+ second = seconds.GetLast();
+ minute = _minutes.GetLast();
+ hour = _hours.GetLast();
+ if (!adjusting)
+ day = _days.GetLast();
}
//
@@ -119,7 +128,7 @@
// object. Otherwise we would get an exception.
//
- if (year > Calendar.MaxSupportedDateTime.Year)
+ if (year < Calendar.MinSupportedDateTime.Year)
return null;
//
@@ -143,25 +152,26 @@
if (day > 28 && dateChanged && day > Calendar.GetDaysInMonth(year, month))
{
- if (year >= endYear && month >= endMonth && day >= endDay)
- return endTime;
+ if (year <= startYear && month <= startMonth && day <= startDay)
+ return startTime;
- day = nil;
+ hour = nil;
+ adjusting = true;
goto RetryDayMonth;
}
- var nextTime = new DateTime(year, month, day, hour, minute, second, 0, baseTime.Kind);
+ var prevTime = new DateTime(year, month, day, hour, minute, second, 0, baseTime.Kind);
- if (nextTime >= endTime)
- return endTime;
+ if (prevTime <= startTime)
+ return startTime;
//
// Day of week
//
- if (_daysOfWeek.Contains((int)nextTime.DayOfWeek))
- return nextTime;
+ if (_daysOfWeek.Contains((int)prevTime.DayOfWeek))
+ return prevTime;
- return TryGetNextOccurrence(new DateTime(year, month, day, 23, 59, 59, 0, baseTime.Kind), endTime);
+ return TryGetPrevOccurrence(new DateTime(year, month, day, 0, 0, 0, 0, baseTime.Kind), startTime);
}Unfortunately, this at the line level, but actually the delta is even less when you look at the level of characters and then the duplication really stands out. If each method is extracted into a separate files, say named |
…ards in time. Created Incrementor and CrontabField.Iterator classes to abstract out the direction in time that we are moving. The TryGetOccurrence method was too long. Refactored out methods to reduce length and to better self-document types of operations occurring. Promoted "nil" constant to class-level internal, to avoid literal uses of '-1' .
@atifaziz, are you using a particular tool to determine the uncovered blocks? I would like use the same tool locally to avoid guessing at when I've covered all blocks with unit tests. Thanks! |
I used the one built into Visual Studio for a very quick and cursory analysis, but unfortunately, the feature is only available in the Enterprise Edition. I'll refresh the coverage & reporting as part of the CI/CD for this repo so it's more accessible and drop a note here when it's available. |
This is now done with #115 and available in |
…ady exist for next occurrences.
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #108 +/- ##
==========================================
+ Coverage 88.37% 89.80% +1.42%
==========================================
Files 5 8 +3
Lines 430 500 +70
==========================================
+ Hits 380 449 +69
- Misses 50 51 +1
☔ View full report in Codecov by Sentry. |
|
@atifaziz, Note: @SammyROCK, there were some bugs that was in the version of my fork that you cloned. You may want to pull down the latest. |
There was a problem hiding this comment.
I believe all your requests have been completed.
Thanks for the follow-up.
I have abstracted the logic such that there is now only one method used by Next and Prev for getting an occurrence called
CrontabSchedule.TryGetOccurrence.
While it's really cool to see the extensive coverage through tests, the changes to the library have got me concerned about three things.
First and foremost, the calculation of a schedule's occurrence was a single and simple method that read pretty linear (minus a goto and recursion), but now the logic is spread across many methods and types, making it difficult to understand and reason about. Moreover, the core algorithm has a lot of code diff noise due to a new feature (calculating an occurrence in the past) that I am not prepared to commit to just yet. What I mean by this is that had the impact and changes been minimal, it would have been easier to review and accept the risk. The code is now even a bit alien to me, as it took me some time to wrap my head around how it was factored to go either direction in time.
Second, the new classes/abstractions have added many new allocations (DateTimeComponents, Incrementor) and cost to the computation, which is a bit of a regression. For example, each call to a ICrontabField.Next causes several allocations (Iterator + Incrementor).
Third and finally, the addition of new members to the ICrontabField are breaking changes.
The good news, I think, is that most of this can be mitigated.
Some of the said abstractions are not true abstractions. For example, Incrementor is just hiding a conditional. For me, an abstraction for Incrementor, for example, would have been something like this:
interface IIncrementor
{
bool BeforeOrEqual(int value, int limit);
bool After(int value, int limit);
bool AfterOrEqual(int value, int limit);
bool AfterOrEqual(DateTime value, DateTime limit);
int Increment(int value);
}with two cached implementation instances since each is stateless:
static class Incrementor
{
public static IIncrementor Forward => new ForwardIncrementor();
public static IIncrementor Backward => new BackwardIncrementor();
sealed class ForwardIncrementor : IIncrementor
{
public bool BeforeOrEqual(int value, int limit) => value <= limit;
public bool After(int value, int limit) => value > limit;
public bool AfterOrEqual(int value, int limit) => value >= limit;
public bool AfterOrEqual(DateTime value, DateTime limit) => value >= limit;
public int Increment(int value) => value + 1;
}
sealed class BackwardIncrementor : IIncrementor
{
public bool BeforeOrEqual(int value, int limit) => value >= limit;
public bool After(int value, int limit) => value < limit;
public bool AfterOrEqual(int value, int limit) => value <= limit;
public bool AfterOrEqual(DateTime value, DateTime limit) => value <= limit;
public int Increment(int value) => value - 1;
}
}Then the selection of the implementation needs to happen once:
var incrementor = _forwardMoving ? Incrementor.Forward : Incrementor.Backward;But if we look at each implementation then most of the abstractions are already covered. For example, the comparison like <, <=, > and >= are already abstracted via IComparable<T>. For integers, this can avoid virtual dispatches through generic implementations.
The Increment method implementations just add or subtract, but those could just be reduced to additions where the second operand is +1 or -1. Anyway, I think you get my point.
My original intention of sharing the delta between the TryGetNextOccurrence and TryGetPreviousOccurrence from the initial commit was to illustrate that there's very little difference between the two. Most of the difference can be covered by an int increment that's -1 or 1 depending on the direction and similarly on results of IComparable<T>.CompareTo. There are some other trickier bits, but I think they can be worked out cleverly (perhaps through delegates as abstraction over signatures). I did a quick & dirty spike around this and all your tests passed, which means that perhaps it requires a less seismic approach. Thoughts?
There was a problem hiding this comment.
These changes here belong in PublicAPI.Unshipped.txt in the same folder as this is new API that's not shipped yet.
There was a problem hiding this comment.
These changes here belong in PublicAPI.Unshipped.txt in the same folder as this is new API that's not shipped yet.
There was a problem hiding this comment.
These changes here belong in PublicAPI.Unshipped.txt in the same folder as this is new API that's not shipped yet.
| int GetLast(); | ||
| #pragma warning disable CA1716 // Identifiers should not match keywords (by design) | ||
| int Prev(int start); |
There was a problem hiding this comment.
These are new members to a publicly shipped interface so really a breaking change. This means this feature would need to be held back until the next major release.
|
Hi there! What is this "prev" feature's status? I also would need it, and I see that it is almost ready. |
|
Hi just coming in to chime this would be an awesome feature, I wrote a hacky solution to achieve the same thing, but would love a built in version! |
|
Could really use this feature as well, any update on the PR ? |
…methods. #64
Moves from the start DateTime in a reverse fashion to avoid a need to move forward and guess how far in the past to begin the search from.