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

IsPublicHoliday(DateTime date, CountryCode countryCode) result inconsistent with GetPublicHoliday(DateTime startDate, DateTime endDate, CountryCode countryCode) #221

Closed
pmpontes opened this issue Nov 13, 2020 · 6 comments
Assignees
Labels

Comments

@pmpontes
Copy link

Important specifications

  • Which country is affected? DE

DateSystem.IsPublicHoliday(date: new DateTime(2020, 11, 18), CountryCode.DE) returns false

However:

DateSystem.GetPublicHoliday(startDate: new DateTime(2020, 11, 18), endDate: new DateTime(2020, 11, 19), CountryCode.DE)
returns a list which includes the DateTime(2020, 11, 18).

Description

The two aforementioned methods return results which are inconsistent.
From a quick Google search, it seems this is indeed a public holiday, so the second result seems to be correct, rather than the first.

@tinohager tinohager self-assigned this Nov 13, 2020
@tinohager
Copy link
Member

It is only a holiday in a county not in the full country.

return new PublicHoliday(dayOfPrayer, localName, englishName, countryCode, null, new string[] { "DE-SN" });

@pmpontes
Copy link
Author

pmpontes commented Nov 13, 2020

I realized that after some more research.
Still, it seems somewhat misleading, considering (as far as I could find) there's no indication that the method only looks at national level holidays while the other returns holidays regardless of specificity, other than diving into the implementation.
Appreciate the quick reply, though.

@tinohager
Copy link
Member

In the other method you must check this field

public string[] Counties { get; set; }

@tinohager
Copy link
Member

What would your implementation for the problem look like?

@pmpontes
Copy link
Author

After realizing this, we added a check on PublicHoliday.Global when using GetPublicHoliday(startDate,endDate,countryCode).
I do understand the reason for the distinction, and don't think this is so much an implementation problem, more of a naming issue... What qualifies as a public holiday kind of changes from one to the other... Maybe the first should be named IsNationalPublicHoliday(...), which would be more suggestive of what it does, or more simply, update the description to note that only national holidays are considered.
Not that this is a major issue, it's a bit of nitpicking.

@uniquelau
Copy link

Thank-you both, this thread helped me resolve an issue in here in the UK.

As Easter Monday, is a bank holiday in England, but not Scotland.

I adjusted our API calls to include the countyCode.

if (DateSystem.IsPublicHoliday(value.UtcDateTime, _countryCode, "GB-ENG"))

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants