Improve month duration logic for non-gregorian calendars #146
Conversation
|
Warning Rate limit exceeded@rlskoeser has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 2 minutes and 13 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughAdds MIN_YEAR and MAX_YEAR constants to HebrewDateConverter. Reworks Undate possible_years to derive from initial_values["year"] with partial-digit handling and step inference. Updates duration month handling to compute possible months from initial_values and aggregate max days across representative years. Adds tests for non-Gregorian calendars covering possible_years and month durations. Changes
Sequence Diagram(s)sequenceDiagram
participant C as Caller
participant U as Undate.possible_years
Note over U: Input: initial_values["year"]
C->>U: request possible_years
alt year is int (fully known)
U-->>C: [year]
else year is str with some known digits
U->>U: earliest = replace_missing_with_0(year)
U->>U: latest = replace_missing_with_9(year)
U->>U: step = 10^(position_of_first_missing - 1)
U-->>C: range(earliest, latest, step) inclusive
else year missing or no known digits
U-->>C: raise ValueError("completely unknown year")
end
sequenceDiagram
participant C as Caller
participant U as Undate.duration (MONTH)
participant Cal as Calendar
Note over U: Inputs: calendar, initial_values.month, representative_years
C->>U: duration for MONTH
alt month is int
U->>U: possible_months = [month]
else month is partial string
U->>U: earliest = replace_missing_with_0
U->>U: latest = min(cal_max_month, replace_missing_with_9)
U->>U: possible_months = range(earliest, latest)
end
loop for m in possible_months, y in representative_years
U->>Cal: max_day(y, m)
Cal-->>U: day_count
U->>U: collect possible_max_days
end
U-->>C: deterministic day or UnInt(...) from possible_max_days
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
✨ 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. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #146 +/- ##
===========================================
+ Coverage 98.72% 98.78% +0.06%
===========================================
Files 38 38
Lines 1956 1973 +17
===========================================
+ Hits 1931 1949 +18
+ Misses 25 24 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/undate/converters/calendars/hebrew/converter.py (1)
24-28: Avoid float-to-int conversion for MAX_YEAR; tighten the commentCasting from a float literal is unnecessary and risks subtle rounding. Also, the comment uses 2.5^16 (caret) but the intended notation is 2.5e16.
- #: earliest possible year in the Hebrew calendar is year 1, it does not go negative - MIN_YEAR: int = 1 - # convertdate gives a month 34 for numpy max year 2.5^16, so scale it back a bit - MAX_YEAR = int(2.5e12) + #: earliest possible year in the Hebrew calendar is year 1; it does not go negative + MIN_YEAR: int = 1 + # convertdate returns invalid months for extremely large years (~2.5e16); use a safer bound + MAX_YEAR: int = 2_500_000_000_000tests/test_undate.py (1)
459-472: Solid coverage for non‑Gregorian month durations; consider adding “unknown month” casesThese checks validate fixed and variable month lengths across calendars. To prevent regressions with partially/fully unknown months, add cases like:
- Undate(month="XX", calendar="Hebrew") → UnInt(29, 30) or UnInt(29, 30, 29/30/…) depending on representative years
- Undate(month="0X", calendar="Islamic") → UnInt(29, 30)
I can add a parametrized test to cover "XX" and "0X" inputs for Hebrew/Islamic/Seleucid months.
src/undate/undate.py (1)
585-591: Construct UnDelta with sorted bounds to avoid order/arity pitfallspossible_max_days is a set; argument order to UnDelta is non-deterministic. Prefer min/max.
- if len(possible_max_days) > 1: - return UnDelta(*possible_max_days) + if len(possible_max_days) > 1: + lo, hi = min(possible_max_days), max(possible_max_days) + return UnDelta(lo, hi) return Timedelta(possible_max_days.pop())
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
src/undate/converters/calendars/hebrew/converter.py(1 hunks)src/undate/undate.py(2 hunks)tests/test_undate.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/undate/undate.py (5)
src/undate/date.py (2)
year(230-231)month(234-238)src/undate/converters/calendars/hebrew/converter.py (3)
max_month(41-44)representative_years(64-88)max_day(55-58)src/undate/converters/calendars/islamic/converter.py (3)
max_month(45-47)representative_years(49-73)max_day(37-39)src/undate/converters/base.py (3)
max_month(165-167)representative_years(194-200)max_day(177-179)src/undate/converters/calendars/gregorian.py (3)
max_month(25-27)representative_years(48-73)max_day(29-46)
tests/test_undate.py (2)
src/undate/undate.py (3)
possible_years(475-506)duration(526-593)month(446-456)src/undate/date.py (3)
days(29-31)month(234-238)UnInt(35-121)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (python)
🔇 Additional comments (1)
tests/test_undate.py (1)
413-415: LGTM: preserves original calendar for fully known non‑Gregorian yearsThis assertion guards against unintended Gregorian normalization in possible_years. Good coverage.
1 participant
this is to address bug reported in #140 - durations for non-Gregorian months were being calculated as uncertain when they are in fact known
Summary by CodeRabbit
New Features
Bug Fixes
Tests