refactor: modernize lunardate package#21
refactor: modernize lunardate package#21ajbrittle1975 wants to merge 4 commits intolidaobing:masterfrom
Conversation
|
Full refactor of original codebase. |
|
Hi, @ajbrittle1975 thanks for your great work. How about keeping Thanks, |
There was a problem hiding this comment.
Pull request overview
This PR modernizes the legacy single-file lunardate.py module into a typed, src/-layout Python package with separated data/conversion logic, updated packaging (pyproject.toml), refreshed docs, and a new pytest suite.
Changes:
- Refactors the library into
src/lunardate/with_data.py,_conversions.py, and a modernizedLunarDatepublic API (snake_case + deprecated camelCase aliases). - Replaces legacy packaging artifacts (
setup.py, committed*.egg-info) with PEP 621pyproject.toml, addspy.typed, and updates CI/lint/type-check configuration. - Adds pytest coverage for API behavior, conversions, and data-table integrity; updates README/NOTICE/LICENSE attribution and project documentation.
Reviewed changes
Copilot reviewed 21 out of 22 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
src/lunardate/lunardate.py |
New LunarDate implementation with validation, ordering, arithmetic, hashing, and deprecated aliases. |
src/lunardate/_conversions.py |
Core conversion/validation helpers (solar↔lunar, offsets, month enumeration). |
src/lunardate/_data.py |
Extracted year info table and derived year-day calculations/constants. |
src/lunardate/__init__.py |
Public package surface + __version__ and re-export. |
src/lunardate/py.typed |
PEP 561 marker for typed package distribution. |
tests/conftest.py |
Shared fixture for known conversion pairs (and test import path setup). |
tests/test_lunardate.py |
Tests for public API, arithmetic, ordering, hashing, immutability, and deprecations. |
tests/test_conversions.py |
Tests for conversion helpers, enum_month behavior, and validation. |
tests/test_data.py |
Tests for data-table integrity and derived constants. |
pyproject.toml |
New build metadata + dev dependencies + tooling config (ruff/mypy/pytest). |
.github/workflows/python-package.yml |
Updates CI matrix and replaces flake8/doctest approach with ruff+mypy+pytest. |
README.md |
Updated quickstart, migration guide, dev instructions, and attribution/licensing notes. |
NOTICE.md / LICENSE.txt |
Added/expanded attribution and license preface. |
.gitignore |
Modern Python ignores (caches, venvs, build artifacts, etc.). |
setup.py / lunardate.py / lunardate.egg-info/* |
Removed legacy packaging/module artifacts. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def solar_to_lunar(year: int, month: int, day: int) -> tuple[int, int, int, bool]: | ||
| """Convert a solar (Gregorian) date to a lunar date tuple.""" | ||
| solar_date = datetime.date(year, month, day) | ||
| offset = (solar_date - _START_DATE).days | ||
| return offset_to_lunar(offset) |
There was a problem hiding this comment.
There are no tests asserting the behavior for solar dates outside the supported range (before 1900-01-31 or beyond the last supported date). Once range validation is added for solar_to_lunar() / offset_to_lunar(), add pytest coverage for these boundaries to prevent regressions and ensure a clear error is raised.
| steps: | ||
| - uses: actions/checkout@v3 | ||
| - uses: actions/checkout@v4 | ||
| - name: Set up Python ${{ matrix.python-version }} | ||
| uses: actions/setup-python@v3 | ||
| uses: actions/setup-python@v5 | ||
| with: | ||
| python-version: ${{ matrix.python-version }} |
There was a problem hiding this comment.
The workflow YAML has incorrect indentation under steps:: the - uses: entries are aligned with steps: instead of being indented beneath it. This will make the workflow invalid YAML / fail to load. Indent the list items under steps: (e.g., two spaces deeper than steps:).
| def offset_to_lunar(offset: int) -> tuple[int, int, int, bool]: | ||
| """Convert a day offset from the lunar epoch start date into a lunar date tuple.""" | ||
| offset = int(offset) | ||
|
|
||
| for idx, year_days in enumerate(YEAR_DAYS): | ||
| if offset < year_days: | ||
| year = START_YEAR + idx | ||
| year_info = YEAR_INFOS[idx] | ||
| break | ||
| offset -= year_days | ||
| else: | ||
| raise ValueError("offset out of range") |
There was a problem hiding this comment.
offset_to_lunar() accepts negative offsets and will return an invalid lunar day (e.g., day <= 0) instead of raising. This can happen for solar dates before the epoch start (1900-01-31) via solar_to_lunar(), leading to confusing downstream errors. Add an explicit if offset < 0: raise ValueError(...) (and consider validating the upper bound in solar_to_lunar() similarly) so out-of-range solar dates fail fast with a clear message.
| __slots__ = ("_day", "_is_leap_month", "_month", "_year") | ||
|
|
||
| def __init__(self, year: int, month: int, day: int, is_leap_month: bool = False) -> None: | ||
| _conversions.validate_lunar_date(year, month, day, is_leap_month) | ||
| self._year = int(year) | ||
| self._month = int(month) | ||
| self._day = int(day) | ||
| self._is_leap_month = bool(is_leap_month) |
There was a problem hiding this comment.
The class is intended to behave like an immutable, hashable value type, but callers can still mutate the slotted private fields (e.g., instance._year = ...) after construction. That breaks the hashing contract (objects used as dict keys / set members can become unfindable) and undermines the immutability guarantee tested. Consider preventing post-init mutation (e.g., via a custom __setattr__ that only allows assignments during __init__) or otherwise making the object effectively immutable once created.
| __slots__ = ("_day", "_is_leap_month", "_month", "_year") | |
| def __init__(self, year: int, month: int, day: int, is_leap_month: bool = False) -> None: | |
| _conversions.validate_lunar_date(year, month, day, is_leap_month) | |
| self._year = int(year) | |
| self._month = int(month) | |
| self._day = int(day) | |
| self._is_leap_month = bool(is_leap_month) | |
| __slots__ = ("_day", "_is_leap_month", "_month", "_year", "_initialized") | |
| def __setattr__(self, name: str, value) -> None: | |
| # Allow attribute setting only during initialization to keep instances immutable. | |
| if getattr(self, "_initialized", False): | |
| raise AttributeError(f"{self.__class__.__name__} instances are immutable") | |
| object.__setattr__(self, name, value) | |
| def __init__(self, year: int, month: int, day: int, is_leap_month: bool = False) -> None: | |
| _conversions.validate_lunar_date(year, month, day, is_leap_month) | |
| self._year = int(year) | |
| self._month = int(month) | |
| self._day = int(day) | |
| self._is_leap_month = bool(is_leap_month) | |
| # Mark the instance as fully initialized; further mutations are disallowed. | |
| object.__setattr__(self, "_initialized", True) |
| def __add__(self, other: datetime.timedelta) -> LunarDate: | ||
| if isinstance(other, datetime.timedelta): | ||
| solar = self.to_solar_date() + other | ||
| return self.from_solar_date(solar.year, solar.month, solar.day) | ||
| raise TypeError | ||
|
|
||
| def __radd__(self, other: datetime.timedelta) -> LunarDate: | ||
| return self + other | ||
|
|
||
| def __sub__(self, other: object) -> datetime.timedelta | LunarDate: | ||
| if isinstance(other, LunarDate): | ||
| return self.to_solar_date() - other.to_solar_date() | ||
| if isinstance(other, datetime.date): | ||
| return self.to_solar_date() - other | ||
| if isinstance(other, datetime.timedelta): | ||
| solar = self.to_solar_date() - other | ||
| return self.from_solar_date(solar.year, solar.month, solar.day) | ||
| raise TypeError | ||
|
|
||
| def __rsub__(self, other: object) -> datetime.timedelta: | ||
| if isinstance(other, datetime.date): | ||
| return other - self.to_solar_date() | ||
| raise TypeError |
There was a problem hiding this comment.
Several operators raise a bare TypeError without a message (__add__, __sub__, __rsub__). This makes debugging harder and is inconsistent with the informative message used in __lt__. Prefer raising TypeError with a clear message indicating the unsupported operand types (e.g., what was provided vs what is supported).
No description provided.