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

FIX: Easter Saturday not a public holiday in New Zealand #177

Merged
merged 5 commits into from
Apr 11, 2016

Conversation

ghiculescu
Copy link
Contributor

@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) to 88.431% when pulling 9078cda on ghiculescu:master into daeb14a on holidays:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 88.551% when pulling 6199053 on ghiculescu:master into daeb14a on holidays:master.

@@ -161,6 +161,22 @@ def to_weekday_if_boxing_weekend(date)
DateCalculatorFactory.weekend_modifier.to_weekday_if_boxing_weekend(date)
end

def to_tuesday_if_sunday_or_monday_if_saturday(year)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For future reference, I am considering whether these methods need to actually be 'public'. I think with my large refactor of 'custom methods' we can reference the factory directly in the YAML files. The only reason these are publicly available is because of how custom methods used to work.

I definitely don't want you to change this because this is how it works today, just wanted you to be aware that I might be changing this in the near future. End result will be the same. 😄

@ppeble
Copy link
Member

ppeble commented Apr 8, 2016

@ghiculescu A few comments/questions.

In addition, can you please add tests for the new 'weekend modifier' methods you added? They go here: https://github.com/holidays/holidays/blob/master/test/holidays/date_calculator/test_weekend_modifier.rb

@ppeble
Copy link
Member

ppeble commented Apr 10, 2016

@ghiculescu I posted this in the other PR as well, FYI.

I've changed my thinking just a bit based on the work I've done this weekend on my refactor branch. Let me lay out my thoughts and you can let me know what you think about the timeline and how reasonable (or unreasonable!) it is.

My current plan was to include this PR in a 3.3.0 release so that it was available before my 4.0.0 breaking change. This way users could receive the benefit of your work but not need to worry about the breaking changes contained in my release. However, I don't want to let my large refactor drift too much. It's been sitting out for 10 days now. So with that in mind I'm going to set an entirely arbitrary deadline for your PR of Sunday, 4/17.

There is absolutely nothing concrete about this date. It is totally open for negotiation. If you think you'll need more time or if you think this date is unreasonable then let's talk!

We have some options:

  • push this arbitrary date back a bit so we can get this PR in with 3.3.0
  • I can merge my changes in and you can release this PR in a future point release (like 4.1.0). The downside to this one is that you'll be required to manage more merge conflicts. The refactor was pretty big and while I personally think it is manageable it will be more work for you. 😦

Let me know what you think! Again, no need to stress, we can work something out that works for both of us.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.08%) to 88.329% when pulling 684fe4b on ghiculescu:master into daeb14a on holidays:master.

@ghiculescu
Copy link
Contributor Author

Tests added: 12019fb

I'm pretty happy to include this in 3.3.0 if you are @ptrimble

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.06%) to 88.348% when pulling 12019fb on ghiculescu:master into daeb14a on holidays:master.

@ppeble
Copy link
Member

ppeble commented Apr 11, 2016

Thank you @ghiculescu! I appreciate the fast response. I'll definitely get this out before the 4.0.0 release.

@ppeble ppeble merged commit e9ab549 into holidays:master Apr 11, 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.

None yet

3 participants