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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove :au_tas test for the time being #35

Merged
merged 1 commit into from
Apr 8, 2017

Conversation

ppeble
Copy link
Member

@ppeble ppeble commented Apr 8, 2017

Hey @ghiculescu and @adamlyons2. Unfortunately I have more bad news here. What I've discovered is that everything is working as intended, except...for this in the ruby repo: https://github.com/holidays/holidays/blob/master/lib/holidays/finder/rules/in_region.rb#L9-L21

Basically, when we determine whether a specific date is 'in a region' we also currently check its parent region. By adding back in :au we are telling :au_tas to also grab any :au holidays, which means it ALSO picks up the NYD from 1/1. This isn't what you wanted but...I don't see an easy fix for the moment.

The problem is that there are a lot of other tests/features that rely on this functionality. But you want to be able to say :au_tas and have it NOT pick up any :au holidays. I think this is a new feature (which is totally fine) but I was hoping you could provide some feedback. What if we added a new flag to the ruby gem to 'ignore parent regions' if specified? For example, you could call:

Holidays.on(Date.civil(2017, 1, 1), :au_tas, :exact)

And it would only pull back exact matches for the region specified, not doing any expansion.

I don't know if :exact is a good name or what. But I think it makes sense that 'sub regions' pick up their parents by default...that makes the most sense to me. So adding in a flag to disable that might be the best option.

I know I'm going back and forth on this and I apologize. Last three months have been a blur and I haven't really done you right on this front. 馃槮

Thoughts? I'm gonna merge this in so I can get green builds on the ruby side (since it's been so long) but we can definitely talk about it in the coming days.

@ppeble ppeble merged commit f919ebe into holidays:master Apr 8, 2017
@ppeble ppeble deleted the more_au_fixes branch April 8, 2017 19:22
@ppeble ppeble mentioned this pull request Apr 13, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant