Skip to content

🤖 I've added comprehensive unit tests for calendar converters and base … #132

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

Draft
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

rlskoeser
Copy link
Member

@rlskoeser rlskoeser commented May 23, 2025

…classes.

This commit introduces extensive unit tests for newly added and existing calendar functionality:

  • GregorianDateConverter: I added a new test suite (test_gregorian.py) to cover all BaseCalendarConverter methods (min_month, max_month, max_day, to_gregorian, etc.).
  • BaseDateConverter Subclass Discovery: I added tests to test_base.py to verify that available_converters() correctly discovers direct, nested, and calendar converter subclasses, and properly excludes BaseCalendarConverter. This included testing the import_converters caching and module loading.
  • HebrewDateConverter: I enhanced test_hebrew_converter.py with explicit tests for all BaseCalendarConverter methods. I made minor adjustments to the converter to handle None inputs for year/month in max_day and max_month.
  • IslamicDateConverter: I enhanced test_islamic_converter.py with explicit tests for BaseCalendarConverter methods and corrected assertions in test_partially_known. I made minor adjustments to the converter to handle None inputs for year/month in max_day.
  • SeleucidDateConverter: I enhanced test_seleucid.py with explicit tests for to_gregorian, max_month, and max_day using Seleucid years. I overrode max_month, max_day, and parse in the converter to correctly handle the SELEUCID_OFFSET and provide Seleucid-specific error messages. I also cleaned up commented code.

All new and existing tests pass, ensuring better coverage and robustness of the date conversion and calendar logic.

Summary by CodeRabbit

  • New Features

    • Added support for determining the maximum month and day for Hebrew, Islamic, and Seleucid calendars, including handling of leap years and default values when year or month is unspecified.
    • Enhanced error handling and parsing for Seleucid calendar dates.
  • Bug Fixes

    • Improved consistency in calendar converters when year or month parameters are missing.
  • Tests

    • Expanded and improved test coverage for Gregorian, Hebrew, Islamic, and Seleucid calendar converters, including boundary conditions, leap year handling, and error messages.
    • Refactored and enhanced tests for converter discovery and availability mechanisms.

…classes.

This commit introduces extensive unit tests for newly added and existing calendar functionality:

- **GregorianDateConverter:** I added a new test suite (`test_gregorian.py`) to cover all `BaseCalendarConverter` methods (`min_month`, `max_month`, `max_day`, `to_gregorian`, etc.).
- **BaseDateConverter Subclass Discovery:** I added tests to `test_base.py` to verify that `available_converters()` correctly discovers direct, nested, and calendar converter subclasses, and properly excludes `BaseCalendarConverter`. This included testing the `import_converters` caching and module loading.
- **HebrewDateConverter:** I enhanced `test_hebrew_converter.py` with explicit tests for all `BaseCalendarConverter` methods. I made minor adjustments to the converter to handle `None` inputs for year/month in `max_day` and `max_month`.
- **IslamicDateConverter:** I enhanced `test_islamic_converter.py` with explicit tests for `BaseCalendarConverter` methods and corrected assertions in `test_partially_known`. I made minor adjustments to the converter to handle `None` inputs for year/month in `max_day`.
- **SeleucidDateConverter:** I enhanced `test_seleucid.py` with explicit tests for `to_gregorian`, `max_month`, and `max_day` using Seleucid years. I overrode `max_month`, `max_day`, and `parse` in the converter to correctly handle the `SELEUCID_OFFSET` and provide Seleucid-specific error messages. I also cleaned up commented code.

All new and existing tests pass, ensuring better coverage and robustness of the date conversion and calendar logic.
Copy link
Contributor

coderabbitai bot commented May 23, 2025

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Walkthrough

Optional parameters and default handling were added to max_month and max_day methods in Hebrew, Islamic, and Seleucid calendar converters. The Seleucid converter also gained custom error handling and calendar offset logic. Extensive new and revised tests were introduced for these converters, focusing on calendar boundaries, leap years, and error handling.

