Consider a Holiday class

[jaraco]

The implementation of this Holiday class enables an individual calendar implementer to more accurately describe each holiday using a first-class object. These changes demonstrate how the use of such a class gives more flexibility about the definition of each holiday, including when it is observed.

As a result, this fixes (or provides a mechanism to fix) #76, #77, and #78.

As much as possible, these changes attempt to maintain backward compatibility, but it's plausible that some users will find the changes incompatible with their customizations.

All the tests pass (on Python 3 at least).

I'm not necessarily requesting that these changes be merged as-is, but I did want to collect these changes as a concrete proposal for consideration and discussion. Accepting changes like these would be essential for me to adopt workalendar for my application.

I look forward to your feedback.

[brunobord]

First of all, thanks for you PR. It triggers a lot of thinking, which is good.

If the workalendar rendered calendar is sent as a tuple (date + string as a label), that was because it was simple. No extra class, only using standard-lib types, etc.

Your Holiday class looks interesting, but there's at least one issue to me: the "weekend_hint" which looks like a "mandatory" behaviour. AFAICT not every calendar is using this "shift to the next monday" strategy. In France, for example, if a holiday happens on sunday or saturday, it's not shifted. Let's imagine there's a calendar with 8-day-long weeks. This argument will be very difficult to handle in this Holiday object...
Holiday class should be more "calendar-agnostic" from this point of view.

This triggers me one bigger thought: that you're hiding the "shift / do not shift" logic in the Holiday class.
It doesn't look great to me. Holidays instances should be "dummy" instances that only take care of rendering when they're generated through the calendar. That's the Calendar that does computations and assigns the Holiday instances, computes "business days" adding and all. That's the Calendar class that knows the month or week lengths, and knows when a day must be shifted or not.

Again, I'm quite pleased by your take on the Holiday class, but it bears too much logic to me. I'm sure this class can be simplified, and I'd gladly integrate it into workalendar... Even if it causes me to migrate every other calendar to use it as an output object for calendar rendering.

[jaraco]

Excellent. I completely agree with your assessment. The weekend shift behavior was an artifact of a simplistic (single-calendar) implementation from which I drew. Giving the shift behavior some calendar-context seems like the right thing to do.

Here's the reason that the shift behavior was written into the Holiday class - our company has calendars which include adjacent, floating holidays, such as Christmas and Christmas Eve. When one or both of those holidays lands on a weekend, it's important how the observed date is calculated. As it turned out, the earlier date is always observed on Friday and the latter date always observed on Monday. The calendar itself doesn't know which way to shift an observed Holiday, so it needed a hint.

I'll continue to refine the implementation, encouraged by your conditional endorsement.

Thanks.

[jaraco]

@brunobord I struggled with how to transition from my previous implementation to something that worked better within the calendar model, but I'm pleased to say I'm much happier with this latest implementation, and I think you will be too. It completely abstracts the shift for observation date from the Holiday class (though provides one small hook so the observation shift can be overridden by the holiday). Now the calendar is aware of the indicated holidays (cal.holidays()) and observed holidays (honored by cal.is_working_day). The new Holiday class is less particular about the parameters it requires, but is very lenient about the keyword arguments it allows, enabling an individual calendar implementation to extend the instances with additional detail.

I would like to make more changes, but I've been striving to maintain backward compatibility as much as possible.

If at some point you want to remove the ability for holidays to be defined as two-tuples, much of the compatibility code goes away.

[jaraco]

Will the project consider accepting these changes? They're essential to our use-case and provide a good deal of additional value with minimal imposition.

[brunobord]

Hi,

I have deeply thought about your PR and I've come to this conclusion: I still think that workalendar should keep as lightweight as possible.
Your input is a year, your output is a list or set of easy-to-handle standard types. It's KISS and we like it. After all, workalendar is "just" a simple library. Its main goal is to provide a quick way to compute holidays for a given year and country. What can be done on these dates is another matter.

This is why (and I feel sorry because I think you've worked hard on this feature) your pull-request won't be merged into master.
Still, you have the opportunity to build your own module on top of workalendar, that could extract our basic types and create Holiday instances out of the regular tuples.

Again, thank you for your PR proposal, and I wish you don't keep hard feelings on this project.
Good luck.

[jaraco]

Hi. I'm disappointed in this choice. What dismays me the most is that it's unclear what the stance is of workalendar on the issues that this PR addresses (#76, #77, and #78). Will the project attempt to solve those issues in a different way? Particularly when it comes to the issues of duplicate entries and reflecting the indicated vs. observed dates for Holidays, the calendar as implemented is not adequate.

My goal in using workalendar was to merge the very similar work we were both doing to achieve both of our goals through a single library. I will consider your suggestion to create a separate wrapper library, but that approach seems riddled with problems:

1. The holiday definitions within workalendar aren't adequate for our needs, so every calendar would have to be re-implemented.
2. The support for discerning indicated vs. observed holiday would have to be monkey-patched into the core.Calendar or my implementation would have to subclass core.Calendar, and then re-create concrete classes from that, so every calendar would have to be re-implemented.
3. Creating a test suite that could adequately capture the desired behavior without repeating a lot of code from workalendar would be challenging.
4. The library would need to be kept in tight sync with changes to workalendar.

I also was considering further extending workalendar to support non-working days of other types (sick leave, vacation, etc), but if the intention of the project is to limit innovation to a certain scope that only uses simple types for anything except Calendar objects, it's unlikely that will happen either.

Given that workalendar is deciding to limit its scope of development, I'm inclined to think that a fork of the project would be preferable.

Since it is your project, I respect your decision.