Clean up flagged by @coderabbitai with additional refactoring

Author: rlskoeser

src/undate/converters/base.py

@@ -48,6 +48,8 @@
 from functools import cache
 from typing import Dict, Type
 
+from undate.date import Date
+
 logger = logging.getLogger(__name__)
 
 
@@ -58,6 +60,10 @@ class BaseDateConverter:
     #: Converter name. Subclasses must define a unique name.
     name: str = "Base Converter"
 
+    # provisional...
+    LEAP_YEAR = 0
+    NON_LEAP_YEAR = 0
+
     def parse(self, value: str):
         """
         Parse a string and return an :class:`~undate.undate.Undate` or
@@ -163,8 +169,17 @@ def max_day(self, year: int, month: int) -> int:
         raise NotImplementedError
 
     def days_in_year(self, year: int) -> int:
-        """number of days in the specified year in this calendar"""
-        raise NotImplementedError
+        """Number of days in the specified year in this calendar. The default implementation
+        uses min and max month and max day methods along with Gregorian conversion method
+        to calculate the number of days in the specified year.
+        """
+        year_start = Date(*self.to_gregorian(year, self.min_month(), 1))
+        last_month = self.max_month(year)
+        year_end = Date(
+            *self.to_gregorian(year, last_month, self.max_day(year, last_month))
+        )
+        # add 1 because the difference doesn't include the end point
+        return (year_end - year_start).days + 1
 
     def to_gregorian(self, year, month, day) -> tuple[int, int, int]:
         """Convert a date for this calendar specified by numeric year, month, and day,

src/undate/converters/calendars/seleucid.py

@@ -22,3 +22,7 @@ def to_gregorian(self, year: int, month: int, day: int) -> tuple[int, int, int]:
         logic with :attr:`SELEUCID_OFFSET`. Returns a tuple of year, month, day.
         """
         return super().to_gregorian(year + self.SELEUCID_OFFSET, month, day)
+
+    def days_in_year(self, year: int) -> int:
+        """the number of days in the specified year for this calendar"""
+        return super().days_in_year(year + self.SELEUCID_OFFSET)

src/undate/undate.py

@@ -19,7 +19,7 @@
 # Pre 3.10 requires Union for multiple types, e.g. Union[int, None] instead of int | None
 from typing import Dict, Optional, Union
 
-from undate.converters.base import BaseDateConverter
+from undate.converters.base import BaseCalendarConverter, BaseDateConverter
 from undate.date import ONE_DAY, Date, DatePrecision, Timedelta, UnDelta
 
 
@@ -32,10 +32,14 @@ class Calendar(StrEnum):
     SELEUCID = auto()
 
     @staticmethod
-    def get_converter(calendar):
+    def get_converter(calendar) -> BaseCalendarConverter:
         # calendar converter must be available with a name matching
         # the title-case name of the calendar enum entry
         converter_cls = BaseDateConverter.available_converters()[calendar.value.title()]
+        if not issubclass(converter_cls, BaseCalendarConverter):
+            raise ValueError(
+                f"Requested converter {converter_cls} is not a CalendarConverter"
+            )
         return converter_cls()
 
 
@@ -492,13 +496,12 @@ def duration(self) -> Timedelta | UnDelta:
                 self.calendar_converter.LEAP_YEAR,
                 self.calendar_converter.NON_LEAP_YEAR,
             ]
-        possible_max_days = None
+        possible_max_days = set()
 
         # if precision is month and year is unknown,
         # calculate month duration within a single year (not min/max)
         if self.precision == DatePrecision.MONTH:
             # for every possible month and year, get max days for that month,
-            possible_max_days = set()
             # appease mypy, which says month values could be None here;
             # Date object allows optional month, but earliest/latest initialization
             # should always be day-precision dates
@@ -511,40 +514,17 @@ def duration(self) -> Timedelta | UnDelta:
 
         # if precision is year but year is unknown, return an uncertain delta
         elif self.precision == DatePrecision.YEAR:
-            possible_max_days = set()
             # this is currently hebrew-specific due to the way the start/end of year wraps for that calendar
-            try:
-                possible_max_days = set(
-                    [self.calendar_converter.days_in_year(y) for y in possible_years]
-                )
-            except NotImplementedError:
-                pass
-
-            if not possible_max_days:
-                for year in possible_years:
-                    # TODO: shift logic to calendars for parity with Hebrew?
-                    year_start = Date(
-                        *self.calendar_converter.to_gregorian(
-                            year, self.calendar_converter.min_month(), 1
-                        )
-                    )
-                    last_month = self.calendar_converter.max_month(year)
-                    year_end = Date(
-                        *self.calendar_converter.to_gregorian(
-                            year,
-                            last_month,
-                            self.calendar_converter.max_day(year, last_month),
-                        )
-                    )
-
-                    year_days = (year_end - year_start).days + 1
-                    possible_max_days.add(year_days)
+            # with contextlib.suppress(NotImplementedError):
+            possible_max_days = {
+                self.calendar_converter.days_in_year(y) for y in possible_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
-        if possible_max_days and len(possible_max_days) > 1:
-            return UnDelta(*possible_max_days)
-        else:
+        if possible_max_days:
+            if len(possible_max_days) > 1:
+                return UnDelta(*possible_max_days)
             return Timedelta(possible_max_days.pop())
 
         # otherwise, subtract earliest from latest and add a day to include start day in the count

tests/test_undate.py

@@ -448,9 +448,9 @@ def test_partiallyknownyear_duration(self):
         assert Undate("108X", calendar="Islamic").duration().days == UnInt(354, 355)
         # NOTE: completely unknown years is not yet supported for other calendars, will cause an error
         # assert Undate("XXXX", calendar="Islamic").duration().days == UnInt(354, 355)
-        #
-        print(Undate("536X", calendar="Hebrew").duration())
         assert Undate("536X", calendar="Hebrew").duration().days == UnInt(353, 385)
+        # different set of years could vary
+        assert Undate("53X2", calendar="Hebrew").duration().days == UnInt(354, 385)
 
     def test_known_year(self):
         assert Undate(2022).known_year is True