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. 🗂️ Base branches to auto review (1)
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)}") |
There was a problem hiding this comment.
This very long Kypso comment seems to just suggest adding a catchall Exception at the end?
There was a problem hiding this comment.
We should probably throw a TypeError in parse() if we get an invalid value - anything other than a string?
| | 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.
There was a problem hiding this comment.
Do we have such a range? I'm not very familiar with the Hebrew calendar, but it looks like the AM systems starts at Biblical creation (3761 BCE) - so anything prior to that is likely invalid? But no future limit?
| 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") |
There was a problem hiding this comment.
Think handling this upstream in the converter and checking it here makes sense
|
|
||
| @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): |
There was a problem hiding this comment.
It looks like the unit tests don't run when the target branch isn't develop - do we want to change that if we're going to have feature to feature merges? I ran them on a fresh codespace with this branch and they all pass, so looks good on that front
Is the plan to merge this PR first and then the Hijri PR to develop?
There was a problem hiding this comment.
The change here makes gregorian the default if no calendar is specified? It seems a bit cleaner to keep these unimplemented in converters/base.py and then implement the needed methods in the specific calendars, but maybe I'm missing something.
|
|
||
| def max_day(self, year: int, month: int) -> 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 |
There was a problem hiding this comment.
Is there a maintainer for convertdate? Looks like that branch has been hanging for a couple of years. Not sure if that's an issue if the library is doing what we need it to
There was a problem hiding this comment.
You're right, it does look pretty unmaintained. My PR a couple of years ago was merged in pretty quickly but never released. I see some open PRs hanging out that look kind of old...
Not great, but the existing calendar conversion logic is enough for what we need now, so I'm inclined to keep using it until it becomes a problem. I'm not aware of another comparable option in python.
| 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.
This very long Kypso comment seems to just suggest adding a catchall Exception at the end?
| // 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.
Do we have such a range? I'm not very familiar with the Hebrew calendar, but it looks like the AM systems starts at Biblical creation (3761 BCE) - so anything prior to that is likely invalid? But no future limit?
| @@ -0,0 +1,3 @@ | |||
| from undate.converters.calendars.hijri.converter import HijriDateConverter | |||
|
|
|||
| __all__ = ["HijriDateConverter"] | |||
There was a problem hiding this comment.
We want to be able to import HijriDateConverter from undate.converters.calendars.hebrew?
There was a problem hiding this comment.
no, whoops, I just missed some things when I copied hijri calendar code over for hebrew calendar (caught some but not all)
There was a problem hiding this comment.
Really liking how clean lark is here
| def max_month(self, year: int) -> int: | ||
| """maximum numeric month for the specified year in this calendar""" | ||
| return 12 | ||
|
|
There was a problem hiding this comment.
This is being pulled because the base converter now defaults to 12?
There was a problem hiding this comment.
yes, does that makes sense to you?
There was a problem hiding this comment.
I think I prefer keeping the unimplemented methods as stubs in the base and requiring specific converters to implement them, since there aren't too many. But it makes sense either way
src/undate/undate.py
Outdated
| max_month = self.calendar_converter.max_month(max_year) | ||
| # get first and last month from the calendar, since it is not | ||
| # always 1 and 12 | ||
| # TODO need to differentiate between min/max and first/last! |
There was a problem hiding this comment.
Is this also an issue in Hebrew? It looks like there are two different start months (Tishri which is ~September for calendar year, and Nisan for religious year which is ~March/April?)
There was a problem hiding this comment.
I think it only applies to Hebrew calendar and not Hijri - but maybe others I'm not aware of
| 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.
We should probably throw a TypeError in parse() if we get an invalid value - anything other than a string?
| HebrewDateConverter().parse("January 2, 1991") | ||
| # empty string should also error | ||
| with pytest.raises(ValueError): | ||
| HebrewDateConverter().parse("") |
There was a problem hiding this comment.
Think handling this upstream in the converter and checking it here makes sense
I was trying to do stacked prs, so once the hijri is merged this one would be updated to point to develop. But if I'm doing that I should probably make unit tests run on any pr... |
Yeah, I think that change makes sense. |
src/undate/converters/base.py
Outdated
| def min_month(self) -> int: | ||
| """First month for this calendar. Defaults to 1.""" | ||
| return 1 | ||
|
|
||
| def max_month(self) -> int: | ||
| """Last month for this calendar. Defaults to 12.""" | ||
| return 12 |
There was a problem hiding this comment.
Why is min_month / max_month set to 1 and 12 in this base class? Should this not be abstract here and set in the gregorian class?
| def max_month(self, year: int) -> int: | ||
| """Maximum month for this calendar for this year""" | ||
| return 12 | ||
|
|
There was a problem hiding this comment.
Shouldn't min_month and max_month be defined in this class? (see comment above)
| from undate.converters.calendars.hijri.converter import HijriDateConverter | ||
|
|
||
| __all__ = ["HijriDateConverter"] |
There was a problem hiding this comment.
Why does this export HijriDateConverter? (copy/paste error?)
|
|
||
| __all__ = ["HijriDateConverter"] | ||
| __all__ = ["HijriDateConverter", "HebrewDateConverter"] |
There was a problem hiding this comment.
Why does this export HebrewDateConverter? (copy/paste error?)
| def max_month(self, year: int) -> int: | ||
| """maximum numeric month for the specified year in this calendar""" | ||
| return 12 | ||
|
|
There was a problem hiding this comment.
Shouldn't concrete min/max_month definitions stay in the concrete classes? (see above)
No description provided.