refactor: improve leap year calculation for issue #48 #49
Conversation
Replace the old leap year calculation method with a more accurate algorithm based on ICU4X's Persian calendar implementation. The new method uses a 33-year cycle rule with specific corrections for known exceptions.
WalkthroughThis pull request introduces minor updates across multiple files in the persiantools project. The changes include version bumps for pre-commit hooks and development packages, an update to the library's version and copyright year, and significant improvements to the Jalali calendar's leap year calculation logic. The modifications enhance the library's accuracy and test coverage, with a focus on precise date conversion and leap year detection. Changes
Sequence DiagramsequenceDiagram
participant JalaliDate
participant LeapYearCalculator
JalaliDate->>LeapYearCalculator: Check if year is leap
LeapYearCalculator-->>JalaliDate: Validate against NON_LEAP_CORRECTION
LeapYearCalculator-->>JalaliDate: Apply 33-year leap year rule
JalaliDate->>JalaliDate: Determine final leap year status
Poem
Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/test_jalalidate.py (1)
206-304: Consider restructuring the leap year tests for better maintainability.The test cases provide excellent coverage of the leap year calculation logic, including:
- Multiple 33-year cycles
- Edge cases and known exceptions
- Historical dates
- Known non-leap years
However, consider improving the test structure:
@pytest.mark.parametrize("test_id,year,expected", [ # First 33-year cycle ("first_cycle_1", 1210, True), ("first_cycle_2", 1214, True), # ... more cases ... # Second 33-year cycle ("second_cycle_1", 1247, True), # ... more cases ... # Known non-leap years ("non_leap_1", 1206, False), # ... more cases ... ]) def test_is_leap(test_id, year, expected): assert JalaliDate.is_leap(year) == expectedThis structure would:
- Make it easier to add/remove test cases
- Provide better error messages
- Allow running specific test cases using test IDs
- Improve test report readability
persiantools/jdatetime.py (1)
385-409: Consider enhancing the documentation of the 33-year rule.The implementation is excellent:
- Efficient handling of special cases
- Clear control flow with early returns
- Proper validation of input range
- Correct use of the new constants
However, the documentation could be enhanced to explain the mathematical basis of the formula
(25 * year + 11) % 33 < 8.Add this explanation to the docstring:
""" Determines if a given Persian year is a leap year using the 33-year rule, with corrections for specific years that deviate from the rule. The 33-year rule is based on the fact that the Persian calendar has 8 leap years in every 33-year cycle. The formula (25 * year + 11) % 33 < 8 determines if a year is a leap year by: 1. Multiplying the year by 25 and adding 11 to distribute leap years 2. Taking modulo 33 to work within the cycle 3. Comparing with 8 (the number of leap years in the cycle) This function is based on the Rust implementation from the ICU4X project: https://github.com/unicode-org/icu4x/blob/main/utils/calendrical_calculations/src/persian.rs Args: year (int): The Persian year to check. Returns: bool: True if the year is a leap year, False otherwise. """
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Pipfile.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
.pre-commit-config.yaml(2 hunks)Pipfile(1 hunks)persiantools/__init__.py(1 hunks)persiantools/jdatetime.py(2 hunks)tests/test_jalalidate.py(4 hunks)
✅ Files skipped from review due to trivial changes (2)
- persiantools/init.py
- .pre-commit-config.yaml
🔇 Additional comments (5)
Pipfile (1)
10-14: Verify package versions exist and check for security advisories.The specified versions are from 2025. Let's verify their existence and security status.
✅ Verification successful
All specified package versions exist and are secure
The package versions are valid on PyPI and have no known security vulnerabilities. These changes appear to be routine development dependency updates.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify package versions exist and check for security advisories # Function to check PyPI for package version check_package() { local package=$1 local version=$2 echo "Checking $package==$version..." curl -s "https://pypi.org/pypi/$package/$version/json" | jq -r '.info.version // "Not found"' } # Check each package version check_package "coverage" "7.6.10" check_package "pytest" "8.3.4" check_package "tomli" "2.2.1" # Check for security advisories for package in "coverage" "pytest" "tomli"; do echo "Checking security advisories for $package..." gh api graphql -f query='{ securityVulnerabilities(first: 5, ecosystem: PIP, package: "'$package'") { nodes { advisory { summary severity publishedAt } vulnerableVersionRange firstPatchedVersion { identifier } } } }' doneLength of output: 2528
tests/test_jalalidate.py (2)
15-17: LGTM! Good test coverage for historical dates.The added test cases for years 1210-1211 help validate the conversion accuracy for historical dates, which is particularly important for the new leap year calculation algorithm.
74-76: LGTM! Good bidirectional test coverage.The added test cases ensure accurate bidirectional conversion between Gregorian and Jalali calendars for historical dates, which helps validate the robustness of the conversion logic.
persiantools/jdatetime.py (2)
134-218: LGTM! Well-structured constants for leap year calculation.The new constants are well-designed:
NON_LEAP_CORRECTIONprovides an ordered list of exception yearsNON_LEAP_CORRECTION_SETenables O(1) lookups using a frozensetMIN_NON_LEAP_CORRECTIONcaches the first exception year for optimization
221-221: LGTM! Clear type hints.The added type hints improve code clarity and enable better IDE support.
Replace the old leap year calculation method with a more accurate algorithm based on ICU4X's Persian calendar implementation. The new method uses a 33-year cycle rule with specific corrections for known exceptions.
Summary by CodeRabbit
Version Updates
pyupgradeandbanditcoverage,pytest,tomli)persiantoolslibrary version from 5.1.0 to 5.1.1Improvements
Testing