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 Brunei Darussalam holidays #1168

Merged
merged 38 commits into from
Jun 7, 2023
Merged

Conversation

PPsyrius
Copy link
Collaborator

@PPsyrius PPsyrius commented May 10, 2023

Proposed change

Add Brunei holidays (en_US, ms, th localization).

Closes #1157. Currently waiting until a decision on #1140 and #1148 is made.

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 upgrade (version update)
  • 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)

arkid15r and others added 3 commits May 3, 2023 19:27
  - Update existing l10n tests to use TestCase::assertLocalizedHolidays
  - Change `check` make target to include `make l10n`
  - Fix .po file generation warning for AR, CO, CR, RS
  - Bulk update stale .po files (location)
  - Remove unused l10n test methods from common::TestCase
@PPsyrius
Copy link
Collaborator Author

I'm tackling these 2 errors so far to no avail:

______________ ERROR at setup of TestBrunei.test_armed_forces_day ______________
[gw0] linux -- Python 3.9.16 /opt/hostedtoolcache/Python/3.9.16/x64/bin/python

cls = <class 'tests.countries.test_brunei.TestBrunei'>

    @classmethod
    def setUpClass(cls):
>       super().setUpClass(
            Brunei,
            years=range(1984, 2077),
            years_non_observed=range(1984, 2077),
        )

tests/countries/test_brunei.py:19: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
tests/common.py:48: in setUpClass
    cls.holidays = test_class(years=years)
holidays/countries/brunei.py:73: in __init__
    super().__init__(*args, **kwargs)
holidays/holiday_base.py:327: in __init__
    self._populate(year)
holidays/countries/brunei.py:109: in _populate
    _add_observed(self._add_isra_and_miraj_day(tr("Israk dan Mikraj")))
holidays/countries/brunei.py:85: in _add_observed
    if self.observed and (self._is_friday(dt) or self._is_sunday(dt)):
holidays/holiday_base.py:842: in _is_friday
    return self._check_weekday(FRI, *args)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

self = {datetime.date(2048, 1, 1): 'Awal Tahun Masihi', datetime.date(2048, 2, 14): 'Tahun Baru Cina', datetime.date(2048, 2, 15): 'Tahun Baru Cina - Diperhatikan', datetime.date(2048, 5, 10): 'Israk dan Mikraj* (*angarran)'}
weekday = 4, args = ({datetime.date(2048, 5, 10)},)
dt = {datetime.date(2048, 5, 10)}

    def _check_weekday(self, weekday: int, *args) -> bool:
        """
        Returns True if `weekday` equals to the date's week day.
        Returns False otherwise.
        """
        dt = args[0] if len(args) == 1 else date(self._year, *args)
>       return dt.weekday() == weekday
E       AttributeError: 'set' object has no attribute 'weekday'

holidays/holiday_base.py:827: AttributeError
___________________ TestEntityLoader.test_countries_imports ____________________
[gw0] linux -- Python 3.9.16 /opt/hostedtoolcache/Python/3.9.16/x64/bin/python

self = <tests.test_registry.TestEntityLoader testMethod=test_countries_imports>

    def test_countries_imports(self):
        warnings.simplefilter("ignore")
    
        import holidays
    
        loader_entities = set()
        for module, entities in registry.COUNTRIES.items():
            module = importlib.import_module(f"holidays.countries.{module}")
            for entity in entities:
>               countries_cls = getattr(countries, entity)
E               AttributeError: module 'holidays.countries' has no attribute 'Brunei'

tests/test_registry.py:31: AttributeError

@arkid15r
Copy link
Collaborator

AttributeError: module 'holidays.countries' has no attribute 'Brunei'

It seems that the country hasn't been added to countries/__init__.py. Could you check this one?

holidays/countries/brunei.py Outdated Show resolved Hide resolved
holidays/countries/brunei.py Outdated Show resolved Hide resolved
holidays/countries/brunei.py Outdated Show resolved Hide resolved
holidays/locale/ms/LC_MESSAGES/BN.po Outdated Show resolved Hide resolved
holidays/countries/brunei.py Outdated Show resolved Hide resolved
holidays/locale/en_US/LC_MESSAGES/BN.po Outdated Show resolved Hide resolved
tests/countries/test_brunei.py Outdated Show resolved Hide resolved
@PPsyrius
Copy link
Collaborator Author

AttributeError: module 'holidays.countries' has no attribute 'Brunei'

It seems that the country hasn't been added to countries/__init__.py. Could you check this one?

Fixed now, thanks for the help :)

@arkid15r arkid15r mentioned this pull request May 23, 2023
13 tasks
@PPsyrius PPsyrius requested a review from KJhellico May 30, 2023 03:54
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.

LGTM ✌️

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.

This would be a great addition to the list of supported countries!
Thanks for all your work @PPsyrius!

Please let me know if you have any questions regarding the comments I've left:

holidays/countries/brunei.py Outdated Show resolved Hide resolved
holidays/countries/brunei.py Outdated Show resolved Hide resolved
holidays/countries/brunei.py Outdated Show resolved Hide resolved
holidays/countries/brunei.py Outdated Show resolved Hide resolved
holidays/countries/brunei.py Outdated Show resolved Hide resolved
holidays/locale/en_US/LC_MESSAGES/BN.po Outdated Show resolved Hide resolved
holidays/locale/en_US/LC_MESSAGES/BN.po Outdated Show resolved Hide resolved
tests/countries/test_brunei.py Outdated Show resolved Hide resolved
@arkid15r arkid15r mentioned this pull request May 31, 2023
@PPsyrius
Copy link
Collaborator Author

If I understand the problem correctly you're looking for something like:

self._add_islamic_calendar_holiday(
    self.tr("%s - Diperhatikan") % self[hol_date],
    (
        (
            obs_date,
            self._year
            not in BruneiIslamicCalendar.EID_AL_FITR_DATES,
        ),
    ),
)

@arkid15r Can you explain more about this? This is supposed to fully replace all instances of _add_observed(dt) in

for dt in self._add_eid_al_adha_day(tr("Hari Raya Aidil Adha")):
    _add_observed(dt)

and placed at the back once all Islamic holidays are already declared, right?

@arkid15r
Copy link
Collaborator

@arkid15r Can you explain more about this?

Sure -- the code example is a reply to the @KJhellico's comment you had during the review. It supposed to address the mentioned issue w/ Eid al-Fitr holiday (that might not be the problem any longer in your current code if observed/estimated holidays get labeled correctly):

This way, Eid al-Fitr (Observed) will never be labeled as "estimated", regardless of the origin of the date (exact or not). But unfortunately, I don't know of a good way to avoid it now. 🤷‍♂️

@PPsyrius PPsyrius requested a review from arkid15r June 5, 2023 03:33
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.

A couple more comments to address and we're good to go:

holidays/countries/brunei.py Outdated Show resolved Hide resolved
holidays/countries/brunei.py Outdated Show resolved Hide resolved
holidays/locale/ms/LC_MESSAGES/BN.po Outdated Show resolved Hide resolved
holidays/locale/th/LC_MESSAGES/BN.po Outdated Show resolved Hide resolved
@PPsyrius PPsyrius requested a review from arkid15r June 6, 2023 03:36
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.

Great work 👍

Marking as ready for beta

@arkid15r arkid15r added the ready for beta Ready to merge on beta branch label Jun 6, 2023
@PPsyrius PPsyrius linked an issue Jun 7, 2023 that may be closed by this pull request
@KJhellico KJhellico merged commit 3d1126d into vacanza:beta Jun 7, 2023
@KJhellico KJhellico removed the ready for beta Ready to merge on beta branch label Jun 7, 2023
@KJhellico
Copy link
Collaborator

Thank you for the contribution!

@PPsyrius PPsyrius deleted the brunei-holidays branch June 7, 2023 18:09
@arkid15r arkid15r mentioned this pull request Jun 19, 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 Brunei Darussalam holidays
4 participants