seatgeek / businesstime

A simple python utility for calculating business time aware timedeltas between two datetimes
BSD 2-Clause "Simplified" License
85 stars 36 forks source link

Holiday generator misses previous holidays #25

Closed seantsb closed 6 years ago

seantsb commented 6 years ago

When isholiday is called, the holiday generator is invoked. But the holiday generator maintains a cache, and it assumes that future calls to isholiday will be in the future (while len(self._holidays) == 0 or dt > self._holidays[-1]). However, my code iterates backward and breaks this, because the generator is "forward-only".

agans commented 6 years ago

It seems it misses holidays when the higher date is checked first.

>>> businesstime_cal.isholiday(some_date) # 31st May 2018
False
>>> businesstime_cal.isholiday(mem_date) # 28th May 2018
False
>>> businesstime_cal = BusinessTime(holidays=USFederalHolidays(),business_hours=(datetime.time(hour=9),datetime.time(hour=20)))
>>> businesstime_cal.isholiday(mem_date) # 28th May 2018
True
>>> businesstime_cal.isholiday(some_date) # 31st May 2018
False
>>> 

So we can either create a new instance for each date, or check dates going from lowest to highest.

@erwaller was this intentional here: https://github.com/seatgeek/businesstime/blob/master/businesstime/__init__.py#L42

erwaller commented 6 years ago

Definitely not intentional. I think the problem is that the logic above doesn't reset self.holidays to [] and so fails to actually blow away the temporary storage and cause it to get rebuilt across the proper dates.

@agans are you up for creating a failing test and trying to fix it?

agans commented 6 years ago

@erwaller Sure! I will take a closer look at it sometime this week or early next week.