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

Discuss AU holidays #37

Open
ppeble opened this issue Apr 13, 2017 · 2 comments
Open

Discuss AU holidays #37

ppeble opened this issue Apr 13, 2017 · 2 comments

Comments

@ppeble
Copy link
Member

ppeble commented Apr 13, 2017

I merged a PR that removed a test from :au_tas here: #35

I laid out why I did this and what we need to discuss. In short: any :au holiday will also apply to :au_tas. Since we have New Year's Day specified here with :au when you call :au_tas it will pick it up.

This is not what we have specified that we want. We want to call :au_tas and have it pick up ONLY the :au_tas holiday.

One solution that I just thought of: what if a subregion has the exact same holiday definition? If it does, could it just replace the 'parent' region's definition automatically? I think that would give us what we want.

Another solution is to add some kind of flag to not apply parent region holidays when querying a subregion. I'm not sure what users will want more.

Tagging @ghiculescu and @adamlyons2 for this discussion.

@ghiculescu
Copy link
Contributor

You say that any :au holiday will also apply to :au_tas too. How does that work with ANZAC Day? According to my link there's an :au wide definition, then more specific ones for the states that handle how it is observed.

(I haven't been following this closely over the last two weeks so forgive me if this has already been answered.)

@ghiculescu
Copy link
Contributor

Ignore the above, just read #35 and the attached code and now I better understand. In ANZAC Day's case we are adding additional dates for particular regions, whereas for NYD we want to remove a date from applying for a particular region.

Looking at Adam's original commit, how about this as an API:

  - name: New Year's Day
    regions: [au, au_nsw, au_vic, au_act, au_sa, au_wa, au_nt, au_qld, au_tas]
    mday: 1
    observed: au_nyd_observed_to_monday_if_weekend(date, region)
    function: au_nyd_function_to_monday_if_weekend(date, region)

I should learn more about the holidays internals - I don't really know how feasible this is - but my idea is that we'd pass an optional region argument to all observed/function methods. You can then use this to return nil if you want to. So in this case, au_nyd_observed_to_monday_if_weekend would return nil for :au_tas, and au_nyd_function_to_monday_if_weekend would return nil for anything except :au_tas. They would probably all call to_monday_if_weekend internally for all other cases.

Most importantly (to me) this wouldn't require upstream code changes, unlike the :exact suggestion

Thoughts?

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

No branches or pull requests

2 participants