search

18F / standup-slack-bot

A Slack bot to streamline team standup without disturbing the overall flow of conversation
https://standup-slack-bot.app.cloud.gov/
Other
87 stars 31 forks source link

Only show reports on business days #19

Closed mgwalker closed 8 years ago

mgwalker commented 8 years ago

Just as #4 suggests. Also adds US federal holidays to the US locale, so holidays will not be considered business days.

stvnrlly commented 8 years ago

Nice!

Would it be simpler, though, to change schedule.scheduleJob('* * * * 1-5', botLib.getReportRunner(bot)) and then just a fedHolidays.isAHoliday(myDate) check?

mgwalker commented 8 years ago

This is why I like PRs and code review - changing the cron schedule hadn't even occurred to me. I'm not sure about "simpler," but it certainly wouldn't be any more difficult. And it'd mean removing the moment-business-days dependency.

My only reservation about that would be that there are two places involved in determining when to run the report function, but that's already true with this PR so ¯_(ツ)_/¯. I like that changing the cron schedule eliminates 2,880 checks per week, though. :stuck_out_tongue:

stvnrlly commented 8 years ago

Cool, then let's eliminate that dependency. Would you like to make that change or should I?

mgwalker commented 8 years ago

Go for it if you want! If it's still open Tuesday, I'll knock it out.

mgwalker commented 8 years ago

Got it. Changed the cron schedule and checking federal holidays directly.

stvnrlly commented 8 years ago

:rocket: