Add grammar & converter for Christian holidays#162
Conversation
- Adapted from Hale/Eliot dataset code - Converted with help from OpenCode and Claude
WalkthroughThis PR extends the combined date parser to support Christian liturgical holidays and movable feasts. It introduces a new Changes
Sequence DiagramsequenceDiagram
participant User
participant HolidayDateConverter
participant LarkParser
participant HolidayTransformer
participant EasterCalc as convertdate.holidays
participant Undate
User->>HolidayDateConverter: parse("Easter 1916")
HolidayDateConverter->>LarkParser: Parse using holidays.lark grammar
LarkParser->>HolidayTransformer: Movable feast + year tokens
HolidayTransformer->>EasterCalc: easter(1916) for movable feast
EasterCalc-->>HolidayTransformer: Easter date (month, day)
HolidayTransformer->>HolidayTransformer: Apply feast offset<br/>(Easter+0 days)
HolidayTransformer->>Undate: Create with month, day, year
Undate-->>HolidayDateConverter: Undate object
HolidayDateConverter-->>User: Undate(month=4, day=3, year=1916)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #162 +/- ##
===========================================
- Coverage 99.00% 98.96% -0.04%
===========================================
Files 45 47 +2
Lines 2216 2327 +111
===========================================
+ Hits 2194 2303 +109
- Misses 22 24 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/undate/converters/holidays.py (2)
49-50: Pass the holiday calendar explicitly.
HolidayTransformerdeclarescalendar = Calendar.GREGORIAN, butholiday_date()currently relies onUndate's default by callingUndate(**parts). Passingcalendar=self.calendarhere makes the converter self-contained and avoids silent behavior changes if that default ever moves.♻️ Suggested tweak
def holiday_date(self, items): parts = self._get_date_parts(items) - return Undate(**parts) + return Undate(calendar=self.calendar, **parts)Also applies to: 75-77
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/undate/converters/holidays.py` around lines 49 - 50, HolidayTransformer sets calendar = Calendar.GREGORIAN but holiday_date() (and the similar block around the other occurrence) constructs Undate using Undate(**parts) which relies on Undate's default; update those constructors to pass the transformer's calendar explicitly (e.g., Undate(calendar=self.calendar, **parts)) so the converter is self-contained and won't change behavior if Undate's default calendar changes; locate this in the HolidayTransformer class and in the other conversion site referenced and add calendar=self.calendar to the Undate(...) calls.
10-12: Consider explicitly adding type stubs forconvertdateto improve type-checking transparency.While the CI workflow uses
mypy --install-types, which should automatically fetchtypes-convertdatefrom typeshed, adding it to thecheckoptional-dependencies group would make this requirement explicit and improve IDE/local development experience. You can either add"types-convertdate"to thechecklist inpyproject.toml, or rely on the auto-installation during CI—both approaches work, but explicit declaration is clearer for maintainability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/undate/converters/holidays.py` around lines 10 - 12, Add an explicit dependency on the convertdate type stubs so type-checking is deterministic: update your project's optional-dependencies "check" group in pyproject.toml to include "types-convertdate" (this relates to the import of convertdate/holidays in src/undate/converters/holidays.py), or document that CI relies on mypy's --install-types behavior; ensure the change is reflected in dependency metadata so IDEs and local mypy runs pick up the types for convertdate.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/undate/converters/grammars/holidays.lark`:
- Line 17: The grammar rule year: /\d{4}/ currently accepts "0000", which later
causes datetime.date(*holidays.easter(parts["year"])) in the movable-feast
resolver to raise ValueError; update the year rule to disallow "0000" (e.g. use
a regex that rejects all-zero year such as a negative lookahead or require the
first digit 1-9) so that inputs like "0000" fail during parsing rather than
during transformation.
---
Nitpick comments:
In `@src/undate/converters/holidays.py`:
- Around line 49-50: HolidayTransformer sets calendar = Calendar.GREGORIAN but
holiday_date() (and the similar block around the other occurrence) constructs
Undate using Undate(**parts) which relies on Undate's default; update those
constructors to pass the transformer's calendar explicitly (e.g.,
Undate(calendar=self.calendar, **parts)) so the converter is self-contained and
won't change behavior if Undate's default calendar changes; locate this in the
HolidayTransformer class and in the other conversion site referenced and add
calendar=self.calendar to the Undate(...) calls.
- Around line 10-12: Add an explicit dependency on the convertdate type stubs so
type-checking is deterministic: update your project's optional-dependencies
"check" group in pyproject.toml to include "types-convertdate" (this relates to
the import of convertdate/holidays in src/undate/converters/holidays.py), or
document that CI relies on mypy's --install-types behavior; ensure the change is
reflected in dependency metadata so IDEs and local mypy runs pick up the types
for convertdate.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: f81e7b2a-9863-49d0-a914-86a214cb91e9
📒 Files selected for processing (7)
src/undate/converters/combined.pysrc/undate/converters/grammars/combined.larksrc/undate/converters/grammars/holidays.larksrc/undate/converters/holidays.pysrc/undate/date.pytests/test_converters/test_combined_parser.pytests/test_converters/test_holidays.py
| // holidays that are always on the same date | ||
| fixed_date: EPIPHANY | CANDLEMASS | ST_PATRICKS | ALL_FOOLS | ST_CYPRIANS | ||
|
|
||
| year: /\d{4}/ |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
python - <<'PY'
import datetime
for year in (0, 1):
try:
print(year, datetime.date(year, 1, 1).isoformat())
except Exception as exc:
print(year, type(exc).__name__, exc)
PYRepository: dh-tech/undate-python
Length of output: 113
🏁 Script executed:
# Find and examine the holidays.py file
fd -t f holidays.pyRepository: dh-tech/undate-python
Length of output: 137
🏁 Script executed:
# Look at the resolver code around the mentioned lines
find . -name "holidays.py" -type f | head -5 | xargs wc -lRepository: dh-tech/undate-python
Length of output: 104
🏁 Script executed:
# Search for the resolver code that uses datetime.date
rg -A 5 -B 5 "datetime.date\|holidays.easter" --type pyRepository: dh-tech/undate-python
Length of output: 47
🏁 Script executed:
cat -n src/undate/converters/holidays.py | head -150Repository: dh-tech/undate-python
Length of output: 6947
🏁 Script executed:
python - <<'PY'
from convertdate import holidays
import datetime
# Test what happens when calling holidays.easter(0)
for year in (0, 1, 9999, 10000):
try:
easter_result = holidays.easter(year)
print(f"holidays.easter({year}) = {easter_result}")
# Now try to create datetime.date with it
try:
d = datetime.date(*easter_result)
print(f" datetime.date(*{easter_result}) = {d.isoformat()}")
except Exception as e:
print(f" datetime.date(*{easter_result}) raises {type(e).__name__}: {e}")
except Exception as e:
print(f"holidays.easter({year}) raises {type(e).__name__}: {e}")
PYRepository: dh-tech/undate-python
Length of output: 188
🏁 Script executed:
# Find project configuration files
fd -t f -e "toml" -e "txt" -e "cfg" | grep -i "setup\|requirements\|pyproject"Repository: dh-tech/undate-python
Length of output: 79
🏁 Script executed:
# Test datetime.date year boundaries
python - <<'PY'
import datetime
# Test valid year boundaries
test_years = [1, 9999, 10000, -1]
for year in test_years:
try:
d = datetime.date(year, 1, 1)
print(f"datetime.date({year}, 1, 1) = {d.isoformat()}")
except Exception as e:
print(f"datetime.date({year}, 1, 1) raises {type(e).__name__}: {e}")
PYRepository: dh-tech/undate-python
Length of output: 279
🏁 Script executed:
cat -n src/undate/converters/grammars/holidays.larkRepository: dh-tech/undate-python
Length of output: 1407
Reject 0000 before it reaches the movable-feast calculation.
The grammar currently allows year: /\d{4}/ to match 0000, but the movable-feast resolver at line 127 uses datetime.date(*holidays.easter(parts["year"])), which only supports years 1–9999. Parsing inputs like Easter 0000 will therefore result in a ValueError at transformation time instead of a normal parse failure.
Grammar fix
-year: /\d{4}/
+year: /(?!0000)\d{4}/📝 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.
| year: /\d{4}/ | |
| year: /(?!0000)\d{4}/ |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/undate/converters/grammars/holidays.lark` at line 17, The grammar rule
year: /\d{4}/ currently accepts "0000", which later causes
datetime.date(*holidays.easter(parts["year"])) in the movable-feast resolver to
raise ValueError; update the year rule to disallow "0000" (e.g. use a regex that
rejects all-zero year such as a negative lookahead or require the first digit
1-9) so that inputs like "0000" fail during parsing rather than during
transformation.
1 participant
Adds a new grammar and converter for Christian holidays; both standalone and omnibus.
Adapted from code originally written from Hale/Eliot dataset project; converted & updated with help from OpenCode and Claude
Summary by CodeRabbit
New Features
Tests