Closed bchavez closed 7 years ago
Hi Eric,
I made Recurrence
work with static and instance holiday collections. You review the changes here: https://github.com/bchavez/PDI/commit/c7f683d050cb627ddfb121a56b16029320571a1e
Everything builds, demos, doc examples, and version numbers updated. Let me know your thoughts and/or recommendations. I'd be happy to send you the changes if you approve.
I think the changes I made might be a good tradeoff to keep the same static behavior of Recurrence
while still allowing modification of the Recurrence.Holiday
property per Recurrence
instance.
Thanks, Brian
The problem is that your changes are a significant breaking change. I would much prefer to see an additional InstanceHolidays
property added for example that returns a local holiday collection instance or the static Holidays
collection if the instance copy is null.
It minimizes the changes to just the Recurrence
class and maintains API compatibility with prior releases. All existing code will work as-is and you are free to assign instance based holiday collections as needed.
It may be odd to have both a static and instance based property for holidays but it's been static for over a decade and I'd rather not force rewriting all existing code to handle one specific use-case.
Hi Eric,
I understand your concerns. Thanks for letting me know your thoughts.
I would agree also, it is odd and confusing to have two kinds of holiday properties. I think having both static + non-static could also encourage new users to mistakenly use holidays properties and end up with weird date/time calculations for the API user. :crying_cat_face:
What should we do if both are used? Should we add exception guards that only allow one (static or non-static, but not both) holidays to avoid confusing situations?
Thanks, Brian
Hey Eric,
I started over and I run into CanOccurOnHoliday = false
which manipulates a global static variable for all Recurrence
instances. :crying_cat_face:
public bool CanOccurOnHoliday
{
get { return canOccurOnHoliday; }
set
{
canOccurOnHoliday = value;
if(!value && holidays == null)
holidays = new HolidayCollection();
}
}
How are we going to present the semantics to the API end user? Should we duplicate this property too? CanOccurOnInstanceHoliday
?
Many thanks, Brian
Hi Eric,
I think I got us out of the CanOccurOnHoliday
predicament. You can review the changes here: https://github.com/bchavez/PDI/commit/40094e54fe8ace514c171afe70cf035362a43aa8
The changes should not be breaking changes I think. Let me know.
Thanks, Brian
Rereading your original post I see that what you are trying to do is effectively exclude an arbitrary set of dates, not necessarily holidays. I missed that yesterday and got focused on the holiday collection. Sorry about that.
All things considered, a simple ExcludedDates property of the type IEnumerable<DateTime>
is probably all that's needed. A simple check would then be all that's needed in GenerateInstances
to exclude them:
public IEnumerable<DateTime> ExcludedDates { get; set; }
if(this.ExcludedDates != null && this.ExcludedDates.Contains(rdt.ToDateTime().Date))
{
rdtc.RemoveAt(idx);
idx--;
count--;
continue;
}
If you needed to include times you could probably add a Boolean property that indicates whether or not the ExcludedDates
values contain a time. If they do, the exclusion check can factor them in accordingly.
Hi Eric,
Thanks for your guidance. It's really helpful. My specific problem domain with excluded dates is, they work exactly like Holiday
s where excluded date can be Fixed
and Floating
exclusion dates.
In your suggestion, it seems more like PDI would more work with fixed excluded dates only? Would I have to resolve these Fixed
and Floating
DateTime
s manually if we used Recurrence.ExcludedDates
? I kinda have a feeling the code might not be so good. =(
Let me know your thoughts.
Thanks, Brian
If you know the years between which you are generating recurrence dates, you can generate all of the possible dates using the holiday collection (HolidaysBetween
) and assign that to the property. If that's not feasible, the InstanceHolidays
approach may be the better choice. If so, I'd make a few adjustments.
The CanOccurOnHoliday
property can be left as is (revert the change on line 213). There's no guarantee that the instance holidays will be set before it. Also, there's no harm in having both the static and instance collections set. The instance collection will take precedence and, if set back to null, it would revert to using the static collection. All things considered, the creation check could probably be removed and instead use this.Holidays
in GenerateInstances
since the property will also create the collection on first use.
On a similar note, I'd remove line 440 in the InstanceHolidays
setter that sets the static collection to null. Doing so would clear the holidays for instances not using the instance-based collection in the event they were both used.
Eric
Hi Eric,
Thanks again for your feedback. I think InstanceHolidays
approach is the better choice and most flexible. So, I've reverted some changes per your suggestions. How's it looking now?
https://github.com/bchavez/PDI/commit/708f22e2d0aaf217b75859b75bbbee4f2c922ee6
Thanks, Brian
It looks good. Submit a pull request and I'll merge the changes.
Hi Eric,
Thanks again for open-sourcing PDI.
I'm trying to use
Recurrence
for some date calculations, but I have a situation where I need to maintain two sets of "holidays" (basically using it as an exclusion list of dates) for recurrence calculations.Would you consider making
HolidayCollection holidays
inRecurrence
non-static so different instances ofRecurrence
can use differentHolidayCollection
s?Another alternative is to have a
Recurrence.ExcludeCollection
that works similarly toHolidayCollection
? Let me know your thoughts. I'd be more than happy to send you a pull-request.Thanks, Brian