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 Barbados holidays #1393

Merged
merged 13 commits into from
Aug 15, 2023

Conversation

arjunanan6
Copy link
Contributor

@arjunanan6 arjunanan6 commented Jul 24, 2023

Proposed change

Adding Barbados holidays.

closes: #1153

Type of change

  • New country/market holidays support (thank you!)
  • Supported country/market holidays update (calendar discrepancy fix, localization)
  • Existing code/documentation/test/process quality improvement (best practice, cleanup, refactoring, optimization)
  • Dependency update (version deprecation/upgrade)
  • Bugfix (non-breaking change which fixes an issue)
  • Breaking change (a code change causing existing functionality to break)
  • New feature (new python-holidays functionality in general)

Checklist

  • I've followed the contributing guidelines
  • This PR is filed against beta branch of the repository
  • This PR doesn't contain any merge conflicts and has clean commit history
  • The code style looks good: make pre-commit
  • All tests pass locally: make test, make tox (we strongly encourage adding tests to your code)
  • The related documentation has been added/updated (check off the box for free if no updates is required)

@KJhellico KJhellico changed the title Arjunanan6/add barbados holidays Add Barbados holidays Jul 24, 2023
@KJhellico KJhellico changed the base branch from master to beta July 24, 2023 18:06
Copy link
Collaborator

@arkid15r arkid15r left a comment

Choose a reason for hiding this comment

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

Thanks for contributing this! Your persistent work is much appreciated!

Please run make check after addressing the l10n issue comment below.

I guess that will be it to get the PR to a regular review process.

"""

country = "BB"
default_language = "en"
Copy link
Collaborator

Choose a reason for hiding this comment

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

All right, so this is the reason why make l10n wasn't working. You set the default_language but didn't do any l10n work. As a quick fix you need to remove the default_language line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Aha, I see. I'll look into how to do that for next time (If needed). So, I removed this line and have pushed the changes. However make check fails:

make doc
sphinx-build -E -T -W -b html -D language=en -j auto -q docs/source docs/build
Traceback (most recent call last):
  File "/Users/arjun/Documents/git/python-holidays/holidayenv/bin/sphinx-build", line 8, in <module>
    sys.exit(main())
             ^^^^^^
  File "/Users/arjun/Documents/git/python-holidays/holidayenv/lib/python3.11/site-packages/sphinx/cmd/build.py", line 313, in main
    locale.setlocale(locale.LC_ALL, '')
  File "/Library/Frameworks/Python.framework/Versions/3.11/lib/python3.11/locale.py", line 626, in setlocale
    return _setlocale(category, locale)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
locale.Error: unsupported locale setting
make[1]: *** [doc] Error 1
make: *** [check] Error 2

Copy link
Collaborator

Choose a reason for hiding this comment

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

This might be a Windows specific issue. @KJhellico any suggestions here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm actually on a mac @arkid15r.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@coveralls
Copy link

coveralls commented Jul 24, 2023

Pull Request Test Coverage Report for Build 5648588296

Details

  • 31 of 31 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 99.975%

Totals Coverage Status
Change from base Build 5616707875: 0.0%
Covered Lines: 8994
Relevant Lines: 8995

💛 - Coveralls

Copy link
Collaborator

@KJhellico KJhellico left a comment

Choose a reason for hiding this comment

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

@arjunanan6 , thank you for your efforts! Please look at suggestions below.

holidays/countries/barbados.py Show resolved Hide resolved
holidays/countries/barbados.py Outdated Show resolved Hide resolved
holidays/countries/barbados.py Outdated Show resolved Hide resolved
holidays/countries/barbados.py Outdated Show resolved Hide resolved
holidays/countries/barbados.py Outdated Show resolved Hide resolved
holidays/countries/barbados.py Outdated Show resolved Hide resolved
holidays/countries/barbados.py Outdated Show resolved Hide resolved
arjunanan6 and others added 8 commits August 12, 2023 11:38
Co-authored-by: ~Jhellico <KJhellico@users.noreply.github.com>
Co-authored-by: ~Jhellico <KJhellico@users.noreply.github.com>
Co-authored-by: ~Jhellico <KJhellico@users.noreply.github.com>
Co-authored-by: ~Jhellico <KJhellico@users.noreply.github.com>
Co-authored-by: ~Jhellico <KJhellico@users.noreply.github.com>
Co-authored-by: ~Jhellico <KJhellico@users.noreply.github.com>
@arjunanan6
Copy link
Contributor Author

@KJhellico Could you please check again? Thank you :)

Copy link
Collaborator

@arkid15r arkid15r left a comment

Choose a reason for hiding this comment

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

Hey @arjunanan6 it's great to have you back! I hope your moving process wasn't too exhaustive!

Yeah, some changes have been introduced recently so the PR requires a bit more work (nothing really serious, just modifying 5 more lines of code).

self._add_new_years_day("New Year's Day")

# Errol Barrow Day
self._add_holiday("Errol Barrow Day", JAN, 21)
Copy link
Collaborator

Choose a reason for hiding this comment

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

self._add_holiday_1st_mon_of_aug("Kadooment Day")

The same way you can now use

Suggested change
self._add_holiday("Errol Barrow Day", JAN, 21)
self._add_holiday_jan_21("Errol Barrow Day")

type of methods for month/day holidays and get rid of the

from holidays.calendars.gregorian import JAN, APR, AUG, NOV in the import line above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey @arkid15r, thank you! It went pretty okay, but you know, the actual work begins after the move, but we're almost there! :)

Just made the suggested changes and pushing it now after testing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And there we go @arkid15r, tests passed :)

Copy link
Collaborator

@arkid15r arkid15r left a comment

Choose a reason for hiding this comment

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

🇧🇧 LGTM

@arkid15r arkid15r merged commit 6986d19 into vacanza:beta Aug 15, 2023
22 checks passed
@arjunanan6
Copy link
Contributor Author

🇧🇧 LGTM

Super, I hope to contribute more going forward! :)

@arjunanan6 arjunanan6 deleted the arjunanan6/add-barbados-holidays branch August 15, 2023 16:12
@arkid15r
Copy link
Collaborator

All right, it's in beta now!
Thank you for such a valuable contribution 👍

This was referenced Aug 15, 2023
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.

Add Barbados holidays
4 participants