Fix conflict between holidays and special dates

[maread99]

NB. Vast majority of changes relate to regularizing timezone of adhoc_holiday dates (23431fe)

Fixes #33 by allowing special closes / opens to be defined for dates that are holidays and then filtering out these conflicts in ExchangeCalendar._special_dates

Regularizes adhoc_holiday dates as tz-naive (previously a mix of tz-naive, "UTC", "Asia/Jerusalem" and defined on one calendar as list of Holiday instances).

- [ ] If merged and #54 merged then remove extensions of test_start_bound and/or test_end_bound tests marked xfail on test_xist_calendar, test_xkls_calendar and test_xnys_calendar. EDIT: moved to #54

[gerrymanoim]

Does this one also need UTC removed?

[maread99]

Thanks for getting another review in.

I left March1BadWeather as UTC because it feeds through to special_closes_adhoc, not adhoc_holidays. I've added a comment to 'XDUB' to note the difference.

A side note: the suggested fix creates a DatetimeIndex of adhoc_holidays, thereby requiring them to be defined consistently in terms of either UTC or tz-naive (at least within any specific calendar). At the risk of returning to the #42 UTC / tz-naive discussion 😀, I went with tz-naive in light of what they represent. Having worked through the implementation I was happy they could be defined in tz-naive terms (which was pretty clear anyway from the fact that some already were). Also, it didn't seem much point asking someone defining adhoc_holidays to set the timezone to UTC when that extra context isn't used (infact, as in some calendars, they could all just be defined as strings, e.g. "2021-07-09").

I didn't need to work through the workings of special_closes_adhoc and didn't want to risk introducing a bug by changing something for the sake of changing it - allowing special_closes_adhoc to be defined as tz-aware seemed reasonable in any case.

[gerrymanoim]

LGTM - do you mind adding a test to make sure we don't regress later and hit #33 again? I actually think since this removes the manual collisions logic we don't need that.

[maread99]

I've added a quick test to enforce specification of adhoc_holidays as tz-naive (and made a couple of changes that the test raised and I'd missed).

It's worth noting that if someone were to mix tz-naive and UTC adhoc_holidays then this test wouldn't fail, rather an error would be raised when _special_dates tries to create a DatetimeIndex from a combination of UTC and tz-naive dates.

Hence, the only purpose of this test is to avoid someone defining all adhoc_holidays in UTC (which would not otherwise raise an error) which in turn could lead to someone else unwittingly mixing them again.

[maread99]

Hi @gerrymanoim, is there anything outstanding here or would you be happy to merge with the added test?

I've been holding off on PRs for the get_calendar 'side' option until this PR and #54 are resolved. I'm away for quite a while from the end of the month. I appreciate that time is very much a finite resource, although if you were in a position to review the remaining PR I could get the 'side' PRs together before I go.

[gerrymanoim]

@maread99 So sorry - work has been super busy lately.

Appreciate all the work you've been doing, thanks!

[maread99]

@gerrymanoim, no worries at all, time is what it is. Thanks for merging. The other one (#54) is less involved than the number of file changes might suggest (swaps ad hoc out-of-bounds handling with a common base implementation).