Skip to content

Conversation

@PPsyrius
Copy link
Collaborator

Proposed change

As raised by coderabbitai during #2928's code review:

  • Benin: HolidayBase, ChristianHolidays, InternationalHolidays, IslamicHolidays ✅ (alphabetical)
  • Burkina Faso: ObservedHolidayBase, ChristianHolidays, InternationalHolidays, IslamicHolidays ✅ (alphabetical)
  • Burundi: ObservedHolidayBase, ChristianHolidays, InternationalHolidays, IslamicHolidays ✅ (alphabetical)
  • Eritrea: HolidayBase, ChristianHolidays, InternationalHolidays, IslamicHolidays ✅ (alphabetical)
  • Djibouti: HolidayBase, ChristianHolidays, IslamicHolidays, InternationalHolidays ❌ (not alphabetical - appears to be an outlier)

Your Algeria implementation with HolidayBase, ChristianHolidays, HebrewCalendarHolidays, InternationalHolidays, IslamicHolidays perfectly follows the alphabetical convention:

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/pin/upgrade)
  • Bugfix (non-breaking change which fixes an issue)
  • Breaking change (a code change causing existing functionality to break)
  • New feature (new holidays functionality in general)

Checklist

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Sep 16, 2025

Summary by CodeRabbit

  • Bug Fixes
    • Adjusted Djibouti holiday calculation to ensure correct precedence between international and Islamic observances, improving accuracy on overlapping dates.
    • Enhances consistency of holiday results across years without changing how users interact with the feature.
    • No configuration or API changes required for users.

Walkthrough

Reorders base classes in Djibouti holiday class to place InternationalHolidays before IslamicHolidays, changing the method resolution order. No other edits.

Changes

Cohort / File(s) Summary
Country holiday class MRO update
holidays/countries/djibouti.py
Changed class signature from class Djibouti(HolidayBase, ChristianHolidays, IslamicHolidays, InternationalHolidays) to class Djibouti(HolidayBase, ChristianHolidays, InternationalHolidays, IslamicHolidays) to adjust MRO.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Suggested reviewers

  • arkid15r
✨ Finishing touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 7347cd0 and 6b4fbea.

📒 Files selected for processing (1)
  • holidays/countries/djibouti.py (1 hunks)
🧰 Additional context used
🧠 Learnings (10)
📓 Common learnings
Learnt from: KJhellico
PR: vacanza/holidays#2593
File: holidays/countries/senegal.py:66-110
Timestamp: 2025-06-06T14:40:31.932Z
Learning: In the holidays library, within the _populate_public_holidays method, holidays should be arranged by calendar type (Islamic holidays first, then Gregorian holidays) without additional type grouping comments. The organization by calendar type is sufficient and follows the project's established conventions.
Learnt from: KJhellico
PR: vacanza/holidays#2774
File: tests/countries/test_liberia.py:15-16
Timestamp: 2025-08-08T14:37:03.045Z
Learning: In holidays/countries/__init__.py, re-export country classes using absolute imports (e.g., 'from holidays.countries.liberia import Liberia, LR, LBR') and keep alphabetical ordering (e.g., Lesotho, Liberia, Libya). Avoid relative imports in this file.
Learnt from: PPsyrius
PR: vacanza/holidays#2928
File: holidays/countries/algeria.py:16-22
Timestamp: 2025-09-16T04:11:33.485Z
Learning: In the holidays library, country class base inheritance follows alphabetical ordering after HolidayBase. For example, Algeria correctly uses: HolidayBase, ChristianHolidays, HebrewCalendarHolidays, InternationalHolidays, IslamicHolidays (alphabetical after the required HolidayBase first position).
Learnt from: PPsyrius
PR: vacanza/holidays#2928
File: holidays/countries/algeria.py:16-22
Timestamp: 2025-09-16T04:11:33.485Z
Learning: In the holidays library, country class base inheritance follows alphabetical ordering after HolidayBase. For example, Algeria correctly uses: HolidayBase, ChristianHolidays, HebrewCalendarHolidays, InternationalHolidays, IslamicHolidays (alphabetical after the required HolidayBase first position).
Learnt from: PPsyrius
PR: vacanza/holidays#2632
File: holidays/countries/solomon_islands.py:95-98
Timestamp: 2025-06-16T12:28:31.641Z
Learning: Library-wide holiday patterns and their optimizations should be handled at the base class level (like InternationalHolidays) rather than documenting workarounds in individual country modules. This maintains separation of concerns and avoids documentation duplication.
Learnt from: KJhellico
PR: vacanza/holidays#2671
File: holidays/countries/libya.py:51-108
Timestamp: 2025-06-29T10:36:06.138Z
Learning: There is no project-wide convention in the holidays library to organize holidays by calendar type (Islamic holidays first, then Gregorian holidays). Countries organize holidays in various ways - often chronologically, by importance, or by logical grouping - and Islamic holidays are frequently placed at the end of the _populate_public_holidays method rather than at the beginning.
Learnt from: KJhellico
PR: vacanza/holidays#2608
File: holidays/countries/saint_vincent_and_the_grenadines.py:39-44
Timestamp: 2025-06-07T11:19:38.945Z
Learning: In the holidays library, it's standard practice to explicitly call ChristianHolidays.__init__(self) and InternationalHolidays.__init__(self) in country class __init__ methods, even when these mixins don't define their own __init__ methods. This follows the established codebase convention across all country implementations (confirmed in Zimbabwe, Zambia, and other countries).
Learnt from: KJhellico
PR: vacanza/holidays#2860
File: tests/common.py:365-372
Timestamp: 2025-08-26T21:29:47.753Z
Learning: In the holidays library, countries with Islamic holidays inherit directly from the IslamicHolidays class (e.g., `class Afghanistan(HolidayBase, InternationalHolidays, IslamicHolidays)`). The separate `_CustomIslamicHolidays` classes (like `AfghanistanIslamicHolidays`) are helper classes for specific date data, not the main country classes. Therefore, `isinstance(holidays_instance, IslamicHolidays)` is sufficient to detect all countries with Islamic holidays.
Learnt from: PPsyrius
PR: vacanza/holidays#2881
File: tests/countries/test_djibouti.py:27-29
Timestamp: 2025-09-14T16:10:02.721Z
Learning: For Djibouti in the holidays library, only the default PUBLIC category is used, so there's no need to specify `categories=Djibouti.supported_categories` in test_no_holidays methods, as it would be equivalent to the default behavior and Djibouti doesn't define supported_categories.
Learnt from: PPsyrius
PR: vacanza/holidays#2881
File: tests/countries/test_djibouti.py:27-29
Timestamp: 2025-09-14T16:10:02.721Z
Learning: For Djibouti in the holidays library, only the default PUBLIC category is used, so there's no need to specify `categories=Djibouti.supported_categories` in test_no_holidays methods, as it would be equivalent to the default behavior and Djibouti doesn't define supported_categories.
📚 Learning: 2025-09-16T04:11:33.485Z
Learnt from: PPsyrius
PR: vacanza/holidays#2928
File: holidays/countries/algeria.py:16-22
Timestamp: 2025-09-16T04:11:33.485Z
Learning: In the holidays library, country class base inheritance follows alphabetical ordering after HolidayBase. For example, Algeria correctly uses: HolidayBase, ChristianHolidays, HebrewCalendarHolidays, InternationalHolidays, IslamicHolidays (alphabetical after the required HolidayBase first position).

Applied to files:

  • holidays/countries/djibouti.py
📚 Learning: 2025-06-13T12:18:03.539Z
Learnt from: PPsyrius
PR: vacanza/holidays#2614
File: holidays/countries/guyana.py:78-90
Timestamp: 2025-06-13T12:18:03.539Z
Learning: The holidays codebase now uses the constructor signature pattern `__init__(self, *args, islamic_show_estimated: bool = True, **kwargs)` across country classes.

Applied to files:

  • holidays/countries/djibouti.py
📚 Learning: 2025-08-08T14:37:03.045Z
Learnt from: KJhellico
PR: vacanza/holidays#2774
File: tests/countries/test_liberia.py:15-16
Timestamp: 2025-08-08T14:37:03.045Z
Learning: In holidays/countries/__init__.py, re-export country classes using absolute imports (e.g., 'from holidays.countries.liberia import Liberia, LR, LBR') and keep alphabetical ordering (e.g., Lesotho, Liberia, Libya). Avoid relative imports in this file.

Applied to files:

  • holidays/countries/djibouti.py
📚 Learning: 2025-09-14T16:10:02.721Z
Learnt from: PPsyrius
PR: vacanza/holidays#2881
File: tests/countries/test_djibouti.py:27-29
Timestamp: 2025-09-14T16:10:02.721Z
Learning: For Djibouti in the holidays library, only the default PUBLIC category is used, so there's no need to specify `categories=Djibouti.supported_categories` in test_no_holidays methods, as it would be equivalent to the default behavior and Djibouti doesn't define supported_categories.

Applied to files:

  • holidays/countries/djibouti.py
📚 Learning: 2025-06-07T11:19:38.945Z
Learnt from: KJhellico
PR: vacanza/holidays#2608
File: holidays/countries/saint_vincent_and_the_grenadines.py:39-44
Timestamp: 2025-06-07T11:19:38.945Z
Learning: In the holidays library, it's standard practice to explicitly call ChristianHolidays.__init__(self) and InternationalHolidays.__init__(self) in country class __init__ methods, even when these mixins don't define their own __init__ methods. This follows the established codebase convention across all country implementations (confirmed in Zimbabwe, Zambia, and other countries).

Applied to files:

  • holidays/countries/djibouti.py
📚 Learning: 2025-08-26T21:29:47.753Z
Learnt from: KJhellico
PR: vacanza/holidays#2860
File: tests/common.py:365-372
Timestamp: 2025-08-26T21:29:47.753Z
Learning: In the holidays library, countries with Islamic holidays inherit directly from the IslamicHolidays class (e.g., `class Afghanistan(HolidayBase, InternationalHolidays, IslamicHolidays)`). The separate `_CustomIslamicHolidays` classes (like `AfghanistanIslamicHolidays`) are helper classes for specific date data, not the main country classes. Therefore, `isinstance(holidays_instance, IslamicHolidays)` is sufficient to detect all countries with Islamic holidays.

Applied to files:

  • holidays/countries/djibouti.py
📚 Learning: 2025-06-07T11:19:38.945Z
Learnt from: KJhellico
PR: vacanza/holidays#2608
File: holidays/countries/saint_vincent_and_the_grenadines.py:39-44
Timestamp: 2025-06-07T11:19:38.945Z
Learning: In the holidays library, it's standard practice to explicitly call ChristianHolidays.__init__(self) and InternationalHolidays.__init__(self) in country class __init__ methods, even when these mixins don't define their own __init__ methods. This follows the established codebase convention across all country implementations.

Applied to files:

  • holidays/countries/djibouti.py
📚 Learning: 2025-06-19T02:34:18.382Z
Learnt from: PPsyrius
PR: vacanza/holidays#2643
File: holidays/countries/mauritius.py:144-169
Timestamp: 2025-06-19T02:34:18.382Z
Learning: Custom holiday classes that extend _CustomHinduHolidays, _CustomIslamicHolidays, _CustomBuddhistHolidays, etc. in the holidays library do not use docstrings. They follow a pattern of using only inline comments above date dictionaries, as seen in Malaysia, Singapore, UAE, and other country implementations.

Applied to files:

  • holidays/countries/djibouti.py
📚 Learning: 2025-06-19T02:34:14.456Z
Learnt from: PPsyrius
PR: vacanza/holidays#2643
File: holidays/countries/mauritius.py:171-184
Timestamp: 2025-06-19T02:34:14.456Z
Learning: In the holidays library, `_CustomIslamicHolidays` subclasses follow a consistent pattern of NOT having docstrings. They go directly to defining date dictionaries, as evidenced by Malaysia, Singapore, UAE, and dozens of other country implementations.

Applied to files:

  • holidays/countries/djibouti.py
🧬 Code graph analysis (1)
holidays/countries/djibouti.py (3)
holidays/groups/christian.py (1)
  • ChristianHolidays (23-499)
holidays/groups/international.py (1)
  • InternationalHolidays (20-236)
holidays/groups/islamic.py (1)
  • IslamicHolidays (20-453)
⏰ Context from checks skipped due to timeout of 300000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
  • GitHub Check: Test docs build
  • GitHub Check: Build distribution
🔇 Additional comments (1)
holidays/countries/djibouti.py (1)

20-20: Approve — base-class MRO check passed

No overlapping add* helper method names found between holidays/groups/international.py and holidays/groups/islamic.py.

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 Ruff (0.12.2)
holidays/countries/djibouti.py

�[1;31mruff failed�[0m
�[1mCause:�[0m Failed to load configuration /ruff.toml
�[1mCause:�[0m Failed to parse /ruff.toml
�[1mCause:�[0m TOML parse error at line 26, column 3
|
26 | "RSE100", # Use of assert detected
| ^^^^^^^^
Unknown rule selector: RSE100

Tip

👮 Agentic pre-merge checks are now available in preview!

Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.

  • Built-in checks – Quickly apply ready-made checks to enforce title conventions, require pull request descriptions that follow templates, validate linked issues for compliance, and more.
  • Custom agentic checks – Define your own rules using CodeRabbit’s advanced agentic capabilities to enforce organization-specific policies and workflows. For example, you can instruct CodeRabbit’s agent to verify that API documentation is updated whenever API schema files are modified in a PR. Note: Upto 5 custom checks are currently allowed during the preview period. Pricing for this feature will be announced in a few weeks.

Please see the documentation for more information.

Example:

reviews:
  pre_merge_checks:
    custom_checks:
      - name: "Undocumented Breaking Changes"
        mode: "warning"
        instructions: |
          Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).

Please share your feedback with us on this Discord post.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Pre-merge checks

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed The PR description directly describes the change and its motivation (aligning mixin ordering to the project's alphabetical convention), cites the related review comment, and notes that local checks passed, which ties it clearly to the changeset. The content is relevant and not off-topic.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.
Title Check ✅ Passed The title succinctly identifies the primary change—reordering Djibouti's holiday group mixins which affects the class MRO—and names the affected module, so it accurately and concisely reflects the main intent of the changeset.

@sonarqubecloud
Copy link

@codecov
Copy link

codecov bot commented Sep 16, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (7347cd0) to head (6b4fbea).
⚠️ Report is 2 commits behind head on dev.

Additional details and impacted files
@@            Coverage Diff            @@
##               dev     #2931   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          296       296           
  Lines        17593     17593           
  Branches      2270      2270           
=========================================
  Hits         17593     17593           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

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!

@arkid15r arkid15r changed the title Refactor Djibouti holidays: HolidayGroup import sort Update Djibouti holidays: change holiday groups MRO Sep 16, 2025
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 added this pull request to the merge queue Sep 16, 2025
Merged via the queue into vacanza:dev with commit 1566212 Sep 16, 2025
36 checks passed
@PPsyrius PPsyrius deleted the djibouti_import_sort branch September 18, 2025 02:29
@arkid15r arkid15r mentioned this pull request Oct 6, 2025
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