Add duration logic for uncertain years #135
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #135 +/- ##
===========================================
- Coverage 98.74% 98.72% -0.02%
===========================================
Files 37 38 +1
Lines 1588 1956 +368
===========================================
+ Hits 1568 1931 +363
- Misses 20 25 +5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
WalkthroughThe base calendar converter was extended with Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Undate
participant CalendarConverter
participant HebrewDateConverter
User->>Undate: duration()
alt Year is partially/fully unknown
Undate->>CalendarConverter: representative_years(possible_years)
loop for each representative year
Undate->>CalendarConverter: days_in_year(year)
CalendarConverter-->>Undate: days in year
end
Undate-->>User: UnDelta (uncertain duration) or Timedelta
else Year is fully known
Undate->>Undate: Calculate earliest/latest dates
Undate-->>User: Timedelta (definite duration)
end
Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes found. Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
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
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
tests/test_undate.py (1)
449-450: Address the commented assertion for completely unknown years.The comment indicates that completely unknown years (
XXXX) are not yet supported for non-Gregorian calendars. This limitation should either be:
- Implemented to support all calendars consistently
- Documented as a known limitation
- Tested to ensure it raises an appropriate error
Would you like me to help implement support for completely unknown years in non-Gregorian calendars or add proper error handling and tests for this case?
src/undate/undate.py (4)
129-129: Good catch on calendar-specific year bounds.The TODO comment correctly identifies that min/max allowable years should be calendar-specific, as different calendars have different valid year ranges.
Would you like me to help implement calendar-specific year bounds as a follow-up improvement?
516-521: Apply static analysis suggestion for cleaner code.- try: - possible_max_days = set( - [self.calendar_converter.days_in_year(y) for y in possible_years] - ) - except NotImplementedError: - pass + import contextlib + with contextlib.suppress(NotImplementedError): + possible_max_days = {self.calendar_converter.days_in_year(y) for y in possible_years}This also converts the list comprehension to a set comprehension as suggested by Pylint.
545-548: Simplify conditional logic by removing unnecessary else.- if possible_max_days and len(possible_max_days) > 1: - return UnDelta(*possible_max_days) - else: - return Timedelta(possible_max_days.pop()) + if possible_max_days and len(possible_max_days) > 1: + return UnDelta(*possible_max_days) + + return Timedelta(possible_max_days.pop())
477-549: Consider refactoring the duration method for better maintainability.The duration calculation logic has become quite complex with the addition of uncertain year handling. Consider breaking it into smaller, focused helper methods:
_get_possible_years()- to determine the list of possible years_calculate_month_duration()- for month precision calculations_calculate_year_duration()- for year precision calculationsThis would make the code more readable and testable.
Would you like me to help refactor this method into smaller, more maintainable components?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/undate/converters/base.py(1 hunks)src/undate/converters/calendars/hebrew/converter.py(1 hunks)src/undate/undate.py(4 hunks)tests/test_undate.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/undate/converters/calendars/hebrew/converter.py (1)
src/undate/converters/base.py (1)
days_in_year(165-167)
src/undate/converters/base.py (1)
src/undate/converters/calendars/hebrew/converter.py (1)
days_in_year(50-52)
🪛 Ruff (0.11.9)
src/undate/undate.py
516-521: Use contextlib.suppress(NotImplementedError) instead of try-except-pass
Replace with contextlib.suppress(NotImplementedError)
(SIM105)
🪛 Pylint (3.3.7)
src/undate/undate.py
[refactor] 517-519: Consider using a set comprehension
(R1718)
[refactor] 545-548: Unnecessary "else" after "return", remove the "else" and de-indent the code inside it
(R1705)
🔇 Additional comments (3)
src/undate/converters/base.py (1)
165-167: LGTM! Good addition to the base calendar converter interface.The abstract method follows the established pattern and provides a clean interface for calendar-specific year duration calculations.
src/undate/converters/calendars/hebrew/converter.py (1)
50-52: Clean implementation of thedays_in_yearmethod.The implementation correctly uses the convertdate library's
hebrew.year_daysfunction and ensures the return type is an integer.src/undate/undate.py (1)
516-521: Verify that all calendar converters implementdays_in_year.The code has a fallback for
NotImplementedError, suggesting some calendars may not implement this method yet. Let's verify which calendars are missing this implementation.#!/bin/bash # Check which calendar converters have implemented days_in_year method # Find all calendar converter files echo "=== Checking calendar converter implementations ===" fd -e py . src/undate/converters/calendars/ --exec grep -l "class.*Converter.*BaseCalendarConverter" {} \; | while read -r file; do echo "Checking $file:" if grep -q "def days_in_year" "$file"; then echo " ✓ Implements days_in_year" else echo " ✗ Missing days_in_year implementation" fi done
src/undate/undate.py
Outdated
| self.calendar_converter.LEAP_YEAR, | ||
| self.calendar_converter.NON_LEAP_YEAR, | ||
| ] |
There was a problem hiding this comment.
Fix undefined attributes LEAP_YEAR and NON_LEAP_YEAR.
The code references self.calendar_converter.LEAP_YEAR and self.calendar_converter.NON_LEAP_YEAR, but these attributes are not defined in the BaseCalendarConverter class or its implementations.
You need to either:
- Define these constants in the base converter class
- Use a different approach to get representative leap and non-leap years for each calendar
For a quick fix, you could use hardcoded Gregorian examples:
- possible_years = [
- self.calendar_converter.LEAP_YEAR,
- self.calendar_converter.NON_LEAP_YEAR,
- ]
+ # TODO: make these calendar-specific
+ # For now, use Gregorian examples
+ possible_years = [2024, 2023] # leap year, non-leap year📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| self.calendar_converter.LEAP_YEAR, | |
| self.calendar_converter.NON_LEAP_YEAR, | |
| ] | |
| # TODO: make these calendar-specific | |
| # For now, use Gregorian examples | |
| possible_years = [2024, 2023] # leap year, non-leap year |
🤖 Prompt for AI Agents
In src/undate/undate.py around lines 492 to 494, the attributes LEAP_YEAR and
NON_LEAP_YEAR are referenced on self.calendar_converter but are not defined in
the BaseCalendarConverter class or its subclasses. To fix this, either define
these constants in the base converter class or replace these references with
hardcoded representative leap and non-leap year values (e.g., 2020 for leap year
and 2019 for non-leap year) to ensure the code has valid year values to work
with.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/undate/undate.py (2)
498-514: Verify constants are defined before accessing them.The implementation logic is good, but based on past review comments, there may be issues with accessing
LEAP_YEARandNON_LEAP_YEARconstants. The constants should be properly defined in the base class as shown in the base.py changes.
530-560: Add safety check for empty set and improve code structure.The enhanced duration logic is excellent and properly handles representative years for accurate calculations. However, there's a potential issue with calling
pop()on an empty set.possible_max_days = { self.calendar_converter.days_in_year(y) for y in self.representative_years } # if there is more than one possible value for number of days - # due to range including lear year / non-leap year, return an uncertain delta + # due to range including leap year / non-leap year, return an uncertain delta if possible_max_days: if len(possible_max_days) > 1: return UnDelta(*possible_max_days) - return Timedelta(possible_max_days.pop()) + return Timedelta(possible_max_days.pop()) # otherwise, subtract earliest from latest and add a day to include start day in the count return self.latest - self.earliest + ONE_DAYNote: The current code structure should work correctly since
possible_max_daysis only checked when it's truthy, but consider adding an explicit empty check for clarity.
🧹 Nitpick comments (1)
src/undate/converters/base.py (1)
184-190: ```shell
#!/bin/bashSearch for BaseCalendarConverter instantiation or direct usage in tests
grep -R "BaseCalendarConverter" -n tests/
Search for NotImplementedError in tests
grep -R "NotImplementedError" -n tests/
</blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used: CodeRabbit UI** **Review profile: CHILL** **Plan: Pro** <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 5b5746d70337b5a62aa26c26f3fe934ee50c3b43 and b436f33a7b4784e41ddd3389ed04b21b48fe108f. </details> <details> <summary>📒 Files selected for processing (9)</summary> * `src/undate/converters/base.py` (3 hunks) * `src/undate/converters/calendars/gregorian.py` (2 hunks) * `src/undate/converters/calendars/hebrew/converter.py` (2 hunks) * `src/undate/converters/calendars/islamic/converter.py` (2 hunks) * `src/undate/converters/calendars/seleucid.py` (1 hunks) * `src/undate/undate.py` (7 hunks) * `tests/test_converters/test_calendars/test_hebrew/test_hebrew_converter.py` (1 hunks) * `tests/test_converters/test_calendars/test_islamic/test_islamic_converter.py` (1 hunks) * `tests/test_undate.py` (2 hunks) </details> <details> <summary>🚧 Files skipped from review as they are similar to previous changes (1)</summary> * src/undate/converters/calendars/hebrew/converter.py </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>🧬 Code Graph Analysis (6)</summary> <details> <summary>tests/test_converters/test_calendars/test_hebrew/test_hebrew_converter.py (3)</summary><blockquote> <details> <summary>src/undate/converters/calendars/hebrew/converter.py (1)</summary> * `representative_years` (59-83) </details> <details> <summary>src/undate/converters/base.py (1)</summary> * `representative_years` (184-190) </details> <details> <summary>src/undate/undate.py (1)</summary> * `representative_years` (499-514) </details> </blockquote></details> <details> <summary>src/undate/converters/calendars/seleucid.py (1)</summary><blockquote> <details> <summary>src/undate/converters/base.py (1)</summary> * `days_in_year` (171-182) </details> </blockquote></details> <details> <summary>tests/test_converters/test_calendars/test_islamic/test_islamic_converter.py (4)</summary><blockquote> <details> <summary>tests/test_undate.py (1)</summary> * `test_representative_years` (410-415) </details> <details> <summary>src/undate/converters/calendars/islamic/converter.py (1)</summary> * `representative_years` (44-68) </details> <details> <summary>src/undate/converters/base.py (1)</summary> * `representative_years` (184-190) </details> <details> <summary>src/undate/undate.py (1)</summary> * `representative_years` (499-514) </details> </blockquote></details> <details> <summary>src/undate/converters/calendars/islamic/converter.py (2)</summary><blockquote> <details> <summary>src/undate/converters/base.py (1)</summary> * `representative_years` (184-190) </details> <details> <summary>src/undate/undate.py (2)</summary> * `representative_years` (499-514) * `year` (430-438) </details> </blockquote></details> <details> <summary>src/undate/converters/calendars/gregorian.py (3)</summary><blockquote> <details> <summary>src/undate/converters/base.py (1)</summary> * `representative_years` (184-190) </details> <details> <summary>src/undate/converters/calendars/hebrew/converter.py (1)</summary> * `representative_years` (59-83) </details> <details> <summary>src/undate/undate.py (2)</summary> * `representative_years` (499-514) * `year` (430-438) </details> </blockquote></details> <details> <summary>tests/test_undate.py (5)</summary><blockquote> <details> <summary>src/undate/undate.py (3)</summary> * `possible_years` (470-496) * `representative_years` (499-514) * `duration` (516-563) </details> <details> <summary>src/undate/converters/calendars/islamic/converter.py (1)</summary> * `representative_years` (44-68) </details> <details> <summary>src/undate/converters/base.py (1)</summary> * `representative_years` (184-190) </details> <details> <summary>src/undate/converters/calendars/hebrew/converter.py (1)</summary> * `representative_years` (59-83) </details> <details> <summary>src/undate/converters/calendars/gregorian.py (1)</summary> * `representative_years` (48-73) </details> </blockquote></details> </details><details> <summary>🪛 GitHub Check: codecov/patch</summary> <details> <summary>src/undate/converters/calendars/seleucid.py</summary> [warning] 28-28: src/undate/converters/calendars/seleucid.py#L28 Added line #L28 was not covered by tests </details> <details> <summary>src/undate/converters/calendars/gregorian.py</summary> [warning] 55-55: src/undate/converters/calendars/gregorian.py#L55 Added line #L55 was not covered by tests </details> <details> <summary>src/undate/undate.py</summary> [warning] 40-40: src/undate/undate.py#L40 Added line #L40 was not covered by tests --- [warning] 509-509: src/undate/undate.py#L509 Added line #L509 was not covered by tests </details> <details> <summary>src/undate/converters/base.py</summary> [warning] 190-190: src/undate/converters/base.py#L190 Added line #L190 was not covered by tests </details> </details> <details> <summary>🪛 Pylint (3.3.7)</summary> <details> <summary>src/undate/undate.py</summary> [refactor] 477-494: Unnecessary "else" after "return", remove the "else" and de-indent the code inside it (R1705) </details> </details> <details> <summary>🪛 Ruff (0.11.9)</summary> <details> <summary>tests/test_undate.py</summary> 408-408: Found useless attribute access. Either assign it to a variable or remove it. (B018) </details> </details> </details> <details> <summary>🔇 Additional comments (13)</summary><blockquote> <details> <summary>src/undate/converters/calendars/seleucid.py (1)</summary> `26-28`: **Implementation looks good, but needs test coverage.** The `days_in_year` method correctly follows the established pattern of applying `SELEUCID_OFFSET` and delegating to the superclass, consistent with the existing `to_gregorian` method. However, static analysis indicates this method lacks test coverage. Please add tests to verify the method works correctly: ```shell #!/bin/bash # Description: Check if there are existing tests for SeleucidDateConverter that could be extended # Search for existing SeleucidDateConverter tests rg -A 10 "SeleucidDateConverter" tests/tests/test_converters/test_calendars/test_hebrew/test_hebrew_converter.py (1)
157-182: Comprehensive test coverage for representative years functionality.The test method thoroughly covers all the key scenarios:
- Single year handling (no filtering)
- Multiple years with different day counts (filtering behavior)
- Default behavior when no years are provided
- Edge case with empty list
The test data is well-chosen with specific Hebrew calendar years that have known different day counts (353, 355, 384 days), which properly validates the filtering logic.
tests/test_converters/test_calendars/test_islamic/test_islamic_converter.py (1)
156-174: Well-structured test for Islamic calendar representative years.The test method effectively validates the Islamic calendar's representative years functionality:
- Correctly tests leap year detection (1458 is leap, 1457/1459 are not)
- Validates the filtering logic returns first occurrence of each year type
- Tests default fallback to predefined constants
- Maintains consistency with other calendar converter tests
src/undate/converters/calendars/gregorian.py (2)
1-1: Good choice of import for leap year detection.Adding
isleapimport is the correct approach for Gregorian calendar leap year detection, ensuring accuracy and consistency with Python's standard library.
48-73: Solid implementation, but needs test coverage for default case.The
representative_yearsmethod is well-implemented:
- Uses Python's built-in
isleapfor accurate leap year detection- Implements early exit optimization
- Follows consistent pattern with other calendar converters
However, static analysis indicates line 55 lacks test coverage. This line handles the default case when no years are provided. Please add a test case to cover this scenario:
#!/bin/bash # Description: Check existing Gregorian converter tests to see if representative_years is tested # Search for existing GregorianDateConverter tests rg -A 20 "def test.*representative" tests/test_converters/test_calendars/src/undate/converters/calendars/islamic/converter.py (2)
24-27: Appropriate constants for Islamic calendar.The choice of Islamic calendar years 1457 (non-leap) and 1458 (leap) as representative constants is well-suited and aligns with the test data used in the corresponding test file.
44-68: Correct implementation using Islamic calendar leap year logic.The
representative_yearsmethod is properly implemented:
- Uses
islamic.leap()for accurate Islamic calendar leap year detection- Follows the established pattern from other calendar converters
- Implements efficient early exit when both year types are found
- Handles edge cases (empty/None input) appropriately
src/undate/converters/base.py (2)
63-66: LGTM! Provisional constants provide a clear interface.The provisional constants establish the expected interface for subclasses while making it clear they need to be overridden with meaningful values.
171-183: Solid default implementation for days_in_year.The implementation correctly leverages existing calendar conversion methods to calculate year length. The logic is sound: convert year boundaries to Gregorian dates and calculate the difference.
tests/test_undate.py (2)
410-415: LGTM! Comprehensive test coverage for representative years.The test correctly validates both the simple case (single known year) and the complex case (uncertain year range with leap/non-leap year selection).
462-474: Excellent test coverage for duration calculations across calendars.The tests thoroughly validate duration calculations for partially known years, including:
- Different calendar systems (Gregorian, Islamic, Hebrew)
- Various uncertainty patterns (190X, XXXX, 19X1)
- Leap year ambiguity handling
The comment on line 470-471 appropriately documents a known limitation.
src/undate/undate.py (2)
22-22: LGTM! Import addition supports enhanced type checking.Adding the
BaseCalendarConverterimport enables the type validation improvements in theget_convertermethod.
35-43: ```shell
#!/bin/bashSearch for existing tests invoking get_converter to check coverage of invalid converter branch
rg -n "get_converter" tests/ -A3 -B3 || echo "No tests invoking get_converter found."
</details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
tests/test_undate.py (1)
396-408: Fix unused property access issue.The test accesses
possible_yearsproperty without using the result, which static analysis flagged as useless.- with pytest.raises( - ValueError, match="cannot be returned for completely unknown year" - ): - Undate("XXXX").possible_years + with pytest.raises( + ValueError, match="cannot be returned for completely unknown year" + ): + _ = Undate("XXXX").possible_yearssrc/undate/undate.py (1)
472-499: Remove unnecessary else clause after return.The code structure can be improved by removing the unnecessary else clause after the return statement, as flagged by static analysis.
step = 10 ** (missing_digit_place - 1) return range(earliest_year, latest_year + 1, step) - else: # year is fully unknown - # returning range from min year to max year is not useful in any scenario! - raise ValueError( - "Possible years cannot be returned for completely unknown year" - ) - - return [] # shouldn't get here, but mypy complains + + # year is fully unknown + # returning range from min year to max year is not useful in any scenario! + raise ValueError( + "Possible years cannot be returned for completely unknown year" + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/undate/converters/base.py(4 hunks)src/undate/converters/calendars/islamic/converter.py(2 hunks)src/undate/undate.py(8 hunks)tests/test_undate.py(2 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
src/undate/undate.py
[warning] 40-40: src/undate/undate.py#L40
Added line #L40 was not covered by tests
[warning] 512-512: src/undate/undate.py#L512
Added line #L512 was not covered by tests
src/undate/converters/base.py
[warning] 200-200: src/undate/converters/base.py#L200
Added line #L200 was not covered by tests
🪛 Pylint (3.3.7)
src/undate/undate.py
[refactor] 480-497: Unnecessary "else" after "return", remove the "else" and de-indent the code inside it
(R1705)
🪛 Ruff (0.11.9)
tests/test_undate.py
408-408: Found useless attribute access. Either assign it to a variable or remove it.
(B018)
🔇 Additional comments (13)
src/undate/converters/calendars/islamic/converter.py (2)
24-32: LGTM! Well-defined calendar constants.The constants for known leap/non-leap years and calendar bounds are appropriately defined for the Islamic calendar.
49-73: LGTM! Solid implementation of representative years logic.The method correctly identifies leap and non-leap years using
islamic.leap()and efficiently stops once both types are found. The fallback to predefined constants when no input is provided is appropriate.tests/test_undate.py (2)
410-416: LGTM! Comprehensive test coverage for representative years.The test correctly validates both single year returns and representative year selection for uncertain years.
462-477: LGTM! Thorough testing of duration logic across calendars.The tests comprehensively cover duration calculations for partially known years across Gregorian, Islamic, and Hebrew calendars, validating both certain and uncertain duration scenarios.
src/undate/converters/base.py (4)
51-52: LGTM! Import required for new functionality.The Date import is needed for the new
days_in_yearmethod implementation.
63-65: LGTM! Provisional constants for base functionality.These constants provide a foundation that can be overridden by specific calendar implementations.
151-159: LGTM! Well-structured calendar converter constants.The class variables for representative years and min/max year bounds provide a clean interface for calendar-specific implementations.
181-200: LGTM! Robust implementation of year duration calculation.The
days_in_yearmethod correctly calculates the number of days by converting the first and last day of the year to Gregorian dates and computing the difference. The logic accounts for different calendar systems and the +1 adjustment properly includes both endpoints.src/undate/undate.py (5)
22-22: LGTM! Import required for enhanced validation.The BaseCalendarConverter import is needed for the type checking in
get_converter.
39-42: LGTM! Enhanced converter validation.The subclass check ensures only proper calendar converters are returned, improving type safety.
131-135: LGTM! Improved fallback logic for calendar bounds.Using calendar-specific min/max years when available provides more accurate bounds for unknown years.
501-518: LGTM! Well-structured representative years property.The implementation correctly delegates to calendar converter when available and falls back to predefined leap/non-leap years for completely unknown years. The exception handling appropriately manages converters that don't implement the method.
533-566: LGTM! Enhanced duration calculation with uncertainty handling.The refactored duration method effectively handles uncertain years by using representative years to calculate possible durations. The logic correctly returns
UnDeltafor ambiguous cases andTimedeltafor certain durations, with appropriate fallback to the original calculation method.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
tests/test_undate.py (1)
398-410: Fix the useless property access flagged by static analysis.The test logic is comprehensive, but line 410 accesses the
possible_yearsproperty without using the result, which static analysis flagged as useless.- with pytest.raises( - ValueError, match="cannot be returned for completely unknown year" - ): - Undate("XXXX").possible_years + with pytest.raises( + ValueError, match="cannot be returned for completely unknown year" + ): + _ = Undate("XXXX").possible_yearssrc/undate/undate.py (2)
200-202: Remove debug print statement from production code.Debug print statements should not be included in production code.
- print( - f"initializing latest, year={max_year} month={latest_month} day={max_day}" - )
477-502: Remove unnecessary else clause after return statement.The implementation logic is sound, but the code structure can be improved by removing the unnecessary else clause.
- else: # year is fully unknown - # returning range from min year to max year is not useful in any scenario! - raise ValueError( - "Possible years cannot be returned for completely unknown year" - ) + + # year is fully unknown + # returning range from min year to max year is not useful in any scenario! + raise ValueError( + "Possible years cannot be returned for completely unknown year" + )Also remove the unreachable return statement at the end.
🧹 Nitpick comments (1)
src/undate/undate.py (1)
38-47: Improve exception chaining for better error traceability.The validation logic is sound, but the exception handling should use proper chaining as flagged by static analysis.
- except KeyError: - raise ValueError(f"Unknown calendar '{calendar}'") + except KeyError as err: + raise ValueError(f"Unknown calendar '{calendar}'") from err
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/undate/undate.py(8 hunks)tests/test_converters/test_calendars/test_hebrew/test_hebrew_converter.py(1 hunks)tests/test_converters/test_calendars/test_seleucid.py(1 hunks)tests/test_undate.py(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_converters/test_calendars/test_hebrew/test_hebrew_converter.py
🧰 Additional context used
🧬 Code Graph Analysis (2)
tests/test_converters/test_calendars/test_seleucid.py (2)
tests/test_converters/test_calendars/test_hebrew/test_hebrew_converter.py (1)
test_days_in_year(157-162)src/undate/converters/calendars/seleucid.py (1)
SeleucidDateConverter(5-28)
src/undate/undate.py (6)
src/undate/converters/base.py (6)
BaseCalendarConverter(144-206)BaseDateConverter(56-141)available_converters(116-120)max_day(177-179)representative_years(194-200)days_in_year(181-192)src/undate/date.py (5)
year(230-231)DatePrecision(283-308)month(234-238)UnDelta(125-169)Timedelta(11-31)src/undate/converters/calendars/islamic/converter.py (2)
max_day(37-39)representative_years(49-73)src/undate/converters/calendars/hebrew/converter.py (3)
max_day(50-53)representative_years(59-83)days_in_year(55-57)src/undate/converters/calendars/gregorian.py (2)
max_day(29-46)representative_years(48-73)src/undate/converters/calendars/seleucid.py (1)
days_in_year(26-28)
🪛 Ruff (0.11.9)
tests/test_undate.py
410-410: Found useless attribute access. Either assign it to a variable or remove it.
(B018)
src/undate/undate.py
43-43: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🪛 Pylint (3.3.7)
tests/test_undate.py
[refactor] 551-551: Too few public methods (0/2)
(R0903)
[refactor] 562-562: Too few public methods (0/2)
(R0903)
src/undate/undate.py
[refactor] 485-502: Unnecessary "else" after "return", remove the "else" and de-indent the code inside it
(R1705)
🪛 GitHub Check: codecov/patch
src/undate/undate.py
[warning] 517-517: src/undate/undate.py#L517
Added line #L517 was not covered by tests
🔇 Additional comments (11)
tests/test_converters/test_calendars/test_seleucid.py (1)
56-60: LGTM! Well-structured test for the new days_in_year method.The test follows the same pattern as other calendar converter tests and properly validates the
days_in_yearmethod for the Seleucid calendar. The expected values (354, 385, 355 days) are consistent with Hebrew calendar year lengths, which is appropriate since Seleucid inherits from Hebrew with an offset.tests/test_undate.py (4)
2-2: LGTM! Appropriate imports for the new test functionality.The new imports support the additional test methods for calendar converter validation and the new Undate properties.
Also applies to: 7-8
412-417: LGTM! Comprehensive test for representative years functionality.The test properly validates both known years (returning single year) and uncertain years (returning representative leap/non-leap years).
464-478: LGTM! Thorough test coverage for partially known year durations across calendars.The test effectively validates the new duration logic for uncertain years, including:
- Gregorian calendar leap year variations
- Islamic and Hebrew calendar year length variations
- Edge cases like years that don't include leap years
- Fully unknown years across different calendars
This provides good coverage of the enhanced duration calculation functionality.
550-571: LGTM! Important validation for calendar converter error handling.The test classes with few public methods are acceptable for testing purposes, despite the pylint warnings. These tests ensure proper error handling when requesting unknown calendars or non-calendar converters, which is crucial for robustness.
src/undate/undate.py (6)
22-22: LGTM! Necessary import for enhanced calendar converter validation.The import of
BaseCalendarConverterandBaseDateConvertersupports the improved type checking in theget_convertermethod.
136-140: LGTM! Improved fallback logic for calendar-specific year bounds.This enhancement properly uses calendar-specific min/max years when available, falling back to global allowable years. This provides better support for calendars with different valid year ranges.
179-179: LGTM! Better fallback year handling for day calculations.Using
max_yearinstead ofNoneensures a valid year is always available for day calculations, preventing potential errors.
433-435: LGTM! Good documentation of the partially known logic.The TODO comment appropriately questions the current behavior and preserves existing functionality while documenting the ambiguity.
506-522: LGTM! Well-designed representative years property with proper fallback.The implementation correctly handles both calendar-specific representative years and provides a sensible fallback using
LEAP_YEARandNON_LEAP_YEARfrom the calendar converter. The exception handling for converters withoutrepresentative_yearssupport is appropriate.
538-568: LGTM! Significant improvement to duration calculations for uncertain years.The enhanced duration method properly handles uncertain years by:
- Using representative years for more accurate calculations
- Leveraging calendar-specific
days_in_yearmethods- Returning
UnDeltafor uncertain durations andTimedeltafor certain onesThe logic correctly handles both month and year precision cases. The set-based approach for collecting possible day counts is efficient and handles the uncertainty appropriately.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/test_converters/test_calendars/test_gregorian.py (2)
16-16: Consider implementing the max_day test.The TODO comment indicates missing test coverage for the
max_daymethod, which is implemented in the converter.Would you like me to generate a test for the
max_daymethod to improve test coverage?
18-35: Fix typo in comment and verify leap year logic.The test logic for
representative_yearsis well-structured and covers the main scenarios correctly. However, there's a minor typo in the comment.Apply this diff to fix the typo:
- # if no years are provided, returns a known leap year and non-leap years + # if no years are provided, returns a known leap year and non-leap yearThe test cases correctly verify:
- Single year returns unchanged
- Multiple non-leap years return only the first
- Mixed leap/non-leap years return first of each type in input order
- Empty/null inputs return predefined LEAP_YEAR and NON_LEAP_YEAR constants
All leap year calculations are accurate (2025, 2026, 2029 are non-leap; 2028 is leap).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/test_converters/test_calendars/test_gregorian.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
tests/test_converters/test_calendars/test_gregorian.py (1)
src/undate/converters/calendars/gregorian.py (1)
GregorianDateConverter(6-81)
🔇 Additional comments (2)
tests/test_converters/test_calendars/test_gregorian.py (2)
1-8: LGTM! Clean test setup and correct no-op verification.The import and
test_to_gregorianmethod correctly verify that the Gregorian converter returns dates unchanged, which is the expected behavior.
10-14: LGTM! Correct boundary tests for Gregorian calendar.Both
test_min_monthandtest_max_monthcorrectly verify the expected values (1 and 12) for the Gregorian calendar.
resolves #130
Summary by CodeRabbit
New Features
Bug Fixes
Tests