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 error when 0 (Sunday) is used as WeekDayNumber in HolidaysDefinition.json #45

Merged
merged 2 commits into from
Apr 10, 2024

Conversation

albertospelta
Copy link
Collaborator

@albertospelta albertospelta commented Mar 26, 2024

Fixes a Wrong configuration in HolidaysDefinition error when 0 (Sunday) is used as WeekDayNumber in HolidaysDefinition.json.
This error has been reported in sql-bi/Bravo#766

This error happens, for example, for Mother's Day holiday celebrated in the United States on the second Sunday in May.

{
  "IsoCountry": "US",
  "MonthNumber": 5,
  "DayNumber": 0,
  "WeekDayNumber": 0,
  "OffsetWeek": 2,
  "OffsetDays": 0,
  "HolidayName": "Mother's Day",
  "SubstituteHoliday": "NoSubstituteHoliday",
  "ConflictPriority": 100
},

@albertospelta albertospelta added the bug Something isn't working label Mar 26, 2024
@albertospelta albertospelta marked this pull request as ready for review March 26, 2024 13:56
Copy link
Collaborator

@marcosqlbi marcosqlbi left a comment

Choose a reason for hiding this comment

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

Originally, 0 was supposed to be the "not defined" state. However, because Sunady is defined as 0 (and we cannot change it now!) we rely on the priority of the conditions. For example, if MonthNumber is 92 (which is not supported), we end up using Sunday as the definition of the weekday.
I would add these conditions to proceed with the code of this case:

  • MonthNumber must be in the range 1..12
  • OffsetWeek must be <> 0
  • DayNumber == 0

@albertospelta albertospelta merged commit 8f32168 into main Apr 10, 2024
1 check passed
@albertospelta albertospelta deleted the alberto/fix-weekdaynumber-zero branch April 10, 2024 07:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants