-
-
Notifications
You must be signed in to change notification settings - Fork 541
Add Mongolia holidays #2601
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 Mongolia holidays #2601
Conversation
Added Mongolia holidays with localisation
Summary by CodeRabbit
Summary by CodeRabbit
WalkthroughSupport for Mongolia as a new country has been added, including a holiday provider, lunisolar calendar calculations, localization files, and comprehensive tests. Documentation and registry updates reflect the new country. Generator scripts and calendar logic for Mongolian holidays are introduced, and integration points are established across the codebase. Changes
Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes detected. Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
🧰 Additional context used🧠 Learnings (1)tests/countries/test_mongolia.py (5)
🪛 Pylint (3.3.7)scripts/calendar/mongolian_generator.py[convention] 1-1: Missing module docstring (C0114) [warning] 18-25: String statement has no effect (W0105) [convention] 27-27: Constant name "m_zero" doesn't conform to UPPER_CASE naming style (C0103) [convention] 28-28: Constant name "epoch" doesn't conform to UPPER_CASE naming style (C0103) [convention] 29-29: Constant name "ixx" doesn't conform to UPPER_CASE naming style (C0103) [convention] 30-30: Constant name "betastar" doesn't conform to UPPER_CASE naming style (C0103) [convention] 31-31: Constant name "beta" doesn't conform to UPPER_CASE naming style (C0103) [convention] 33-33: Constant name "m0" doesn't conform to UPPER_CASE naming style (C0103) [convention] 34-34: Constant name "m1" doesn't conform to UPPER_CASE naming style (C0103) [convention] 35-35: Constant name "m2" doesn't conform to UPPER_CASE naming style (C0103) [convention] 36-36: Constant name "s0" doesn't conform to UPPER_CASE naming style (C0103) [convention] 37-37: Constant name "s1" doesn't conform to UPPER_CASE naming style (C0103) [convention] 38-38: Constant name "s2" doesn't conform to UPPER_CASE naming style (C0103) [convention] 39-39: Constant name "a0" doesn't conform to UPPER_CASE naming style (C0103) [convention] 40-40: Constant name "a1" doesn't conform to UPPER_CASE naming style (C0103) [convention] 41-41: Constant name "a2" doesn't conform to UPPER_CASE naming style (C0103) [convention] 44-44: Missing function or method docstring (C0116) [convention] 48-48: Missing function or method docstring (C0116) [convention] 53-53: Missing function or method docstring (C0116) [convention] 58-58: Missing function or method docstring (C0116) [convention] 64-64: Missing function or method docstring (C0116) [convention] 78-78: Missing function or method docstring (C0116) [convention] 92-92: Missing function or method docstring (C0116) [convention] 102-102: Missing function or method docstring (C0116) [convention] 108-108: Missing function or method docstring (C0116) [convention] 119-119: Missing function or method docstring (C0116) [convention] 123-123: Missing function or method docstring (C0116) [convention] 127-127: Missing function or method docstring (C0116) [convention] 155-155: Missing function or method docstring (C0116) tests/countries/test_mongolia.py[convention] 1-1: Missing module docstring (C0114) [convention] 20-20: Missing class docstring (C0115) [warning] 22-22: Number of parameters was 4 in 'TestCase.setUpClass' and is now 1 in overriding 'TestMongolia.setUpClass' method (W0221) [convention] 27-27: Missing function or method docstring (C0116) [convention] 30-30: Missing function or method docstring (C0116) [convention] 33-33: Missing function or method docstring (C0116) [convention] 36-36: Missing function or method docstring (C0116) [convention] 43-43: Missing function or method docstring (C0116) [convention] 82-82: Missing function or method docstring (C0116) [convention] 90-90: Missing function or method docstring (C0116) [convention] 95-95: Missing function or method docstring (C0116) [convention] 109-109: Missing function or method docstring (C0116) [convention] 116-116: Missing function or method docstring (C0116) [convention] 123-123: Missing function or method docstring (C0116) [convention] 130-130: Missing function or method docstring (C0116) [convention] 133-133: Missing function or method docstring (C0116) [convention] 146-146: Missing function or method docstring (C0116) [convention] 154-154: Missing function or method docstring (C0116) [convention] 171-171: Missing function or method docstring (C0116) [convention] 178-178: Missing function or method docstring (C0116) [convention] 187-187: Missing function or method docstring (C0116) [convention] 194-194: Missing function or method docstring (C0116) [convention] 201-201: Missing function or method docstring (C0116) [convention] 215-215: Missing function or method docstring (C0116) [convention] 228-228: Missing function or method docstring (C0116) [convention] 246-246: Missing function or method docstring (C0116) [convention] 253-253: Missing function or method docstring (C0116) [convention] 271-271: Missing function or method docstring (C0116) [convention] 292-292: Missing function or method docstring (C0116) [convention] 371-371: Missing function or method docstring (C0116) [refactor] 20-20: Too many public methods (28/20) (R0904) 🔇 Additional comments (4)
✨ Finishing Touches
🧪 Generate Unit Tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (14)
README.md
(2 hunks)holidays/calendars/__init__.py
(1 hunks)holidays/calendars/mongolian.py
(1 hunks)holidays/countries/__init__.py
(1 hunks)holidays/countries/mongolia.py
(1 hunks)holidays/groups/__init__.py
(1 hunks)holidays/groups/eastern.py
(1 hunks)holidays/groups/mongolian.py
(1 hunks)holidays/locale/en_MN/LC_MESSAGES/MN.po
(1 hunks)holidays/locale/en_US/LC_MESSAGES/MN.po
(1 hunks)holidays/locale/mn/LC_MESSAGES/MN.po
(1 hunks)holidays/registry.py
(1 hunks)scripts/calendar/asian_generator.py
(4 hunks)tests/countries/test_mongolia.py
(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
tests/countries/test_mongolia.py (1)
Learnt from: PPsyrius
PR: vacanza/holidays#2416
File: tests/countries/test_turkmenistan.py:31-49
Timestamp: 2025-04-05T04:50:40.752Z
Learning: For Turkmenistan holiday tests, use this class structure: `class TestTurkmenistan(CommonCountryTests, TestCase)` with imports `from unittest import TestCase`, `from holidays.countries import Turkmenistan, TM, TKM`, and `from tests.common import CommonCountryTests`. Ensure to call `super().setUp()` in the setUp method.
🧬 Code Graph Analysis (4)
holidays/countries/__init__.py (1)
holidays/countries/mongolia.py (3)
Mongolia
(19-67)MN
(70-71)MNG
(74-75)
holidays/groups/__init__.py (1)
holidays/groups/mongolian.py (1)
MongolianCalendarHolidays
(21-96)
tests/countries/test_mongolia.py (2)
tests/common.py (7)
TestCase
(28-338)CommonCountryTests
(356-374)assertAliases
(121-130)assertNoHolidays
(292-294)assertHolidayName
(195-199)assertHolidays
(228-230)assertLocalizedHolidays
(327-338)holidays/countries/mongolia.py (3)
Mongolia
(19-67)MN
(70-71)MNG
(74-75)
holidays/calendars/mongolian.py (1)
holidays/helpers.py (1)
_normalize_tuple
(39-49)
🪛 Pylint (3.3.7)
holidays/countries/mongolia.py
[convention] 1-1: Missing module docstring
(C0114)
[convention] 70-70: Missing class docstring
(C0115)
[convention] 74-74: Missing class docstring
(C0115)
tests/countries/test_mongolia.py
[convention] 1-1: Missing module docstring
(C0114)
[convention] 19-19: Missing class docstring
(C0115)
[warning] 21-21: Number of parameters was 4 in 'TestCase.setUpClass' and is now 1 in overriding 'TestMongolia.setUpClass' method
(W0221)
[convention] 25-25: Missing function or method docstring
(C0116)
[convention] 28-28: Missing function or method docstring
(C0116)
[convention] 31-31: Missing function or method docstring
(C0116)
[convention] 34-34: Missing function or method docstring
(C0116)
[convention] 39-39: Missing function or method docstring
(C0116)
[convention] 42-42: Missing function or method docstring
(C0116)
[convention] 45-45: Missing function or method docstring
(C0116)
[convention] 48-48: Missing function or method docstring
(C0116)
[convention] 63-63: Missing function or method docstring
(C0116)
[convention] 77-77: Missing function or method docstring
(C0116)
[convention] 92-92: Missing function or method docstring
(C0116)
holidays/calendars/mongolian.py
[convention] 1-1: Missing module docstring
(C0114)
[refactor] 226-226: Redefining argument with the local name 'year'
(R1704)
[convention] 230-230: Missing function or method docstring
(C0116)
[refactor] 24-24: Too few public methods (1/2)
(R0903)
[refactor] 234-234: Too few public methods (1/2)
(R0903)
holidays/groups/mongolian.py
[convention] 1-1: Missing module docstring
(C0114)
[error] 71-71: Instance of 'MongolianCalendarHolidays' has no '_year' member
(E1101)
[error] 83-83: Instance of 'MongolianCalendarHolidays' has no '_year' member
(E1101)
[error] 95-95: Instance of 'MongolianCalendarHolidays' has no '_year' member
(E1101)
[refactor] 21-21: Too few public methods (0/2)
(R0903)
🔇 Additional comments (21)
holidays/groups/__init__.py (1)
25-25
: Mongolian group import added correctly. TheMongolianCalendarHolidays
class is now exposed in the groups namespace, matching the new module.holidays/countries/__init__.py (1)
127-127
: Mongolia provider import integrated properly. TheMongolia, MN, MNG
import is in alphabetical order between Monaco and Montenegro, aligning with project conventions.holidays/groups/eastern.py (1)
31-35
: Docstring update includes Mongolian calendar. The description now correctly lists Mongolian alongside Buddhist, Chinese, Hindu, and Islamic calendars.holidays/calendars/__init__.py (1)
25-25
: Imported Mongolian calendar classes. The_CustomMongolianHolidays
and_MongolianLunisolar
are now available in the calendars namespace.README.md (1)
108-108
: Supported countries count updated. The total was incremented to 184 to reflect Mongolia's addition. Ensure this matches the actual supported list.holidays/locale/en_MN/LC_MESSAGES/MN.po (1)
1-56
: Localization file structure looks good.The PO file follows proper format with appropriate metadata headers and holiday entries. The empty msgstr fields are expected for the en_MN locale.
holidays/locale/en_US/LC_MESSAGES/MN.po (1)
1-56
: English localization looks comprehensive.The en_US locale file properly provides English translations for all Mongolian holidays. Translations are accurate and follow standard naming conventions.
holidays/locale/mn/LC_MESSAGES/MN.po (1)
1-57
: Mongolian translations look well-formatted.The native Mongolian locale file is properly structured with Cyrillic script translations. The formatting and encoding appear correct, though translation accuracy should ideally be verified by a native Mongolian speaker.
holidays/countries/mongolia.py (1)
1-76
: Excellent implementation following established patterns.The Mongolia holiday provider is well-structured with proper inheritance, metadata, and holiday definitions. The conditional logic for Republic Day starting in 1925 correctly reflects historical context. Class aliases follow standard conventions.
🧰 Tools
🪛 Pylint (3.3.7)
[convention] 1-1: Missing module docstring
(C0114)
[convention] 70-70: Missing class docstring
(C0115)
[convention] 74-74: Missing class docstring
(C0115)
tests/countries/test_mongolia.py (3)
19-26
: Perfect test class structure following established patterns.The class correctly inherits from
CommonCountryTests
andTestCase
, and thesetUpClass
method properly sets up the test range from 1912-2049 matching the Mongolia provider's start year.🧰 Tools
🪛 Pylint (3.3.7)
[convention] 19-19: Missing class docstring
(C0115)
[warning] 21-21: Number of parameters was 4 in 'TestCase.setUpClass' and is now 1 in overriding 'TestMongolia.setUpClass' method
(W0221)
[convention] 25-25: Missing function or method docstring
(C0116)
48-61
: Comprehensive year-specific test validates all holidays.The 2025 test thoroughly validates all expected holidays including the multi-day Naadam festival (July 11-15), ensuring correct date-to-name mappings.
🧰 Tools
🪛 Pylint (3.3.7)
[convention] 48-48: Missing function or method docstring
(C0116)
63-105
: Excellent localization test coverage.Tests validate holiday names in default (en_MN), Mongolian (mn), and US English (en_US) locales, ensuring proper internationalization support.
🧰 Tools
🪛 Pylint (3.3.7)
[convention] 63-63: Missing function or method docstring
(C0116)
[convention] 77-77: Missing function or method docstring
(C0116)
[convention] 92-92: Missing function or method docstring
(C0116)
holidays/calendars/mongolian.py (2)
25-215
: Comprehensive date data for Mongolian lunisolar calendar.The extensive dictionary covering 1912-2100 provides solid foundation for Tsagaan Sar calculations. Data appears consistent with lunisolar calendar patterns.
230-231
: Clean API method for Tsagaan Sar date retrieval.The method provides a clear interface for getting Tsagaan Sar dates with estimation flags.
🧰 Tools
🪛 Pylint (3.3.7)
[convention] 230-230: Missing function or method docstring
(C0116)
holidays/groups/mongolian.py (2)
21-28
: Well-designed mixin class for Mongolian calendar holidays.The class properly extends
EasternCalendarHolidays
and initializes the Mongolian calendar with configurable estimation display.🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 21-21: Too few public methods (0/2)
(R0903)
62-96
:✅ Verification successful
Verify _year attribute availability in inheritance chain.
The methods reference
self._year
which isn't defined in this class. Ensure this class is properly mixed with a class that provides the_year
attribute.Run this script to verify the inheritance pattern:
🏁 Script executed:
#!/bin/bash # Description: Verify that MongolianCalendarHolidays is used with classes that provide _year # Search for classes that inherit from MongolianCalendarHolidays ast-grep --pattern 'class $_ ($$$, MongolianCalendarHolidays$$$) { $$$ }' # Search for _year attribute in base classes rg -A 5 "_year.*=" holidays/holiday_base.py holidays/groups/Length of output: 13926
I want to double-check how
MongolianCalendarHolidays
is defined and where it’s mixed into a concrete country class. Let’s inspect the class declaration inholidays/groups/mongolian.py
:
🏁 Script executed:
#!/bin/bash # List class definitions in the Mongolian mixin and show its beginning rg --color=never -n "class " holidays/groups/mongolian.py sed -n '1,200p' holidays/groups/mongolian.pyLength of output: 3918
Let’s inspect the country class that pulls in
MongolianCalendarHolidays
to see how its__init__
chain is wired:#!/bin/bash # 1. Find the file(s) defining the Mongolia country class country_files=$(rg -l "class Mongolia" -g "*.py") echo "Mongolia country class found in:" echo "$country_files" # 2. Show the class declaration and any __init__ override for f in $country_files; do echo -e "\n--- $f: class declaration ---" rg -A10 -n "class Mongolia" "$f" echo -e "\n--- $f: __init__ method ---" rg -A5 -n "def __init__" "$f" || echo "No __init__ in $f" done
Let’s locate and inspect the Mongolia country class to see how its
__init__
wires in bothHolidayBase
andMongolianCalendarHolidays
:
🏁 Script executed:
#!/bin/bash # 1. Find files defining class Mongolia files=$(rg -l "class Mongolia" -g "*.py") echo "Found in: $files" # 2. Show the class declaration and __init__ for each for f in $files; do echo -e "\n--- $f: class & __init__ ---" rg -A5 -n "class Mongolia" "$f" rg -A10 -n "def __init__" "$f" || echo "No __init__ in $f" doneLength of output: 2150
Verified:
_year
is provided byHolidayBase
viasuper().__init__
The
Mongolia
country class callsInternationalHolidays.__init__
and thensuper().__init__(*args, **kwargs)
, which invokesHolidayBase.__init__
(settingself._year
). The mix-in chain guarantees_year
is available—no changes needed.🧰 Tools
🪛 Pylint (3.3.7)
[error] 71-71: Instance of 'MongolianCalendarHolidays' has no '_year' member
(E1101)
[error] 83-83: Instance of 'MongolianCalendarHolidays' has no '_year' member
(E1101)
[error] 95-95: Instance of 'MongolianCalendarHolidays' has no '_year' member
(E1101)
scripts/calendar/asian_generator.py (5)
300-307
: Clean implementation following established patterns.The method correctly calculates Buddha Day using the 15th day of the 4th lunar month, consistent with other lunisolar date methods in the class.
370-376
: Proper integration of Mongolian calendar support.The enumeration and dictionary updates correctly follow the established pattern for adding new calendar types.
380-380
: Correct timezone classification.Adding Mongolia (MN) to the UTC+8 list is factually accurate.
383-410
: Holiday entries follow correct format.The new Mongolian holiday entries properly follow the established tuple pattern and use the correct calendar enumeration.
318-325
:❓ Verification inconclusive
Fix inconsistency between comment and implementation.
The comment states "1st day of the 6th month" but the code uses
lunar_to_gre(year, 10, 1)
(10th month). This mismatch needs clarification.
🏁 Script executed:
#!/bin/bash # Verify the correct lunar month for Genghis Khan's Birthday rg -A 10 -B 5 "Genghis Khan" --type mdLength of output: 40
Let’s broaden our search for any mentions of Genghis Khan’s birthday in the repo to pin down the correct lunar month:
🏁 Script executed:
#!/bin/bash # Search repository for any references to Genghis Khan’s birthday rg -i "genghis khan" -C5Length of output: 1661
Clarify Mongolian lunar month for Genghis Khan’s Birthday
The docstring says “1st day of the 6th month” but both the implementation (lunar_to_gre(year, 10, 1)
) and the holiday tuple(10, 1, "GENGHIS_KHAN_DAY", MONGOLIAN)
use the 10th month. Please confirm the correct lunar month and then either:
- Update the docstring to “1st day of the 10th month of the Mongolian lunar calendar.”
- Or adjust the code/tuple to use the 6th month if that’s the accurate date.
Files to review:
- scripts/calendar/asian_generator.py →
genghis_khan_day_date
docstring (lines 318–325)- scripts/calendar/asian_generator.py → holiday definition tuple
Signed-off-by: Ankush_Kapoor <work.ankushkapoor1626@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
README.md
(2 hunks)holidays/countries/mongolia.py
(1 hunks)holidays/locale/en_MN/LC_MESSAGES/MN.po
(1 hunks)holidays/locale/en_US/LC_MESSAGES/MN.po
(1 hunks)holidays/locale/mn/LC_MESSAGES/MN.po
(1 hunks)tests/countries/test_mongolia.py
(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
tests/countries/test_mongolia.py (1)
Learnt from: PPsyrius
PR: vacanza/holidays#2416
File: tests/countries/test_turkmenistan.py:31-49
Timestamp: 2025-04-05T04:50:40.752Z
Learning: For Turkmenistan holiday tests, use this class structure: `class TestTurkmenistan(CommonCountryTests, TestCase)` with imports `from unittest import TestCase`, `from holidays.countries import Turkmenistan, TM, TKM`, and `from tests.common import CommonCountryTests`. Ensure to call `super().setUp()` in the setUp method.
🪛 Pylint (3.3.7)
holidays/countries/mongolia.py
[convention] 1-1: Missing module docstring
(C0114)
[convention] 75-75: Missing class docstring
(C0115)
[convention] 79-79: Missing class docstring
(C0115)
tests/countries/test_mongolia.py
[convention] 1-1: Missing module docstring
(C0114)
[convention] 19-19: Missing class docstring
(C0115)
[warning] 21-21: Number of parameters was 4 in 'TestCase.setUpClass' and is now 1 in overriding 'TestMongolia.setUpClass' method
(W0221)
[convention] 25-25: Missing function or method docstring
(C0116)
[convention] 28-28: Missing function or method docstring
(C0116)
[convention] 31-31: Missing function or method docstring
(C0116)
[convention] 34-34: Missing function or method docstring
(C0116)
[convention] 39-39: Missing function or method docstring
(C0116)
[convention] 42-42: Missing function or method docstring
(C0116)
[convention] 47-47: Missing function or method docstring
(C0116)
[convention] 58-58: Missing function or method docstring
(C0116)
[convention] 73-73: Missing function or method docstring
(C0116)
[convention] 87-87: Missing function or method docstring
(C0116)
[convention] 102-102: Missing function or method docstring
(C0116)
🔇 Additional comments (13)
README.md (1)
941-946
: LGTM - Mongolia entry properly structured.The table entry follows the correct format with appropriate country code, language support, and alphabetical positioning.
holidays/locale/en_US/LC_MESSAGES/MN.po (1)
1-60
: Well-structured localization file.The PO file follows proper gettext format with appropriate metadata and clean English translations for Mongolian holidays.
holidays/locale/en_MN/LC_MESSAGES/MN.po (1)
1-60
: Consistent English localization for en_MN locale.The file properly supports the en_MN locale with appropriate English translations. Content consistency with en_US version appears intentional.
holidays/locale/mn/LC_MESSAGES/MN.po (1)
30-61
: Appropriate native language handling.The empty msgstrs ensure Mongolian holiday names display in their native form, which is the correct approach for preserving cultural authenticity.
holidays/countries/mongolia.py (2)
19-37
: Solid holiday provider implementation.The class structure properly inherits from the required base classes and sets appropriate metadata. The start year and language configuration align with the discussion in past reviews.
38-73
: Well-implemented holiday logic.The holiday definitions follow proper patterns with conditional logic for historical changes. The use of translatable strings and appropriate date methods looks correct.
tests/countries/test_mongolia.py (7)
1-17
: Well-structured imports and setup following established patterns.The file follows the proper structure with correct imports and matches the pattern from other country test files. The copyright header is standard and imports are appropriate.
🧰 Tools
🪛 Pylint (3.3.7)
[convention] 1-1: Missing module docstring
(C0114)
19-23
: Proper test class setup with correct inheritance.Good use of the CommonCountryTests pattern and appropriate year range (2004-2050). The setUpClass method correctly calls the parent class setup.
🧰 Tools
🪛 Pylint (3.3.7)
[convention] 19-19: Missing class docstring
(C0115)
[warning] 21-21: Number of parameters was 4 in 'TestCase.setUpClass' and is now 1 in overriding 'TestMongolia.setUpClass' method
(W0221)
25-29
: Solid country aliases and boundary testing.Both methods correctly test the country aliases and verify no holidays exist before 2004, establishing proper boundaries for the holiday data.
🧰 Tools
🪛 Pylint (3.3.7)
[convention] 25-25: Missing function or method docstring
(C0116)
[convention] 28-28: Missing function or method docstring
(C0116)
31-46
: Comprehensive coverage of core holidays.The tests for New Year, Women's Day, Children's Day, and Republic Day are well-structured with proper Mongolian holiday names and consistent date ranges.
🧰 Tools
🪛 Pylint (3.3.7)
[convention] 31-31: Missing function or method docstring
(C0116)
[convention] 34-34: Missing function or method docstring
(C0116)
[convention] 39-39: Missing function or method docstring
(C0116)
[convention] 42-42: Missing function or method docstring
(C0116)
47-56
: Excellent handling of holiday name evolution over time.This test properly captures the evolution of Independence Day naming across different periods (2007-2010 vs 2011+), demonstrating thoughtful attention to historical accuracy.
🧰 Tools
🪛 Pylint (3.3.7)
[convention] 47-47: Missing function or method docstring
(C0116)
58-71
: Comprehensive year-specific test with multi-day festival coverage.The 2025 test provides excellent validation of all holidays including the multi-day Naadam festival. This gives confidence in the complete holiday implementation.
🧰 Tools
🪛 Pylint (3.3.7)
[convention] 58-58: Missing function or method docstring
(C0116)
73-115
: Thorough localization testing across multiple locales.The localization tests properly cover default (Mongolian), en_MN, and en_US variants, ensuring proper translation support. This demonstrates solid internationalization coverage.
🧰 Tools
🪛 Pylint (3.3.7)
[convention] 73-73: Missing function or method docstring
(C0116)
[convention] 87-87: Missing function or method docstring
(C0116)
[convention] 102-102: Missing function or method docstring
(C0116)
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Ankush_Kapoor <work.ankushkapoor1626@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 7
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
README.md
(2 hunks)scripts/calendar/mongolian_generator.py
(1 hunks)
🧰 Additional context used
🪛 Pylint (3.3.7)
scripts/calendar/mongolian_generator.py
[convention] 1-1: Missing module docstring
(C0114)
[convention] 19-19: Missing function or method docstring
(C0116)
[convention] 23-23: Missing function or method docstring
(C0116)
[convention] 28-28: Constant name "Mzero" doesn't conform to UPPER_CASE naming style
(C0103)
[convention] 29-29: Constant name "epoch" doesn't conform to UPPER_CASE naming style
(C0103)
[convention] 30-30: Constant name "ixx" doesn't conform to UPPER_CASE naming style
(C0103)
[convention] 31-31: Constant name "betastar" doesn't conform to UPPER_CASE naming style
(C0103)
[convention] 32-32: Constant name "beta" doesn't conform to UPPER_CASE naming style
(C0103)
[convention] 47-47: Missing function or method docstring
(C0116)
[convention] 51-51: Missing function or method docstring
(C0116)
[convention] 56-56: Missing function or method docstring
(C0116)
[convention] 61-61: Missing function or method docstring
(C0116)
[convention] 68-68: Missing function or method docstring
(C0116)
[convention] 82-82: Missing function or method docstring
(C0116)
[convention] 96-96: Missing function or method docstring
(C0116)
[convention] 106-106: Missing function or method docstring
(C0116)
[convention] 112-112: Missing function or method docstring
(C0116)
[convention] 123-123: Missing function or method docstring
(C0116)
[convention] 128-128: Missing function or method docstring
(C0116)
[warning] 132-132: Catching too general exception Exception
(W0718)
[convention] 136-136: Missing function or method docstring
(C0116)
[warning] 142-144: Consider explicitly re-raising using 'except Exception as exc' and 'raise ValueError(f'Invalid lunar festival date in year {y}, month {m_month}, day {m_day}') from exc'
(W0707)
[convention] 171-171: Missing function or method docstring
(C0116)
[warning] 184-184: Catching too general exception Exception
(W0718)
🔇 Additional comments (2)
scripts/calendar/mongolian_generator.py (1)
47-49
: Update constant references after renaming.Remember to update the constant references here.
🧰 Tools
🪛 Pylint (3.3.7)
[convention] 47-47: Missing function or method docstring
(C0116)
README.md (1)
941-946
:✅ Verification successful
Verify default language designation.
The entry correctly adds Mongolia with
mn
as the default language (in bold), which aligns with the past review comment suggesting this.Let me verify Mongolia's language configuration matches what's defined in the codebase:
🏁 Script executed:
#!/bin/bash # Check Mongolia's language configuration in the code echo "Checking Mongolia's default language configuration..." rg -A 5 "class Mongolia" holidays/countries/ echo -e "\nChecking for default_language attribute..." ast-grep --pattern 'default_language = $_'Length of output: 9490
Default language for Mongolia verified
The
default_language
in holidays/countries/mongolia.py:30 is set to"mn"
, which matches the README’s bolded entry. All good to merge.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ankushhKapoor, good work! I like such things. 😋 There is some refactoring suggestions.
Thankss |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
README.md
(2 hunks)
🔇 Additional comments (1)
README.md (1)
941-946
: Mongolia table row is well-formed.The new Mongolia entry is correctly inserted, the HTML table syntax is valid, and the default language (
mn
) is bolded as intended.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
♻️ Duplicate comments (2)
scripts/calendar/mongolian_generator.py (2)
171-171
: Address past reviewer feedback on exception handling.Based on past review comments, the broad exception handling appears unnecessary since no actual exceptions are raised during execution.
Past reviewer KJhellico asked "In what case can exception be raised here?" and the contributor confirmed these exception handlers were added preventatively but aren't needed since no exceptions occur in practice.
🧰 Tools
🪛 Pylint (3.3.7)
[warning] 171-171: Catching too general exception Exception
(W0718)
28-42
: Follow Python naming conventions for constants.Multiple past reviewers have flagged these constants for not following UPPER_CASE naming conventions.
-Mzero = 3 -epoch = 1747 -ixx = 46 -betastar = 10 -beta = 172 +MZERO = 3 +EPOCH = 1747 +IXX = 46 +BETASTAR = 10 +BETA = 172Update all references throughout the file accordingly.
🧰 Tools
🪛 Pylint (3.3.7)
[convention] 28-28: Constant name "Mzero" doesn't conform to UPPER_CASE naming style
(C0103)
[convention] 29-29: Constant name "epoch" doesn't conform to UPPER_CASE naming style
(C0103)
[convention] 30-30: Constant name "ixx" doesn't conform to UPPER_CASE naming style
(C0103)
[convention] 31-31: Constant name "betastar" doesn't conform to UPPER_CASE naming style
(C0103)
[convention] 32-32: Constant name "beta" doesn't conform to UPPER_CASE naming style
(C0103)
[convention] 34-34: Constant name "m0" doesn't conform to UPPER_CASE naming style
(C0103)
[convention] 35-35: Constant name "m1" doesn't conform to UPPER_CASE naming style
(C0103)
[convention] 36-36: Constant name "m2" doesn't conform to UPPER_CASE naming style
(C0103)
[convention] 37-37: Constant name "s0" doesn't conform to UPPER_CASE naming style
(C0103)
[convention] 38-38: Constant name "s1" doesn't conform to UPPER_CASE naming style
(C0103)
[convention] 39-39: Constant name "s2" doesn't conform to UPPER_CASE naming style
(C0103)
[convention] 40-40: Constant name "a0" doesn't conform to UPPER_CASE naming style
(C0103)
[convention] 41-41: Constant name "a1" doesn't conform to UPPER_CASE naming style
(C0103)
[convention] 42-42: Constant name "a2" doesn't conform to UPPER_CASE naming style
(C0103)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
holidays/calendars/mongolian.py
(1 hunks)holidays/groups/mongolian.py
(1 hunks)scripts/calendar/mongolian_generator.py
(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
holidays/calendars/mongolian.py (2)
holidays/holiday_base.py (1)
get
(967-990)scripts/calendar/asian_generator.py (2)
buddha_day_date
(300-307)genghis_khan_day_date
(318-325)
holidays/groups/mongolian.py (3)
holidays/calendars/mongolian.py (4)
_MongolianLunisolar
(24-338)tsagaan_sar_date
(331-332)buddha_day_date
(334-335)genghis_khan_day_date
(337-338)holidays/groups/eastern.py (2)
EasternCalendarHolidays
(19-51)_add_eastern_calendar_holiday
(24-51)scripts/calendar/asian_generator.py (2)
buddha_day_date
(300-307)genghis_khan_day_date
(318-325)
🪛 Pylint (3.3.7)
holidays/calendars/mongolian.py
[convention] 1-1: Missing module docstring
(C0114)
[convention] 331-331: Missing function or method docstring
(C0116)
[convention] 334-334: Missing function or method docstring
(C0116)
[convention] 337-337: Missing function or method docstring
(C0116)
scripts/calendar/mongolian_generator.py
[convention] 1-1: Missing module docstring
(C0114)
[warning] 18-26: String statement has no effect
(W0105)
[convention] 28-28: Constant name "Mzero" doesn't conform to UPPER_CASE naming style
(C0103)
[convention] 29-29: Constant name "epoch" doesn't conform to UPPER_CASE naming style
(C0103)
[convention] 30-30: Constant name "ixx" doesn't conform to UPPER_CASE naming style
(C0103)
[convention] 31-31: Constant name "betastar" doesn't conform to UPPER_CASE naming style
(C0103)
[convention] 32-32: Constant name "beta" doesn't conform to UPPER_CASE naming style
(C0103)
[convention] 34-34: Constant name "m0" doesn't conform to UPPER_CASE naming style
(C0103)
[convention] 35-35: Constant name "m1" doesn't conform to UPPER_CASE naming style
(C0103)
[convention] 36-36: Constant name "m2" doesn't conform to UPPER_CASE naming style
(C0103)
[convention] 37-37: Constant name "s0" doesn't conform to UPPER_CASE naming style
(C0103)
[convention] 38-38: Constant name "s1" doesn't conform to UPPER_CASE naming style
(C0103)
[convention] 39-39: Constant name "s2" doesn't conform to UPPER_CASE naming style
(C0103)
[convention] 40-40: Constant name "a0" doesn't conform to UPPER_CASE naming style
(C0103)
[convention] 41-41: Constant name "a1" doesn't conform to UPPER_CASE naming style
(C0103)
[convention] 42-42: Constant name "a2" doesn't conform to UPPER_CASE naming style
(C0103)
[convention] 45-45: Missing function or method docstring
(C0116)
[convention] 49-49: Missing function or method docstring
(C0116)
[convention] 54-54: Missing function or method docstring
(C0116)
[convention] 59-59: Missing function or method docstring
(C0116)
[convention] 65-65: Missing function or method docstring
(C0116)
[convention] 79-79: Missing function or method docstring
(C0116)
[convention] 93-93: Missing function or method docstring
(C0116)
[convention] 103-103: Missing function or method docstring
(C0116)
[convention] 109-109: Missing function or method docstring
(C0116)
[convention] 120-120: Missing function or method docstring
(C0116)
[convention] 125-125: Missing function or method docstring
(C0116)
[convention] 130-130: Missing function or method docstring
(C0116)
[convention] 160-160: Missing function or method docstring
(C0116)
[warning] 171-171: Catching too general exception Exception
(W0718)
holidays/groups/mongolian.py
[convention] 1-1: Missing module docstring
(C0114)
[error] 52-52: Instance of 'MongolianCalendarHolidays' has no '_year' member
(E1101)
[error] 64-64: Instance of 'MongolianCalendarHolidays' has no '_year' member
(E1101)
[error] 76-76: Instance of 'MongolianCalendarHolidays' has no '_year' member
(E1101)
[error] 89-89: Instance of 'MongolianCalendarHolidays' has no '_year' member
(E1101)
[error] 102-102: Instance of 'MongolianCalendarHolidays' has no '_year' member
(E1101)
[refactor] 20-20: Too few public methods (0/2)
(R0903)
🔇 Additional comments (11)
holidays/calendars/mongolian.py (4)
19-21
: Good choice using descriptive constants for holiday names.The constant definitions provide clear, reusable identifiers that improve maintainability across the codebase.
25-323
: Comprehensive holiday date mappings.The extensive date dictionaries covering 2004-2100 provide solid long-term support for Mongolian holidays. Data appears well-structured and follows consistent formatting.
325-329
: Robust holiday date retrieval logic.The method correctly handles both exact and estimated dates with proper fallback logic. The tuple return format with estimation flag is well-designed for downstream usage.
341-342
: Clean integration design.The
_CustomMongolianHolidays
class provides clean integration between custom calendar functionality and Mongolian lunisolar calculations through multiple inheritance.holidays/groups/mongolian.py (5)
20-28
: Well-structured calendar holidays class.The class design properly extends
EasternCalendarHolidays
and initializes calendar dependencies cleanly. The optional custom calendar class parameter provides good flexibility.🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 20-20: Too few public methods (0/2)
(R0903)
29-41
: Solid delegation pattern for holiday addition.The
_add_mongolian_calendar_holiday
method provides clean delegation to the eastern calendar framework while maintaining Mongolian-specific behavior.
43-53
: Excellent documentation with historical context.The method documentation includes valuable context about Tsagaan Sar with proper Wikipedia references. Implementation correctly uses the calendar's date calculation.
🧰 Tools
🪛 Pylint (3.3.7)
[error] 52-52: Instance of 'MongolianCalendarHolidays' has no '_year' member
(E1101)
55-77
: Consistent pattern for multi-day holidays.The Tsagaan Sar day 2 and day 3 methods follow a clean pattern using
days_delta
parameter. Documentation maintains consistency across related methods.🧰 Tools
🪛 Pylint (3.3.7)
[error] 64-64: Instance of 'MongolianCalendarHolidays' has no '_year' member
(E1101)
[error] 76-76: Instance of 'MongolianCalendarHolidays' has no '_year' member
(E1101)
79-103
: Comprehensive holiday implementations.Both Buddha Day and Genghis Khan Day methods include proper documentation with historical context and correct calendar integration. The implementation pattern is consistent across all holiday methods.
🧰 Tools
🪛 Pylint (3.3.7)
[error] 89-89: Instance of 'MongolianCalendarHolidays' has no '_year' member
(E1101)
[error] 102-102: Instance of 'MongolianCalendarHolidays' has no '_year' member
(E1101)
scripts/calendar/mongolian_generator.py (2)
160-188
: Solid date generation and file output logic.The main generation function correctly iterates through years, formats dates properly, and handles file I/O cleanly. The template-based approach for generating the output class is well-designed.
🧰 Tools
🪛 Pylint (3.3.7)
[convention] 160-160: Missing function or method docstring
(C0116)
[warning] 171-171: Catching too general exception Exception
(W0718)
153-157
: Clean holiday configuration setup.The
MONGOLIAN_HOLIDAYS
list provides a clean mapping between holiday names and calculation functions. The lambda expressions for festival dates are concise and appropriate.
"""
NOTE:
The following observances are currently unimplemented due to unclear week-based scheduling rules:
1. National Literary, Cultural and Book Days (Үндэсний бичиг соёл, номын өдрүүд):
- Observed on Saturday and Sunday of the 3rd week of May and September.
- Officially added on July 2nd, 2021.
2. Environmental Protection Days (Байгаль орчныг хамгаалах өдрүүд):
- Observed during the 4th week of September.
These are defined using "week of the month" logic, but it is unclear whether weeks start from
the 1st of the month or the first full week. We previously contacted contact@mecc.gov.mn for
clarification but received no response.
See:
https://github.com/vacanza/holidays/issues/2672
""" This comment is appropriate to add at the start of Please let me know if there are any other changes or improvements you'd like me to make. |
Let's go with this 👍 |
You need to run |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
README.md
(1 hunks)holidays/countries/__init__.py
(1 hunks)
🧰 Additional context used
🪛 Flake8 (7.2.0)
holidays/countries/__init__.py
[error] 141-141: 'holidays.countries.mongolia.Mongolia' imported but unused
(F401)
[error] 141-141: 'holidays.countries.mongolia.MN' imported but unused
(F401)
[error] 141-141: 'holidays.countries.mongolia.MNG' imported but unused
(F401)
🔇 Additional comments (1)
README.md (1)
997-1002
: Row looks good – no HTML or ordering issues detectedThe Mongolia entry follows the surrounding markup (five
<td>
cells, bold default language, alphabetical placement between Monaco and Montenegro). Nice work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
♻️ Duplicate comments (2)
holidays/groups/mongolian.py (1)
1-1
: The module docstring suggestion remains unaddressedBased on the past review comments, there was a suggestion to add a module docstring, but it appears this wasn't implemented. This indicates the project may follow the same pattern for groups files as country files.
holidays/countries/mongolia.py (1)
159-164
: Verify the 46-day New Harvest Days implementationThe loop correctly implements the extensive New Harvest Days period. However, consider adding a guard to handle the case where
_add_holiday_sep_5
returnsNone
:if self._year >= 2007: # New Harvest Days. name = tr("Шинэ ургацын өдрүүд") dt = self._add_holiday_sep_5(name) - for delta in range(1, 46): - self._add_holiday(name, _timedelta(dt, delta)) + if dt: + for delta in range(1, 46): + self._add_holiday(name, _timedelta(dt, delta))
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (7)
README.md
(2 hunks)holidays/calendars/mongolian.py
(1 hunks)holidays/countries/mongolia.py
(1 hunks)holidays/groups/mongolian.py
(1 hunks)holidays/locale/en_US/LC_MESSAGES/MN.po
(1 hunks)holidays/locale/mn/LC_MESSAGES/MN.po
(1 hunks)tests/countries/test_mongolia.py
(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
holidays/locale/mn/LC_MESSAGES/MN.po (9)
Learnt from: KJhellico
PR: vacanza/holidays#2259
File: holidays/locale/en_IN/LC_MESSAGES/IN.po:30-299
Timestamp: 2025-03-05T17:51:00.633Z
Learning: In the Holidays project, .po files for a country's default locale use empty msgstr fields as a standard convention.
Learnt from: PPsyrius
PR: vacanza/holidays#2438
File: holidays/locale/ar_IQ/LC_MESSAGES/IQ.po:35-81
Timestamp: 2025-04-17T17:08:48.082Z
Learning: In holiday PO files, when the file represents the default language of an entity (e.g., ar_IQ for Iraq), no translations in `msgstr` are required as the `msgid` values are already in the target language.
Learnt from: KJhellico
PR: vacanza/holidays#2394
File: holidays/locale/pt_PT/LC_MESSAGES/CV.po:31-88
Timestamp: 2025-03-31T19:37:57.691Z
Learning: In the holidays library, when a locale file matches the country's default language (e.g., pt_PT for Cape Verde), the msgstr fields should be left empty. Only non-default language files should have filled msgstr fields with translations.
Learnt from: KJhellico
PR: vacanza/holidays#2394
File: holidays/locale/pt_PT/LC_MESSAGES/CV.po:31-88
Timestamp: 2025-03-31T19:37:57.691Z
Learning: In the holidays library localization pattern, when a locale file matches a country's default language (e.g., pt_PT for Cape Verde), the msgstr fields should be left empty. Only non-default language locale files should have translations in the msgstr fields.
Learnt from: KJhellico
PR: vacanza/holidays#2465
File: holidays/locale/nl/LC_MESSAGES/SR.po:31-32
Timestamp: 2025-04-13T19:11:32.337Z
Learning: In the holidays library, when a language is set as the default language for a country (like Dutch/nl for Suriname), the `msgid` strings in the PO file are already in that language, and the corresponding `msgstr` fields are intentionally left empty. This is by design, as the system will fall back to using the `msgid` value when no translation is provided.
Learnt from: KJhellico
PR: vacanza/holidays#2635
File: holidays/locale/dz_BT/LC_MESSAGES/BT.po:29-32
Timestamp: 2025-06-16T14:05:58.032Z
Learning: For Bhutan holidays, dz_BT (Dzongkha) is the default language. In default language .po files, empty msgstr values are correct since the msgid already contains the source text in the default language.
Learnt from: PPsyrius
PR: vacanza/holidays#2388
File: holidays/locale/fr/LC_MESSAGES/CI.po:28-101
Timestamp: 2025-03-30T13:33:31.598Z
Learning: In the holidays library, for localization files of the default language (like French for Ivory Coast in fr/LC_MESSAGES/CI.po), the best practice is to leave the message strings (msgstr) empty to avoid possible typos, since the message IDs (msgid) are already in the target language.
Learnt from: KJhellico
PR: vacanza/holidays#2530
File: holidays/locale/ca/LC_MESSAGES/AD.po:31-40
Timestamp: 2025-05-06T15:25:44.333Z
Learning: In the Holidays project, msgid fields in localization files contain strings in the entity's default language (as defined by default_language attribute), not English source strings as in standard gettext implementations.
Learnt from: PPsyrius
PR: vacanza/holidays#2537
File: holidays/locale/sv_FI/LC_MESSAGES/AX.po:17-27
Timestamp: 2025-05-10T04:34:02.406Z
Learning: The `Plural-Forms` header isn't used in .po file generation for the holidays project and doesn't need to be manually added to localization files.
holidays/countries/mongolia.py (10)
Learnt from: KJhellico
PR: vacanza/holidays#2609
File: holidays/countries/nauru.py:149-154
Timestamp: 2025-06-09T19:50:56.039Z
Learning: In the holidays library project, never suggest adding docstrings for alias classes (like NR, NRU country code aliases that inherit from main country classes). The project deliberately omits docstrings for these simple alias classes.
Learnt from: ankushhKapoor
PR: vacanza/holidays#2601
File: holidays/calendars/mongolian.py:1-1
Timestamp: 2025-06-10T12:43:10.577Z
Learning: The holidays project's calendar files (in holidays/calendars/) follow a consistent pattern of NOT having module-level docstrings. They only use class-level docstrings for their main classes. When reviewing calendar files, maintain this consistency and do not suggest adding module docstrings.
Learnt from: KJhellico
PR: vacanza/holidays#2654
File: holidays/countries/cape_verde.py:1-12
Timestamp: 2025-06-21T16:30:12.740Z
Learning: The holidays project does not use module docstrings in country holiday files. All country files start directly with the copyright header comment block without module docstrings, maintaining a consistent coding style across the project.
Learnt from: KJhellico
PR: vacanza/holidays#2671
File: holidays/countries/libya.py:1-1
Timestamp: 2025-06-22T21:33:17.824Z
Learning: In the holidays library, country files do not use module docstrings. The established pattern is: license header comment block, imports, then class definitions with class docstrings. Module docstrings should not be added to country files as they don't follow this convention.
Learnt from: PPsyrius
PR: vacanza/holidays#2638
File: holidays/countries/svalbard_and_jan_mayen.py:1-12
Timestamp: 2025-06-16T11:42:28.293Z
Learning: In the holidays library codebase, country implementation files in holidays/countries/ follow a standard convention of NOT having module-level docstrings. They start with the license header comment block, followed by imports, then class definitions. This is consistent across all country implementations like austria.py, belgium.py, canada.py, etc.
Learnt from: KJhellico
PR: vacanza/holidays#2583
File: holidays/countries/niger.py:1-1
Timestamp: 2025-06-01T17:58:53.279Z
Learning: In the holidays project, module-level docstrings are not required or needed for country holiday files in the holidays/countries/ directory.
Learnt from: PPsyrius
PR: vacanza/holidays#2416
File: tests/countries/test_turkmenistan.py:52-64
Timestamp: 2025-04-05T04:47:27.213Z
Learning: For holiday tests in the vacanza/holidays project, structure tests by individual holidays rather than by years. Each test method should focus on a specific holiday and test it across multiple years (from start_year through 2050) using helper methods like `assertHolidayName`. For fixed holidays, use generators like `(f"{year}-01-01" for year in range(1991, 2051))`. For movable holidays, specify individual dates for specific years followed by a range check.
Learnt from: KJhellico
PR: vacanza/holidays#2398
File: holidays/countries/guinea.py:106-110
Timestamp: 2025-04-04T10:52:41.546Z
Learning: In the Guinea holidays implementation, observed Eid al-Fitr cases are properly covered by the test_eid_al_fitr_day() method, which tests both the regular holiday dates and the observed dates when the holiday falls on a non-working day (for years >= 2023).
Learnt from: KJhellico
PR: vacanza/holidays#2398
File: holidays/countries/guinea.py:106-110
Timestamp: 2025-04-04T10:52:41.546Z
Learning: In the Guinea holidays implementation, observed Eid al-Fitr cases are covered by the test_eid_al_fitr_day() method, which tests both regular holiday dates and the observed dates when the holiday falls on a non-working day (for years >= 2023).
Learnt from: PPsyrius
PR: vacanza/holidays#2601
File: holidays/countries/mongolia.py:140-146
Timestamp: 2025-06-15T11:50:44.366Z
Learning: The holidays library doesn't currently have helper functions like `_add_holiday_span` for adding consecutive multi-day holidays. Countries like Mongolia, Thailand, and Vietnam implement multi-day holiday spans using inline loops with `_timedelta(dt, delta)` pattern.
tests/countries/test_mongolia.py (3)
Learnt from: PPsyrius
PR: vacanza/holidays#2416
File: tests/countries/test_turkmenistan.py:31-49
Timestamp: 2025-04-05T04:50:40.752Z
Learning: For Turkmenistan holiday tests, use this class structure: `class TestTurkmenistan(CommonCountryTests, TestCase)` with imports `from unittest import TestCase`, `from holidays.countries import Turkmenistan, TM, TKM`, and `from tests.common import CommonCountryTests`. Ensure to call `super().setUp()` in the setUp method.
Learnt from: PPsyrius
PR: vacanza/holidays#2416
File: tests/countries/test_turkmenistan.py:52-64
Timestamp: 2025-04-05T04:47:27.213Z
Learning: For holiday tests in the vacanza/holidays project, structure tests by individual holidays rather than by years. Each test method should focus on a specific holiday and test it across multiple years (from start_year through 2050) using helper methods like `assertHolidayName`. For fixed holidays, use generators like `(f"{year}-01-01" for year in range(1991, 2051))`. For movable holidays, specify individual dates for specific years followed by a range check.
Learnt from: PPsyrius
PR: vacanza/holidays#2601
File: tests/countries/test_mongolia.py:128-156
Timestamp: 2025-06-15T11:52:39.572Z
Learning: In the vacanza/holidays project tests, when testing holidays that span multiple consecutive days across many years (like Mongolia's National Festival spanning July 11-13), prefer explicit for loops over complex nested generator expressions with unpacking. The explicit loops are more readable, easier to maintain, and better communicate the testing intent even though the Big O complexity is equivalent.
🪛 Pylint (3.3.7)
holidays/countries/mongolia.py
[convention] 25-25: Line too long (137/100)
(C0301)
[convention] 26-26: Line too long (117/100)
(C0301)
[convention] 28-28: Line too long (134/100)
(C0301)
[convention] 29-29: Line too long (101/100)
(C0301)
[convention] 30-30: Line too long (106/100)
(C0301)
[convention] 1-1: Missing module docstring
(C0114)
[convention] 197-197: Missing class docstring
(C0115)
[convention] 201-201: Missing class docstring
(C0115)
holidays/calendars/mongolian.py
[convention] 1-1: Missing module docstring
(C0114)
[convention] 331-331: Missing function or method docstring
(C0116)
[convention] 334-334: Missing function or method docstring
(C0116)
[convention] 337-337: Missing function or method docstring
(C0116)
holidays/groups/mongolian.py
[convention] 1-1: Missing module docstring
(C0114)
[error] 53-53: Instance of 'MongolianCalendarHolidays' has no '_year' member
(E1101)
[error] 66-66: Instance of 'MongolianCalendarHolidays' has no '_year' member
(E1101)
[error] 78-78: Instance of 'MongolianCalendarHolidays' has no '_year' member
(E1101)
[error] 90-90: Instance of 'MongolianCalendarHolidays' has no '_year' member
(E1101)
[error] 102-102: Instance of 'MongolianCalendarHolidays' has no '_year' member
(E1101)
[refactor] 20-20: Too few public methods (0/2)
(R0903)
tests/countries/test_mongolia.py
[convention] 1-1: Missing module docstring
(C0114)
[convention] 20-20: Missing class docstring
(C0115)
[warning] 22-22: Number of parameters was 4 in 'TestCase.setUpClass' and is now 1 in overriding 'TestMongolia.setUpClass' method
(W0221)
[convention] 27-27: Missing function or method docstring
(C0116)
[convention] 30-30: Missing function or method docstring
(C0116)
[convention] 33-33: Missing function or method docstring
(C0116)
[convention] 36-36: Missing function or method docstring
(C0116)
[convention] 43-43: Missing function or method docstring
(C0116)
[convention] 82-82: Missing function or method docstring
(C0116)
[convention] 90-90: Missing function or method docstring
(C0116)
[convention] 95-95: Missing function or method docstring
(C0116)
[convention] 109-109: Missing function or method docstring
(C0116)
[convention] 116-116: Missing function or method docstring
(C0116)
[convention] 123-123: Missing function or method docstring
(C0116)
[convention] 130-130: Missing function or method docstring
(C0116)
[convention] 133-133: Missing function or method docstring
(C0116)
[convention] 146-146: Missing function or method docstring
(C0116)
[convention] 154-154: Missing function or method docstring
(C0116)
[convention] 168-168: Missing function or method docstring
(C0116)
[convention] 175-175: Missing function or method docstring
(C0116)
[convention] 184-184: Missing function or method docstring
(C0116)
[convention] 191-191: Missing function or method docstring
(C0116)
[convention] 198-198: Missing function or method docstring
(C0116)
[convention] 212-212: Missing function or method docstring
(C0116)
[convention] 225-225: Missing function or method docstring
(C0116)
[convention] 241-241: Missing function or method docstring
(C0116)
[convention] 248-248: Missing function or method docstring
(C0116)
[convention] 264-264: Missing function or method docstring
(C0116)
[convention] 285-285: Missing function or method docstring
(C0116)
[convention] 364-364: Missing function or method docstring
(C0116)
[refactor] 20-20: Too many public methods (28/20)
(R0904)
🔇 Additional comments (19)
README.md (2)
108-110
: Verify updated country-count figure
The sentence now claims support for 205 country codes. We’ve recently tripped on mis-counts in this exact spot, so please re-run the registry/count helper before merging to ensure “205” is really correct after adding Mongolia.
997-1002
: Row looks good – just check default-language consistency
<strong>mn</strong>
is correctly marked as the default language and the row is alphabetically inserted before Montenegro. No further action needed.holidays/locale/mn/LC_MESSAGES/MN.po (2)
17-28
: Minor metadata sanity-check
Confirm thatProject-Id-Version: Holidays 0.76
matches the upcoming tag; release scripts occasionally forget to bump this header.
30-132
: Catalog structure follows project conventions – nice work
Allmsgstr
entries are intentionally left empty (default-language file) and translator comments are concise. No issues spotted here.holidays/locale/en_US/LC_MESSAGES/MN.po (2)
45-48
: English wording consistent with existing East-Asian catalogs
Keeping “The Buddha's Birthday” (with the definite article) aligns with HK/TW/MO patterns, so no change needed.
61-64
: “Republic Day” vs “Republic Holiday” – intentional?
Two near-identical observances are listed. If the provider exposes both names, great; if not, consider dropping one to avoid string bloat.holidays/calendars/mongolian.py (2)
331-339
: Public helper methods are alphabetical – good catch
Buddha → Genghis Khan → Tsagaan Sar keeps reviewers sane.
325-330
:_get_holiday
returns “estimated=True” even when date is missing
Whenyear
is not in either dict, the method yields(None, True)
.
Call-sites expecting a validdate
may choke onNone
, and “estimated” becomes meaningless without a value. Consider signalling “not available” explicitly instead.- return date(year, *dt) if dt else None, year not in exact_dates + if not dt: + # Holiday not defined for the given year. + return None, False + return date(year, *dt), year not in exact_datesThis keeps the boolean strictly for “estimated vs exact” and lets callers treat
None
as “unsupported year”.⛔ Skipped due to learnings
Learnt from: KJhellico PR: vacanza/holidays#2437 File: holidays/calendars/tibetan.py:805-810 Timestamp: 2025-04-08T14:50:15.325Z Learning: The standard behavior for calendar date methods in the holidays library is to return `(None, True)` when a requested year is not found in the date dictionary, rather than raising an exception or requiring documentation for out-of-range years.
Learnt from: KJhellico PR: vacanza/holidays#2437 File: holidays/calendars/tibetan.py:805-810 Timestamp: 2025-04-08T14:50:15.325Z Learning: The standard behavior for calendar date methods in the holidays library is to return `(None, True)` when a requested year is not found in the date dictionary, rather than raising an exception or requiring documentation for out-of-range years.
holidays/groups/mongolian.py (1)
20-103
: Implementation looks solid for Mongolian calendar holidaysThe class properly extends EasternCalendarHolidays and provides methods for all the key Mongolian lunar calendar holidays. The static analysis errors about missing
_year
member are false positives since this attribute is provided by the parent class during holiday population.holidays/countries/mongolia.py (4)
1-1
: Module docstring not needed for country filesBased on retrieved learnings, country files in the holidays library consistently follow the pattern of NOT having module-level docstrings. The current implementation correctly follows this established convention.
44-102
: Public holidays implementation looks comprehensiveThe implementation incorporates feedback from extensive past discussions about holiday dates and year ranges. The conditional logic for holiday expansions (like Tsagaan Sar day 3 from 2014, Naadam from 2014, etc.) aligns with the historical changes documented in past reviews.
105-123
: Excellent documentation of unimplemented holidaysThe detailed note about Environmental Protection Days and National Literary, Cultural and Book Days provides clear context for why these holidays aren't implemented. Referencing the specific GitHub issue (#2672) is helpful for future maintainers.
197-202
: Alias classes correctly follow project conventionsThe alias classes appropriately omit docstrings, following the established project pattern for country code aliases.
tests/countries/test_mongolia.py (6)
1-18
: Imports and structure look good.The file follows the established project patterns for country test modules with proper imports and class structure.
20-31
: Class setup follows project patterns correctly.The test class structure, setup method, and basic validation tests are implemented properly according to the project standards.
33-183
: Holiday tests are well-structured and comprehensive.The individual holiday test methods follow the project's preferred patterns:
- Testing by individual holidays rather than years
- Proper use of explicit loops for multi-day consecutive holidays
- Correct handling of holiday category distinctions (PUBLIC vs WORKDAY)
- Appropriate testing of name changes over time
The structure aligns with established project conventions and learnings.
175-183
: New Harvest Days test coverage is reasonable.Testing the 46-day holiday period with start, middle, and end dates provides good coverage without being excessive. The approach balances thoroughness with maintainability.
184-284
: Remaining holiday tests and 2025 snapshot are well-implemented.The continued use of consistent testing patterns and the comprehensive 2025 test provide good validation coverage for the holiday implementation.
285-441
: Comprehensive localization tests with excellent coverage.The localization tests thoroughly validate both Mongolian and English holiday names, including:
- Full coverage of the 46-day New Harvest Days period
- Proper handling of multiple holidays on the same date
- Consistent formatting and translation accuracy
This provides robust validation of the localization implementation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🇲🇳
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What a great contribution 👍
Thank you @ankushhKapoor!
Proposed change
Added Mongolia Holidays.
Resolves #1227
Type of change
holidays
functionality in general)Checklist
make check
, all checks and tests are green