spoulson / Itinerary

Lazy time scheduling toolset for .NET
29 stars 7 forks source link

#L not working correctly in Cron DayOfWeekSpec #13

Closed BennyM closed 9 years ago

BennyM commented 12 years ago

Add this testcase to your scheduler test:

new ScheduleUnitTest("Last Friday of every month",
    new CronSchedule("0", "0", "*", "*", "FRI#L", TimeSpan.Zero),
    new DateTime(2012, 1, 1, 0, 0, 0), new DateTime(2012, 6, 1, 0, 0, 0),
    new TimedEvent[] {
        new TimedEvent(new DateTime(2012, 1, 27, 0, 0, 0), TimeSpan.Zero),
        new TimedEvent(new DateTime(2012, 2, 24, 0, 0, 0), TimeSpan.Zero),
        new TimedEvent(new DateTime(2012, 3, 30, 0, 0, 0), TimeSpan.Zero),
        new TimedEvent(new DateTime(2012, 4, 27, 0, 0, 0), TimeSpan.Zero),
        new TimedEvent(new DateTime(2012, 5, 25, 0, 0, 0), TimeSpan.Zero)
     }
   )
SaintPeter commented 12 years ago

I am experiencing a related issue. I have found at least one problem with "ComputeLastDOWOccurance". You add 1 to the "Month" which breaks if the Month is 12. See my solution below.

 private int ComputeLastDOWOccurance(int Year, int Month, int DOW) {
         var A = new DateTime(Year, Month, 1).AddDays(DOW);
         //var B = new DateTime(Year, Month + 1, 1).AddDays(-1);
         var B = new DateTime(Year, Month, 1).AddMonths(1).AddDays(-1);
         return ((int)A.DayOfWeek + B.Day) / 7 - 1;
      }

However, this exposes another issue: When you choose a day which only has 4 instances in a month, that month is not returned. I suspect something to do with the return statement but have not fixed it yet.

spoulson commented 12 years ago

Interesting. If you have a patch, I'd be willing to give it a peer review and merge. Or, however it is you'd do this in Github...

SaintPeter commented 12 years ago

I have a patch! I'm pretty sure that I've even figured out how to submit it to Github. It's all very magical and I'm learning exciting things.

I'm reasonably confident that it is right - I checked it against the next year or so worth of dates, for both last Friday and Last Monday. I'd feel better if you could confirm the solution.

Also, thanks for writing such a useful bit 'o code. Really saved me a bunch of time, even with having to write a patch for it. :)

spoulson commented 9 years ago

SaintPeter, this is long long overdue on my part. I merged in your fix. Thanks for the contribution!