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

Do not expand into other years v2 #606

Merged
merged 5 commits into from
Jan 29, 2022
Merged

Do not expand into other years v2 #606

merged 5 commits into from
Jan 29, 2022

Conversation

mborsetti
Copy link
Contributor

While working on an another PR I discovered that my PR #586 (Do not expand into other years) was incomplete: I missed that holidays for Saudi Arabia for 2006 would also return holidays for 2007, violating the "do not expand into other years" rule and triggering the "dict expansion while iterating" error in people's code.

The issue, it turns out, is multi-day Islamic holidays; in this specific case it was the Eid al-Adha Holiday running from 30-Dec-2006 to 03-Jan-2007, across Gregorian years.

Here is a fix for this issue, covering Saudi Arabia and all other countries affected. It's not very elegant but I couldn't think of a better solution (open to ideas). The fix has been successfully tested on all years from 1950 to 2050 inclusive.

I also addressed:

  • The Saudi Arabia tests had some assertions remarked out, and these were added back in.
  • The Saudi Arabia tests were strengthened by adding assertions for 2022 holidays.
  • The code for Saudi Arabia had the remark "Weekend used to be THU, FRI before June 28th, 2013", which was not implemented in code; I implemented it
  • The test_all_countries test I wrote for PR Do not expand into other years #586 (Do not expand into other years) does not need to be run in all Python versions since it's a logic test, not a code compatibility one; I fixed this so we don't have needlessly long test times (they are already getting a bit too lengthy IMHO).

@coveralls
Copy link

coveralls commented Jan 22, 2022

Pull Request Test Coverage Report for Build 1732618795

  • 145 of 149 (97.32%) changed or added relevant lines in 10 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.3%) to 98.817%

Changes Missing Coverage Covered Lines Changed/Added Lines %
holidays/countries/morocco.py 19 21 90.48%
holidays/countries/nigeria.py 14 16 87.5%
Totals Coverage Status
Change from base Build 1724629910: 0.3%
Covered Lines: 5951
Relevant Lines: 5967

💛 - Coveralls

@mborsetti
Copy link
Contributor Author

@dr-prodigy Maurizio -- #609 will have to be pulled in here. Let me know if you need more info to merge this one.

@dr-prodigy dr-prodigy merged commit be1307f into vacanza:beta Jan 29, 2022
@dr-prodigy
Copy link
Collaborator

dr-prodigy commented Jan 29, 2022

Merged now! Going to look at #609 .. in case of doubts I'll get back to you.
Thank you as usual Mike! 👍

@mborsetti
Copy link
Contributor Author

BTW this PR also fixes #569 which should probably be closed at the next release.

@mborsetti mborsetti deleted the all_in_same_year_2 branch February 16, 2022 04:15
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.

3 participants