Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added the ability to explicitly calculate and cache a given range of holidays #36

Closed
wants to merge 2 commits into from

Conversation

ndbroadbent
Copy link
Contributor

Hi there,

Our app does a lot of date calculations, and we previously had a hard-coded list of holidays. After switching to the holidays gem, we noticed that some of our requests were taking 30% longer because of many holiday calculations.

This pull request adds a simple Hollidays.cache_between method, so that we can calculate and cache a list of holidays when the app boots. For example, I've added this in an initializer:

Holidays.cache_between Time.now, 2.years.from_now, :us, :observed

Please let me know if you think there's a better way to do this.

@hahahana
Copy link
Contributor

Thanks, this looks great. I'll take a look ASAP, hopefully we can get this all gem'd up sometime next week.

@hahahana
Copy link
Contributor

Hi, sorry for the delay, I'm looking at this right now. It'll take a little more time to test it though vs the other pull requests. After going through all the code, I'm a little wary of releasing a gem atm, just because I feel like we are missing a lot of test coverage for the rest of this decade (and have been getting pull requests along the lines of, this definition is wrong/missing, etc.) and I'm thinking about a way to make the data in the base a little more manageable. Basically, right now my main worry is the underlying data itself. But don't worry, I have definitely not forgotten about this part and I really like it a lot.

@ghiculescu
Copy link
Contributor

How does everyone feel about merging this in?

@ppeble
Copy link
Member

ppeble commented Feb 14, 2015

@ghiculescu This is my next priority. I'll spend some time tonight looking it over. In the meantime, could you merge in the latest master to resolve the conflicts? That way we will have a clean merge.

EDIT: Oops, I realize now that this wasn't yours originally, @ghiculescu. My mistake!

@ndbroadbent I know that it has been a while but would you mind merging in the latest master?

ghiculescu added a commit to TandaHQ/holidays that referenced this pull request Feb 14, 2015
@ghiculescu
Copy link
Contributor

@ptrimble I actually tried merging it against master on my own fork anyway. Seemed to go fine.

TandaHQ@7a0cc60

Do you want me to create a new pull request?

@ppeble
Copy link
Member

ppeble commented Feb 15, 2015

Sure, that sounds great. I'll close this one once you create the new PR and we'll merge it in.

@ppeble
Copy link
Member

ppeble commented Feb 15, 2015

Oh, and while we're at it, would you mind adding a small section to the README to show how this functionality is used?

@ghiculescu ghiculescu mentioned this pull request Feb 15, 2015
@ghiculescu
Copy link
Contributor

@ptrimble -- see #92

@ppeble
Copy link
Member

ppeble commented Mar 18, 2015

Here we are! I figured things out and put your names on the commits in a separate PR. Once the builds are green I'll merge them in and our long nightmare will be over. 😉

Thanks for your patience, all! I'm going to close this out in favor of this one: #100

@ppeble ppeble closed this Mar 18, 2015
@ppeble
Copy link
Member

ppeble commented Mar 19, 2015

All done! Thanks again, guys, Release forthcoming.

Murphydbuffalo pushed a commit to Murphydbuffalo/holidays that referenced this pull request Sep 21, 2021
Korean Thanksgiving is on September 15, not September 12 in 2016.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants