Holidays only removed at start/end of time period?
Closed this issue · 8 comments
Hi there,
I noticed that when I run difftime_office_hours( as.POSIXct("2016-01-05 8:00:00"), as.POSIXct("2016-01-07 16:00:00"),working_hours = c(8, 16), holidays = "01/05/2016" ), I get 16 hours as expected, but if I add 1/6 as another holiday: difftime_office_hours(as.POSIXct("2016-01-05 8:00:00"), as.POSIXct("2016-01-07 16:00:00"),working_hours = c(8, 16),holidays = c("01/05/2016","01/06/2016")), I still get 16 hours even though I'm expecting to get 8 hours.
Can you confirm this is a bug, or am I doing something incorrectly? If a bug, I can work on a fix.
cc @johncassil - I can also add tests around this theme (variations on holidays in the date range).
I'll look into this and get back to you!
Tests in general would be appreciated. You can probably find a lot of them in previous issues!
confirmed... looks like a bug. I traced it back to this internal function::
grab_non_holidays_only <- function(dates, holidays) {
holidays_to_remove <- holidays %>% lubridate::mdy(tz = 'UTC') %>% as.POSIXct(tz = 'UTC')
non_holidays <- dates [! dates %in% holidays_to_remove]
return(non_holidays)
}
looks like some timezone bullshittery... the holidays get a UTC marker added to them:
> holidays_to_remove
[1] "2016-01-05 UTC" "2016-01-06 UTC" "2016-01-07 UTC"
wheras the dates it's comparing against are getting an EST (for me, at least)
> non_holidays
[1] "2016-01-06 EST"
I could go ahead and try to solve this, but I will let you, if you'd like. Something to consider here is how R and/or lubridate are timezones (looks like it defaults to EST for me), and we have users of this function around the globe.
Let me know if you'd like more help!
Also, If you're trying for credit with hacktoberfest, I don't think that this repo is tagged with hacktoberfest, but you can submit to https://github.com/johncassil/difftimeOffice and I'll merge it in and then pull here... if that's a thing. lol
Interesting, thanks for verifying. What would you think of just requiring the holidays to be input in as POSIXct/POSIXlt the same way the start and end dates are, since they get converted anyway? Then they should automatically all be in the same timezone.
I think that might work. I really don't think that people are going to feed a list of holidays in with timezones, though, so as long as they don't have to specify that. Is there a lubridate function that might help out?
Abi,
I looked into this tonight and found that I was treating holidays in two confusing and different formats. Holidays should be in YMD format... I am changing the logic for this and also adding an error if the holidays aren't entered in right, so that it won't fail silently.
Closed. Thanks for PR @johncassil!