Changes

File(s) Change Summary
src/undate/converters/calendars/hebrew/converter.py Updated max_month and max_day to accept optional parameters with default fallback logic.
src/undate/converters/calendars/islamic/converter.py Modified max_day to accept optional parameters and use defaults when not provided.
src/undate/converters/calendars/seleucid.py Added max_month, max_day with year offset logic; implemented custom parse method with error handling.
tests/test_converters/test_base.py Replaced simple static tests with comprehensive, mock-based tests for dynamic subclass discovery and converter availability.
tests/test_converters/test_calendars/test_gregorian.py Added new test suite for GregorianDateConverter covering month/day boundaries and conversion logic.
tests/test_converters/test_calendars/test_hebrew/test_hebrew_converter.py Extended tests for HebrewDateConverter to cover month/day boundaries, leap years, and direct conversion to Gregorian.
tests/test_converters/test_calendars/test_islamic/test_islamic_converter.py Enhanced IslamicDateConverter tests for leap years, max day/month logic, and direct Gregorian conversion.
tests/test_converters/test_calendars/test_seleucid.py Added tests for SeleucidDateConverter covering conversion, month/day boundaries, and error handling.

Sequence Diagram(s)

sequenceDiagram
    participant Caller
    participant HebrewConverter
    participant IslamicConverter
    participant SeleucidConverter
    participant ParentConverter

    Caller->>HebrewConverter: max_month(year=None)
    alt year is None
        HebrewConverter-->>Caller: return 12
    else year provided
        HebrewConverter->>hebrew.year_months: get months in year
        hebrew.year_months-->>HebrewConverter: months
        HebrewConverter-->>Caller: return months
    end

    Caller->>HebrewConverter: max_day(year=None, month=None)
    HebrewConverter->>hebrew.month_days: with fallback year/month
    hebrew.month_days-->>HebrewConverter: days
    HebrewConverter-->>Caller: return days

    Caller->>IslamicConverter: max_day(year=None, month=None)
    IslamicConverter->>islamic.month_length: with fallback year/month
    islamic.month_length-->>IslamicConverter: days
    IslamicConverter-->>Caller: return days

    Caller->>SeleucidConverter: max_month(year)
    SeleucidConverter->>ParentConverter: max_month(year + offset)
    ParentConverter-->>SeleucidConverter: months
    SeleucidConverter-->>Caller: return months

    Caller->>SeleucidConverter: parse(value)
    SeleucidConverter->>ParentConverter: parse(value)
    alt parse fails
        SeleucidConverter-->>Caller: raise ValueError with custom message
    else parse succeeds
        SeleucidConverter-->>Caller: return parsed label
    end
Loading

Possibly related PRs

  • dh-tech/undate-python#107: Also updates calendar converter methods (notably IslamicDateConverter's max_day) to accept optional parameters and provide default handling, similar to the changes in this PR.

Suggested reviewers

  • robcast
  • jdamerow
  • ColeDCrawford

Poem

In the warren where calendars leap and bound,
Rabbits code where new defaults are found.
Months and days, now flexible and bright,
Seleucid and Hebrew, all handled right.
With tests that hop through every case,
This patch brings order to time and space!
🐇⏳


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

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Explain this complex logic.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai explain this code block.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and explain its main purpose.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Support

Need 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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

codecov bot commented May 23, 2025

Codecov Report

Attention: Patch coverage is 97.51553% with 8 lines in your changes missing coverage. Please review.

Project coverage is 98.10%. Comparing base (8b3030b) to head (2e30b4f).

Files with missing lines Patch % Lines
tests/test_converters/test_base.py 95.17% 7 Missing ⚠️
...rc/undate/converters/calendars/hebrew/converter.py 88.88% 1 Missing ⚠️
Additional details and impacted files
@@                  Coverage Diff                  @@
##           feature/pgp-dates     #132      +/-   ##
=====================================================
- Coverage              98.60%   98.10%   -0.51%     
=====================================================
  Files                     37       38       +1     
  Lines                   1582     1850     +268     
=====================================================
+ Hits                    1560     1815     +255     
- Misses                    22       35      +13     

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

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

@rlskoeser
Copy link
Member Author

@coderabbitai review

Copy link
Contributor

coderabbitai bot commented May 23, 2025

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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

🧹 Nitpick comments (3)
src/undate/converters/calendars/hebrew/converter.py (1)

49-66: Refactor for better readability and remove unnecessary logic.

The implementation is functionally correct but has some issues:

  1. Unnecessary conditional logic: The conditional check on lines 60-63 for Adar months is redundant since you're already defaulting to a non-leap year (5783).
  2. Code clarity: The extensive comments, while helpful, make the code harder to read.

Consider this cleaner implementation:

-    def max_day(self, year: int = None, month: int = None) -> int: # Added default None
+    def max_day(self, year: int = None, month: int = None) -> int:
         """maximum numeric day for the specified year and month in this calendar"""
-        # NOTE: unreleased v2.4.1 of convertdate standardizes month_days to month_length
-        
-        # Handle None year/month by defaulting to a common non-leap scenario
-        # Default year to a known non-leap year if None, e.g. 5783
-        # Default month to Nisan (1) if None, as it's the first biblical month and always has 30 days.
-        effective_year = year if year is not None else 5783 # 5783 is a non-leap year
-        effective_month = month if month is not None else hebrew.NISAN # Nisan is 1
-
-        # Ensure year is not None for leap check if month is Adar related and year was originally None
-        if year is None and (effective_month == 12 or effective_month == 13): # Adar, Adar I or Adar II
-             # hebrew.month_days needs a concrete year to determine leap month lengths correctly.
-             # We've defaulted to 5783 (non-leap).
-             pass
-
-
-        return hebrew.month_days(effective_year, effective_month)
+        # Default to a known non-leap year and first month if parameters are None
+        effective_year = year if year is not None else 5783  # 5783 is a non-leap year
+        effective_month = month if month is not None else hebrew.NISAN
+        return hebrew.month_days(effective_year, effective_month)
src/undate/converters/calendars/seleucid.py (2)

22-28: Apply static analysis suggestion for cleaner code.

The logic is correct, but the code can be simplified using a ternary operator as suggested by the static analysis tool.

-    def max_month(self, year: int = None) -> int:
-        """Maximum numeric month for this calendar. Adjusted for Seleucid year."""
-        if year is not None:
-            am_year = year + self.SELEUCID_OFFSET
-        else:
-            am_year = None # Let parent handle None AM year if applicable (it defaults to non-leap)
-        return super().max_month(am_year)
+    def max_month(self, year: int = None) -> int:
+        """Maximum numeric month for this calendar. Adjusted for Seleucid year."""
+        am_year = year + self.SELEUCID_OFFSET if year is not None else None
+        return super().max_month(am_year)
🧰 Tools
🪛 Ruff (0.11.9)

24-27: Use ternary operator am_year = year + self.SELEUCID_OFFSET if year is not None else None instead of if-else-block

(SIM108)


30-38: Apply static analysis suggestion and improve consistency.

Similar to max_month, this can be simplified with a ternary operator.

-    def max_day(self, year: int = None, month: int = None) -> int:
-        """Maximum numeric day for the specified year and month. Adjusted for Seleucid year."""
-        if year is not None:
-            am_year = year + self.SELEUCID_OFFSET
-        else:
-            am_year = None # Parent's max_day handles None year by defaulting
-        
-        # Parent's max_day handles None month by defaulting, so pass month as is.
-        return super().max_day(am_year, month)
+    def max_day(self, year: int = None, month: int = None) -> int:
+        """Maximum numeric day for the specified year and month. Adjusted for Seleucid year."""
+        am_year = year + self.SELEUCID_OFFSET if year is not None else None
+        return super().max_day(am_year, month)
🧰 Tools
🪛 Ruff (0.11.9)

