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

Add new holiday type "bank_holiday" and "observance" - rename official holiday to public holiday #467

Merged
merged 1 commit into from
Sep 3, 2024

Conversation

derTobsch
Copy link
Contributor

@derTobsch derTobsch commented Feb 21, 2024

closes #462
closes #482
closes #537

Todo:

  • Add conclusions from #537
  • replace known unofficial holiday with a specific (Bank Holidays in Austria e.g.) -> we will do that later.
  • tests and test with Jackson and and JaxB

https://www.officeholidays.com/about/definitions


as described in #537 we will change the holiday types so the following:

Public: Public holiday
Description: Public holidays are days when most of the public enjoys a paid non-working days. These days are determined by local laws and regulations. Countries use various names for these official non-working days, such as:

Other Names for Public Holiday Country Businesses
Bank Holiday United Kingdom Closed
National Holiday Most Countries Closed
Red Days Norway Closed
Regular Holidays Philippines Closed
Restricted Holidays India Closed
Statutory Holiday Canada Closed

We will declare all of these as Public Holiday

Bank: Bank holiday, banks and offices are closed
Description: Bank holidays are days when financial institutions and government offices (and government-regulated businesses) are closed as determined by local laws and regulations. Other businesses, such as offices and retail stores, may also be closed on these days, though are not mandated to by local laws and regulations.

Observance Observance, is a celebration or commemoration that doesn't include a day off from work.
Description: When people celebrate or commemorate something, but do not have a day off from work for that reason, we call it an observance.

There are different types of observance like Religious, Secular, Awareness, International or National observance. We will declare all of these as Observance

Unofficial: Holidays which are not public or bank holidays (Deprecated)
Description: Unofficial holidays are deprecated and will be replaced by the other types over time.

@derTobsch derTobsch changed the base branch from main to 461-holiday-type-filter February 21, 2024 09:50
@derTobsch derTobsch changed the title 462 new holiday types Add new holiday types Bank, School and Authorities Feb 21, 2024
Base automatically changed from 461-holiday-type-filter to main February 23, 2024 14:52
@derTobsch derTobsch changed the title Add new holiday types Bank, School and Authorities Add new holiday type bank Mar 6, 2024
@derTobsch derTobsch changed the title Add new holiday type bank Add new holiday type bank and rename official holiday to public holiday Mar 6, 2024
@derTobsch derTobsch added this to the 0.27.0 milestone Mar 6, 2024
@derTobsch derTobsch force-pushed the 462-new-holiday-types branch 2 times, most recently from 8e79a43 to 42535ab Compare March 6, 2024 20:35
@derTobsch derTobsch marked this pull request as ready for review March 6, 2024 20:37
Copy link

sonarcloud bot commented Mar 6, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
19.2% Coverage on New Code (required ≥ 80%)
8.7% Duplication on New Code (required ≤ 3%)

See analysis details on SonarCloud

@derTobsch derTobsch modified the milestones: 0.27.0, 0.28.0 Mar 8, 2024
@timboven
Copy link

while I do understand the reasoning behind the changes, it results in the loss of some "holidays" (at least based on how the current contents of the PR).
Looking at e.g. https://github.com/focus-shift/jollyday/pull/467/files#diff-61b40247e4124e916e7622c4e350d43ff183b0f12b0235596b28740364f74cacL16 - this holiday is a public holiday for the French part of Belgium ... but not for Belgium as a whole ...

I use this library since the early days of its existence and back then it didn't have more detailed options then "Country", I noticed there are now options to specify a "region" like e.g. NY -- maybe something similar needs to happen for Belgium as well?
This wouldn't solve it for all currently lost holidays in this PR but the others are maybe more a bank holiday?

Thoughts?

@derTobsch derTobsch force-pushed the 462-new-holiday-types branch 2 times, most recently from eaa0992 to 44b2a7c Compare April 10, 2024 11:56
@derTobsch
Copy link
Contributor Author

derTobsch commented Apr 18, 2024

while I do understand the reasoning behind the changes, it results in the loss of some "holidays" (at least based on how the current contents of the PR). Looking at e.g. https://github.com/focus-shift/jollyday/pull/467/files#diff-61b40247e4124e916e7622c4e350d43ff183b0f12b0235596b28740364f74cacL16 - this holiday is a public holiday for the French part of Belgium ... but not for Belgium as a whole ...

I use this library since the early days of its existence and back then it didn't have more detailed options then "Country", I noticed there are now options to specify a "region" like e.g. NY -- maybe something similar needs to happen for Belgium as well? This wouldn't solve it for all currently lost holidays in this PR but the others are maybe more a bank holiday?

Thoughts?

First of all thanks for your feedback!

The loss of the holidays was one of the things why I did not merge this pull requests and thought a lot about this. Are there further holiday types that can be used for the holidays you are talking about? Maybe something like "authority". E.g. for Kings Feast they say

[...] It is not a national public holiday; however, federal government institutions are closed on this day. [...]

with the new API provided to filter the holidays directly by the holiday type, it would be easy to only return the holidays like "Public Holidays" and so on. In other words, if you would add new holiday types, less is better, than we could still have this holidays that are no public holiday. The only thing we need to do is to have a correct holiday type.

Copy link
Contributor

github-actions bot commented Apr 18, 2024

Dependency Review

✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.

OpenSSF Scorecard

PackageVersionScoreDetails

Scanned Manifest Files

Copy link

sonarcloud bot commented Apr 18, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
19.2% Coverage on New Code (required ≥ 80%)
8.7% Duplication on New Code (required ≤ 3%)

See analysis details on SonarCloud

@derTobsch derTobsch modified the milestones: 0.28.0, 0.29.0 May 18, 2024
@kmcbride3
Copy link
Contributor

kmcbride3 commented May 21, 2024

To further differentiate between 'Public' and 'Bank' holiday types, should this be revised to indicate that 'Public' holidays are statutory holidays (enshrined in local law) mandated to be paid holidays, while 'Bank' holidays are when banks and other businesses may be closed?

In the UK, "Bank" holidays are specifically defined as holidays that are defined by statute, while in other countries there are 'optional' or 'de facto' holidays where they may be widely observed by a given country or region as days businesses are closed and/or people are given a paid day off, but are not enshrined in local law as being a paid holiday.

There's a larger discussion required (started in #537) about what other holiday types should be supported (including 'school' or 'authorities' as suggested in this PR), but providing further clarity and differentiation between 'public', 'bank', and 'unofficial' holidays would help establish how these types should be used and avoid confusion or re-work later on.

@derTobsch derTobsch modified the milestones: 0.29.0, 0.30.0 Jun 11, 2024
@derTobsch derTobsch force-pushed the 462-new-holiday-types branch 2 times, most recently from 620edfd to 937dd8c Compare June 17, 2024 18:15
@derTobsch derTobsch force-pushed the 462-new-holiday-types branch 3 times, most recently from 0b750f0 to 933f7bb Compare July 9, 2024 15:25
@derTobsch derTobsch modified the milestones: 0.30.0, 0.31.0 Jul 15, 2024
@derTobsch derTobsch force-pushed the 462-new-holiday-types branch 9 times, most recently from bac43d9 to 5958428 Compare September 3, 2024 18:56
* Rename Official_Holiday to Public_Holiday
* Add Bank_Holiday
* Add Observance

Add new holiday types: bank holiday, observance
Copy link

sonarcloud bot commented Sep 3, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
27.6% Coverage on New Code (required ≥ 80%)
7.6% Duplication on New Code (required ≤ 3%)

See analysis details on SonarCloud

@derTobsch derTobsch merged commit 29938b5 into main Sep 3, 2024
15 of 16 checks passed
@derTobsch derTobsch deleted the 462-new-holiday-types branch September 3, 2024 19:07
@derTobsch derTobsch changed the title Add new holiday type bank and rename official holiday to public holiday Add new holiday type "bank_holiday" and "observance" - rename official holiday to public holiday Sep 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants