Add converter for hebrew calendar based on hijri calendar #108
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 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 (
|
![kypso-io[bot]](https://avatars.githubusercontent.com/in/843074?s=60&v=4){kind=small}
There was a problem hiding this comment.
📋 Review Summary
- Number of files reviewed: 13
- Number of comments: 5
- Number of suggestions: 3
📚 File changes
| File path | File changes |
|---|---|
| src/undate/converters/base.py | Modified the documentation for adding a new calendar converter and adjusted the method definitions in the BaseCalendarConverter class. |
| src/undate/converters/calendars/gregorian.py | Removed the max_month method that defined the maximum month for the Gregorian calendar. |
| src/undate/converters/calendars/hebrew/init.py | Added import for HijriDateConverter and updated __all__. |
| src/undate/converters/calendars/hebrew/converter.py | Added a converter for Hebrew calendar based on Hijri calendar. |
| src/undate/converters/calendars/hebrew/hebrew.lark | Added a converter for the Hebrew calendar based on the Hijri calendar. |
| src/undate/converters/calendars/hebrew/parser.py | Added imports and parser initialization for Hebrew calendar. |
| src/undate/converters/calendars/hebrew/transformer.py | Added a Hebrew calendar converter based on the Hijri calendar. |
| src/undate/converters/calendars/hijri/init.py | Added import for HebrewDateConverter and updated __all__ to include it. |
| src/undate/converters/calendars/hijri/converter.py | Removed the max_month method and modified the docstring in the parse method. |
| src/undate/undate.py | Added support for the Hebrew calendar in the Calendar enum and adjusted month handling. |
| tests/test_converters/test_calendars/test_hebrew/test_hebrew_converter.py | Added tests for Hebrew date conversion. |
| tests/test_converters/test_calendars/test_hebrew/test_hebrew_parser.py | Added test cases for valid and invalid Hebrew calendar dates. |
| tests/test_converters/test_calendars/test_hebrew/test_hebrew_transformer.py | Added tests for Hebrew calendar conversions. |
Ignored comments
src/undate/converters/base.py
-
refactor_suggestion: The method
max_monthis defined twice, once with a parameteryearand once without. This could lead to confusion about which method should be used. Consider removing the first definition and keeping the one without parameters, or clarify the purpose of each method if both are necessary. -
refactor_suggestion: The method
min_monthis defined aftermax_month, which may lead to confusion regarding the order of methods. Consider reordering the methods to group similar functionalities together for better readability.
src/undate/converters/calendars/gregorian.py
- refactor_suggestion: The
max_monthmethod has been removed, but it may be useful to retain a method that explicitly defines the maximum month for the Gregorian calendar. Consider keeping this method for clarity and future extensibility.
src/undate/converters/calendars/hebrew/init.py
- refactor_suggestion: Consider using a more descriptive name for the
__all__variable to indicate that it includes converters for the Hebrew calendar, especially if more converters are added in the future.
all = ["HebrewDateConverter", "HijriDateConverter"]
src/undate/converters/calendars/hebrew/hebrew.lark
- refactor_suggestion: The
hebrew_daterule currently supports multiple formats, but it may be beneficial to explicitly define the expected formats for clarity. Consider breaking down the formats into separate rules or providing a more detailed comment on the expected input formats.
src/undate/converters/calendars/hebrew/parser.py
- refactor_suggestion: Consider using a context manager for opening files to ensure proper handling of file resources. This can help prevent potential resource leaks if the file is not closed properly in case of an error.
with open(grammar_path, 'r') as grammar:
hebrew_parser = Lark(grammar.read(), start="hebrew_date", strict=True)
src/undate/converters/calendars/hijri/init.py
- refactor_suggestion: Consider organizing the imports alphabetically for better readability and maintainability.
src/undate/converters/calendars/hijri/converter.py
-
refactor_suggestion: The
max_monthmethod has been removed, but it may be useful to keep it for clarity or future use. If it is not needed, consider removing any references to it in the documentation or comments to avoid confusion. -
refactor_suggestion: The comment in the
parsemethod's docstring has been modified. Ensure that the documentation accurately reflects the method's functionality and does not mislead users about the return types.
src/undate/undate.py
-
refactor_suggestion: The addition of the
HEBREWcalendar enum is appropriate, but ensure that the corresponding converter is implemented and available in theBaseDateConverterclass. Without this, theget_convertermethod will raise a KeyError ifHEBREWis used without a defined converter. -
refactor_suggestion: The comments regarding the differentiation between min/max and first/last months should be addressed in the code. Consider implementing a method to clarify this distinction, especially since it can affect how dates are calculated for different calendars.
-
refactor_suggestion: The
_missing_digit_minmaxmethod is being called with hardcoded values for min and max (1 and 12). It would be more flexible to pass these values as parameters to the method, allowing for better adaptability to different calendar systems.
tests/test_converters/test_calendars/test_hebrew/test_hebrew_transformer.py
- refactor_suggestion: Consider organizing the test cases into a more structured format, such as a dictionary, to improve clarity and maintainability. This can help in easily adding new test cases in the future.
testcases = {
"26 Tammuz 4816": (HebrewUndate(4816, 4, 26), DatePrecision.DAY),
"Tammuz 4816": (HebrewUndate(4816, 4), DatePrecision.MONTH),
"4816": (HebrewUndate(4816), DatePrecision.YEAR),
"26 Tishrei 5416": (HebrewUndate(5416, 7, 26), DatePrecision.DAY),
"Ṭevet 5362": (HebrewUndate(5362, 10), DatePrecision.MONTH),
"5362": (HebrewUndate(5362), DatePrecision.YEAR),
}
@pytest.mark.parametrize("date_string, expected, expected_precision", testcases.items())
def test_transform(date_string, expected, expected_precision):
| if not value: | ||
| raise ValueError("Parsing empty string is not supported") | ||
|
|
||
| # parse the input string, then transform to undate object | ||
| try: | ||
| # parse the string with our Hebrew date parser | ||
| parsetree = hebrew_parser.parse(value) | ||
| # transform the parse tree into an undate or undate interval | ||
| undate_obj = self.transformer.transform(parsetree) | ||
| # set the original date as a label, with the calendar name | ||
| undate_obj.label = f"{value} {self.calendar_name}" | ||
| return undate_obj | ||
| except UnexpectedCharacters as err: | ||
| raise ValueError(f"Could not parse '{value}' as a Hebrew date") from err |
There was a problem hiding this comment.
The parse method currently raises a ValueError for an empty string, which is good. However, it would be beneficial to also handle cases where the input string does not conform to expected Hebrew date formats. Consider adding a more specific exception handling for such cases to provide clearer feedback to the user.
Potential fix 🚨
| if not value: | |
| raise ValueError("Parsing empty string is not supported") | |
| # parse the input string, then transform to undate object | |
| try: | |
| # parse the string with our Hebrew date parser | |
| parsetree = hebrew_parser.parse(value) | |
| # transform the parse tree into an undate or undate interval | |
| undate_obj = self.transformer.transform(parsetree) | |
| # set the original date as a label, with the calendar name | |
| undate_obj.label = f"{value} {self.calendar_name}" | |
| return undate_obj | |
| except UnexpectedCharacters as err: | |
| raise ValueError(f"Could not parse '{value}' as a Hebrew date") from err | |
| if not value: | |
| raise ValueError("Parsing empty string is not supported") | |
| # parse the input string, then transform to undate object | |
| try: | |
| # parse the string with our Hebrew date parser | |
| parsetree = hebrew_parser.parse(value) | |
| # transform the parse tree into an undate or undate interval | |
| undate_obj = self.transformer.transform(parsetree) | |
| # set the original date as a label, with the calendar name | |
| undate_obj.label = f"{value} {self.calendar_name}" | |
| return undate_obj | |
| except UnexpectedCharacters as err: | |
| raise ValueError(f"Could not parse '{value}' as a Hebrew date") from err | |
| except Exception as e: | |
| raise ValueError(f"An error occurred while parsing '{value}': {str(e)}") |
| | month_12 | ||
| | month_13 | ||
| // months have 29 or 30 days; we do not expect leading zeroes | ||
| day: /[1-9]/ | /[12][0-9]/ | /30/ |
There was a problem hiding this comment.
The day rule allows for days 1-30, but does not account for the varying number of days in Hebrew months. This could lead to invalid dates being parsed. Consider implementing a validation mechanism to ensure that the day corresponds correctly to the month and year provided.
| // PGP dates use qualifiers like "first decade of" (for beginning of month) | ||
| // "first third of", seasons (can look for more examples) | ||
|
|
||
| year: /\d+/ |
There was a problem hiding this comment.
The year rule currently accepts any numeric input. It would be prudent to restrict this to a valid range of years for the Hebrew calendar to prevent invalid year inputs from being processed.
| HebrewDateConverter().parse("January 2, 1991") | ||
| # empty string should also error | ||
| with pytest.raises(ValueError): | ||
| HebrewDateConverter().parse("") |
There was a problem hiding this comment.
The error handling for parsing invalid strings is good, but it would be beneficial to also handle cases where the input is not a string type (e.g., integers, None). This can prevent unexpected crashes in the application. Consider adding a type check before parsing the date string.
Potential fix 🚨
| HebrewDateConverter().parse("January 2, 1991") | |
| # empty string should also error | |
| with pytest.raises(ValueError): | |
| HebrewDateConverter().parse("") | |
| if not isinstance(date_str, str): | |
| raise ValueError("Input must be a string") |
|
|
||
| @pytest.mark.parametrize("date_string", error_cases) | ||
| def test_should_error(date_string): | ||
| with pytest.raises(Exception): |
There was a problem hiding this comment.
Using a generic Exception in the pytest.raises context can make it difficult to identify the specific error that occurred. It is better to catch a more specific exception that the hebrew_parser.parse function is expected to raise for invalid inputs.
Potential fix 🚨
| with pytest.raises(Exception): | |
| with pytest.raises(ValueError): |
No description provided.