32-35: Use ternary operator am_year = year + self.SELEUCID_OFFSET if year is not None else None instead of if-else-block

(SIM108)

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8b3030b and 0ddd6ec.

📒 Files selected for processing (8)
  • src/undate/converters/calendars/hebrew/converter.py (2 hunks)
  • src/undate/converters/calendars/islamic/converter.py (1 hunks)
  • src/undate/converters/calendars/seleucid.py (1 hunks)
  • tests/test_converters/test_base.py (1 hunks)
  • tests/test_converters/test_calendars/test_gregorian.py (1 hunks)
  • tests/test_converters/test_calendars/test_hebrew/test_hebrew_converter.py (2 hunks)
  • tests/test_converters/test_calendars/test_islamic/test_islamic_converter.py (2 hunks)
  • tests/test_converters/test_calendars/test_seleucid.py (2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
tests/test_converters/test_calendars/test_gregorian.py (2)
src/undate/converters/calendars/gregorian.py (1)
  • GregorianDateConverter (6-51)
tests/test_converters/test_base.py (4)
  • min_month (34-34)
  • max_month (35-35)
  • max_day (36-36)
  • to_gregorian (32-32)
src/undate/converters/calendars/islamic/converter.py (3)
src/undate/converters/calendars/seleucid.py (1)
  • max_day (30-38)
src/undate/converters/calendars/hebrew/converter.py (1)
  • max_day (49-66)
src/undate/converters/base.py (1)
  • max_day (161-163)
🪛 Ruff (0.11.9)
src/undate/converters/calendars/seleucid.py

24-27: Use ternary operator am_year = year + self.SELEUCID_OFFSET if year is not None else None instead of if-else-block

(SIM108)


32-35: Use ternary operator am_year = year + self.SELEUCID_OFFSET if year is not None else None instead of if-else-block

(SIM108)

tests/test_converters/test_base.py

5-5: typing.Type imported but unused

Remove unused import: typing.Type

(F401)


244-244: Local variable original_iter_modules is assigned to but never used

Remove assignment to unused variable original_iter_modules

(F841)

🪛 GitHub Actions: Check style + docs + types
tests/test_converters/test_base.py

[error] 5-5: Ruff: typing.Type imported but unused (F401). Remove unused import.


[error] 244-244: Ruff: Local variable original_iter_modules is assigned to but never used (F841). Remove assignment to unused variable.

🔇 Additional comments (17)
src/undate/converters/calendars/hebrew/converter.py (1)

31-38: LGTM! Clean implementation of optional year parameter.

The max_month method correctly handles the optional year parameter by defaulting to 12 months for non-leap years when year is not specified. This aligns well with the Hebrew calendar's structure.

src/undate/converters/calendars/islamic/converter.py (1)

27-35: Excellent implementation - clean and consistent.

The max_day method implementation is well-structured and follows good practices:

  • Clear, concise logic with appropriate fallback defaults
  • Good choice of 1446 AH (non-leap year) and month 1 (Muharram) for defaults
  • Consistent with the pattern established across other calendar converters
  • Clean documentation explaining the behavior

This implementation is cleaner than the Hebrew converter version and serves as a good example.

tests/test_converters/test_calendars/test_gregorian.py (3)

1-26: Excellent test structure and comprehensive coverage.

The test class setup and basic method tests are well-implemented:

  • Clear test organization with descriptive method names
  • Good coverage of boundary conditions including None parameters
  • Proper testing of leap year scenarios in max_month tests

28-49: Outstanding parametrized test design.

The parametrized test for max_day provides excellent coverage:

  • Comprehensive test cases covering all scenarios (known/unknown year/month combinations)
  • Proper leap year testing (2024 vs 2023, None defaults to non-leap)
  • Clear, readable test data structure
  • Good edge case coverage including the maximum default (31 days)

This is a model example of thorough parametrized testing.


51-62: Complete test coverage with important edge cases.

The to_gregorian test covers crucial scenarios:

  • Regular dates
  • Leap day (Feb 29, 2024)
  • Century leap year exceptions (1900 non-leap, 2000 leap)

This ensures the Gregorian converter correctly handles all calendar edge cases.

src/undate/converters/calendars/seleucid.py (1)

18-21: Good documentation of parser inheritance decision.

The comment clearly explains the current parser strategy and what would need to change if Seleucid date formats differ from Hebrew. This helps future maintainers understand the design decision.

tests/test_converters/test_calendars/test_islamic/test_islamic_converter.py (3)

7-7: Good improvements to test structure!

Adding the convertdate import and class-level converter instance improves test maintainability and performance.

Also applies to: 11-11


92-107: Excellent improvement using dynamic month length calculations!

Using islamic.month_length() instead of hardcoded values ensures the tests remain accurate if the calendar implementation changes.


127-185: Comprehensive test coverage for BaseCalendarConverter methods!

The new tests thoroughly verify:

  • Min/max month boundaries
  • Leap year handling
  • Default behavior with None parameters
  • Direct Gregorian conversions

This significantly improves test coverage and reliability.

tests/test_converters/test_calendars/test_hebrew/test_hebrew_converter.py (2)

7-7: Consistent test structure improvements!

Following the same pattern as the Islamic converter tests improves consistency across the test suite.

Also applies to: 11-11


160-251: Thorough test coverage for Hebrew calendar specifics!

The tests properly handle Hebrew calendar complexities:

  • Leap year detection with 13 months
  • Adar I/II handling in leap years
  • Correct month numbering and day counts
  • Edge cases with None parameters

Excellent attention to calendar-specific details.

tests/test_converters/test_calendars/test_seleucid.py (3)

1-1: Well-structured test setup!

Good additions for pytest assertions and Hebrew leap year checks needed for Seleucid calendar calculations.

Also applies to: 5-5, 9-9


60-77: Excellent test coverage for Seleucid-specific conversions!

The tests properly verify:

  • Direct Gregorian conversions
  • SELEUCID_OFFSET calculations
  • Leap year handling through Hebrew calendar

Good attention to the relationship between Seleucid and Hebrew calendars.


120-135: Comprehensive error message testing!

The tests verify both the error type and message content, ensuring users get helpful error messages specific to Seleucid dates.

tests/test_converters/test_base.py (3)

15-37: Well-designed dummy converters for testing!

The dummy converter classes provide good test coverage for:

  • Direct subclasses
  • Calendar converter subclasses
  • Required method implementations

This enables thorough testing of the discovery mechanism.


41-117: Excellent test coverage for converter discovery mechanisms!

The tests comprehensively verify:

  • Caching behavior of import_converters
  • Module loading side effects
  • Mock-based testing to isolate behavior

Great use of mocking to test the import mechanism without side effects.


197-314: Impressive dynamic module testing!

This test for dynamic module discovery is particularly well-crafted:

  • Creates a temporary module at runtime
  • Tests the discovery mechanism with realistic conditions
  • Properly cleans up after itself

This ensures the converter discovery works for dynamically loaded modules.

🧰 Tools
🪛 Ruff (0.11.9)

244-244: Local variable original_iter_modules is assigned to but never used

Remove assignment to unused variable original_iter_modules

(F841)

🪛 GitHub Actions: Check style + docs + types

[error] 244-244: Ruff: Local variable original_iter_modules is assigned to but never used (F841). Remove assignment to unused variable.

Comment on lines 47 to 71
def parse(self, value: str): # Add type hint later if copying from parent
"""
Parse a Seleucid date string and return an :class:`~undate.undate.Undate` or
:class:`~undate.undate.UndateInterval`.
The Seleucid date string is preserved in the undate label.
This method overrides the parent to provide a Seleucid-specific error message.
"""
if not value: # Copied from HebrewDateConverter.parse
raise ValueError("Parsing empty string is not supported")

# Import the parser used by the parent class directly
from undate.converters.calendars.hebrew.parser import hebrew_parser
try:
# Uses the hebrew_parser and self.transformer (calendar set to SELEUCID in __init__)
parsetree = hebrew_parser.parse(value)
undate_obj = self.transformer.transform(parsetree)
undate_obj.label = f"{value} {self.calendar_name}"
return undate_obj
except Exception as err: # Catching broader Exception to be safe, can refine
# Check if it's an UnexpectedCharacters error if lark is used by hebrew_parser
from lark.exceptions import UnexpectedCharacters
if isinstance(err, UnexpectedCharacters):
raise ValueError(f"Could not parse '{value}' as a Seleucid date") from err
# Re-raise other errors or handle them if they are expected
raise # Re-raise if not the specific parsing error we want to customize
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Simplify error handling and improve code clarity.

The parse method logic is correct but overly complex. The current implementation catches broad exceptions and then checks their type, which is unnecessarily complicated.

-    def parse(self, value: str): # Add type hint later if copying from parent
+    def parse(self, value: str) -> Union[Undate, UndateInterval]:
         """
         Parse a Seleucid date string and return an :class:`~undate.undate.Undate` or
         :class:`~undate.undate.UndateInterval`.
         The Seleucid date string is preserved in the undate label.
-        This method overrides the parent to provide a Seleucid-specific error message.
         """
-        if not value: # Copied from HebrewDateConverter.parse
+        if not value:
             raise ValueError("Parsing empty string is not supported")

-        # Import the parser used by the parent class directly
         from undate.converters.calendars.hebrew.parser import hebrew_parser
+        from lark.exceptions import UnexpectedCharacters
+        
         try:
-            # Uses the hebrew_parser and self.transformer (calendar set to SELEUCID in __init__)
-            parsetree = hebrew_parser.parse(value) 
+            parsetree = hebrew_parser.parse(value)
             undate_obj = self.transformer.transform(parsetree)
             undate_obj.label = f"{value} {self.calendar_name}"
             return undate_obj
-        except Exception as err: # Catching broader Exception to be safe, can refine
-            # Check if it's an UnexpectedCharacters error if lark is used by hebrew_parser
-            from lark.exceptions import UnexpectedCharacters
-            if isinstance(err, UnexpectedCharacters):
-                 raise ValueError(f"Could not parse '{value}' as a Seleucid date") from err
-            # Re-raise other errors or handle them if they are expected
-            raise # Re-raise if not the specific parsing error we want to customize
+        except UnexpectedCharacters as err:
+            raise ValueError(f"Could not parse '{value}' as a Seleucid date") from err

The simplified version:

  • Adds proper type hints
  • Catches the specific exception we care about
  • Removes unnecessary complexity
  • Maintains the same functionality
📝 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.

Suggested change
def parse(self, value: str): # Add type hint later if copying from parent
"""
Parse a Seleucid date string and return an :class:`~undate.undate.Undate` or
:class:`~undate.undate.UndateInterval`.
The Seleucid date string is preserved in the undate label.
This method overrides the parent to provide a Seleucid-specific error message.
"""
if not value: # Copied from HebrewDateConverter.parse
raise ValueError("Parsing empty string is not supported")
# Import the parser used by the parent class directly
from undate.converters.calendars.hebrew.parser import hebrew_parser
try:
# Uses the hebrew_parser and self.transformer (calendar set to SELEUCID in __init__)
parsetree = hebrew_parser.parse(value)
undate_obj = self.transformer.transform(parsetree)
undate_obj.label = f"{value} {self.calendar_name}"
return undate_obj
except Exception as err: # Catching broader Exception to be safe, can refine
# Check if it's an UnexpectedCharacters error if lark is used by hebrew_parser
from lark.exceptions import UnexpectedCharacters
if isinstance(err, UnexpectedCharacters):
raise ValueError(f"Could not parse '{value}' as a Seleucid date") from err
# Re-raise other errors or handle them if they are expected
raise # Re-raise if not the specific parsing error we want to customize
def parse(self, value: str) -> Union[Undate, UndateInterval]:
"""
Parse a Seleucid date string and return an :class:`~undate.undate.Undate` or
:class:`~undate.undate.UndateInterval`.
The Seleucid date string is preserved in the undate label.
"""
if not value:
raise ValueError("Parsing empty string is not supported")
from undate.converters.calendars.hebrew.parser import hebrew_parser
from lark.exceptions import UnexpectedCharacters
try:
parsetree = hebrew_parser.parse(value)
undate_obj = self.transformer.transform(parsetree)
undate_obj.label = f"{value} {self.calendar_name}"
return undate_obj
except UnexpectedCharacters as err:
raise ValueError(f"Could not parse '{value}' as a Seleucid date") from err
🤖 Prompt for AI Agents
In src/undate/converters/calendars/seleucid.py lines 47 to 71, simplify the
parse method by adding proper type hints and catching only the specific
exception (UnexpectedCharacters) instead of a broad Exception. Remove the
general try-except block and directly handle the specific parsing error to
improve clarity while preserving the existing functionality and error messaging.

import undate.converters
converters_path = undate.converters.__path__ # This is a list of paths

original_iter_modules = importlib.import_module('pkgutil').iter_modules
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Remove unused variable assignment.

The original_iter_modules variable is assigned but never used.

-        original_iter_modules = importlib.import_module('pkgutil').iter_modules
-
         def simplified_mock_iter_modules(path=None, prefix=''):
📝 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.

Suggested change
original_iter_modules = importlib.import_module('pkgutil').iter_modules
def simplified_mock_iter_modules(path=None, prefix=''):
🧰 Tools
🪛 Ruff (0.11.9)

244-244: Local variable original_iter_modules is assigned to but never used

Remove assignment to unused variable original_iter_modules

(F841)

🪛 GitHub Actions: Check style + docs + types

[error] 244-244: Ruff: Local variable original_iter_modules is assigned to but never used (F841). Remove assignment to unused variable.

🤖 Prompt for AI Agents
In tests/test_converters/test_base.py at line 244, remove the assignment of the
variable original_iter_modules since it is declared but never used. Simply
delete the entire line that assigns original_iter_modules to
importlib.import_module('pkgutil').iter_modules to clean up unused code.

import sys
import importlib
from unittest.mock import patch, MagicMock
from typing import Type # For type hinting
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Remove unused import.

The Type import is not used in the code.

-from typing import Type # For type hinting
📝 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.

Suggested change
from typing import Type # For type hinting
🧰 Tools
🪛 Ruff (0.11.9)

5-5: typing.Type imported but unused

Remove unused import: typing.Type

(F401)

🪛 GitHub Actions: Check style + docs + types

[error] 5-5: Ruff: typing.Type imported but unused (F401). Remove unused import.

🤖 Prompt for AI Agents
In tests/test_converters/test_base.py at line 5, the import statement for `Type`
from the `typing` module is unused. Remove this import line entirely to clean up
the code and avoid unnecessary imports.

@rlskoeser rlskoeser changed the title I've added comprehensive unit tests for calendar converters and base … 🤖 I've added comprehensive unit tests for calendar converters and base … May 23, 2025
@rlskoeser rlskoeser marked this pull request as draft May 23, 2025 21:04
- Refined `SeleucidDateConverter.parse` method:
    - Added specific type hints for parameters and return values.
    - Ensured the try-except block specifically catches `lark.exceptions.UnexpectedCharacters`.
    - Preserved the existing error message format for parsing failures.
- Cleaned up `tests/test_converters/test_base.py`:
    - Removed an unused variable assignment (`original_iter_modules`).
    - Ensured an unused import (`from typing import Type`) was removed.

All relevant tests pass, confirming the changes are safe and improve code clarity.
Base automatically changed from feature/pgp-dates to develop June 25, 2025 17:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant