Add grammar & converter for Christian holidays#162
Conversation
- Adapted from Hale/Eliot dataset code - Converted with help from OpenCode and Claude
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 10 minutes and 57 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughThis PR adds Christian liturgical holiday parsing: a new holidays grammar and HolidayDateConverter, integrates it into the combined parser, adds a Weekday IntEnum and minor Date.weekday change, and provides comprehensive tests for fixed and movable feast parsing and behavior. Changes
Sequence DiagramsequenceDiagram
participant User as User
participant Converter as HolidayDateConverter
participant Parser as LarkParser
participant Transformer as HolidayTransformer
participant EasterCalc as convertdate.holidays
participant Undate as Undate
User->>Converter: parse("Easter 1916")
Converter->>Parser: parse input with holidays.lark
Parser->>Transformer: produce parse tree (movable_feast + year)
Transformer->>EasterCalc: easter(1916)
EasterCalc-->>Transformer: (month, day)
Transformer->>Transformer: apply feast offset
Transformer->>Undate: construct Undate(year, month, day)
Undate-->>Converter: result
Converter-->>User: return Undate
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #162 +/- ##
===========================================
+ Coverage 99.00% 99.06% +0.05%
===========================================
Files 45 47 +2
Lines 2216 2356 +140
===========================================
+ Hits 2194 2334 +140
Misses 22 22 ☔ 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.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
src/undate/converters/holidays.py (2)
55-55: Remove commented-out code.Several blocks of dead/commented-out code remain in
HolidayTransformer(line 55 inyear, lines 65–66 and 69–73 infixed_date). Since a cleanup commit is already part of this PR, these should be dropped to keep the module tidy and avoid reader confusion about intent.🧹 Proposed cleanup
def year(self, items): value = "".join([str(i) for i in items]) return Token("year", value) - # return Tree(data="year", children=[value])def fixed_date(self, items): item = items[0] holiday_name = item.type.split("__")[-1] - # token_type = item.type - # token type is holiday fixed-date name; use to determine month/day month, day = FIXED_HOLIDAYS.get(holiday_name) return Tree("fixed_date", [Token("month", month), Token("day", day)]) - # for key in FIXED_HOLIDAYS: - # if token_type == key or token_type == f"holidays__{key}": - # month, day = FIXED_HOLIDAYS[key] - # return Tree("fixed_date", [Token("month", month), Token("day", day)]) - # raise ValueError(f"Unknown fixed holiday: {item.type}")Also applies to: 65-73
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/undate/converters/holidays.py` at line 55, Remove the leftover commented-out code blocks inside the HolidayTransformer methods: delete the commented "return Tree(data=\"year\", children=[value])" inside the year method and remove the commented lines in fixed_date (the blocks around the earlier commented logic and any commented returns). Keep the active logic untouched—only delete the dead/commented code in HolidayTransformer.year and HolidayTransformer.fixed_date so the module is tidy and behavior is unchanged.
112-114: Redundantstr(field)cast.
fieldis assigned fromchild.type, which is already astr. The cast is a no-op.♻️ Proposed cleanup
- if field and value: - # currently all date parts are integer only - parts[str(field)] = int(value) + if field and value: + # currently all date parts are integer only + parts[field] = int(value)🤖 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 112 - 114, Remove the redundant str() cast when assigning to the parts dict: in the block where field is set from child.type and you do parts[str(field)] = int(value), change it to parts[field] = int(value) (since field is already a str); update any nearby comments if needed and run relevant unit tests for the converter (functions/variables: field, child.type, parts) to ensure no behavior change.
🤖 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/holidays.py`:
- Around line 22-28: Rename the misspelled holiday key "CANDLEMASS" to
"CANDLEMAS" in the FIXED_HOLIDAYS dict and update all corresponding references
(grammar terminal in holidays.lark and any usages in code, tests, or parser
rules) to the single‑s spelling so the terminal name and dict key match; search
for "CANDLEMASS" across the repo and replace with "CANDLEMAS", then run relevant
tests/parsing to ensure no remaining references break.
- Around line 117-122: The branch handling movable_feast currently accesses
parts["year"] and raises a cryptic KeyError when year is missing; update the
movable feast handling (the code using movable_feast, MOVEABLE_FEASTS,
holidays.easter, and parts.update) to first check for a year in parts and either
raise a clear ValueError (e.g., "movable feast requires a year") or return a
partial Undate according to project semantics; ensure parse() still receives a
consistent exception type by documenting/wrapping this new error behavior (or
re-raising as ValueError) so consumers of parse() see the advertised ValueError
rather than KeyError.
- Around line 62-73: The fixed_date method calls
FIXED_HOLIDAYS.get(holiday_name) and unpacks its result without checking for
None, which causes an opaque TypeError when a terminal is missing; change
fixed_date (in the fixed_date method) to validate the lookup result from
FIXED_HOLIDAYS.get(holiday_name), and if None raise a clear ValueError
(including item.type and holiday_name in the message) so callers get a helpful
error, otherwise continue to return Tree("fixed_date", [Token("month", month),
Token("day", day)]) as before.
- Line 10: The mypy failure is caused by missing type stubs for the third‑party
module imported in the holidays.py file; fix it by either appending a per‑import
mypy ignore to the import statement (the line reading "from convertdate import
holidays") so mypy skips that specific import, or add a mypy config override
that targets module "convertdate" and "convertdate.*" with
ignore_missing_imports = true to silence missing stubs globally for that
package.
---
Nitpick comments:
In `@src/undate/converters/holidays.py`:
- Line 55: Remove the leftover commented-out code blocks inside the
HolidayTransformer methods: delete the commented "return Tree(data=\"year\",
children=[value])" inside the year method and remove the commented lines in
fixed_date (the blocks around the earlier commented logic and any commented
returns). Keep the active logic untouched—only delete the dead/commented code in
HolidayTransformer.year and HolidayTransformer.fixed_date so the module is tidy
and behavior is unchanged.
- Around line 112-114: Remove the redundant str() cast when assigning to the
parts dict: in the block where field is set from child.type and you do
parts[str(field)] = int(value), change it to parts[field] = int(value) (since
field is already a str); update any nearby comments if needed and run relevant
unit tests for the converter (functions/variables: field, child.type, parts) to
ensure no behavior change.
🪄 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: 16e4af3b-4969-417d-968d-f652ee7960a2
📒 Files selected for processing (1)
src/undate/converters/holidays.py
| FIXED_HOLIDAYS = { | ||
| "EPIPHANY": (1, 6), # January 6 | ||
| "CANDLEMASS": (2, 2), # February 2; 40th day & end of epiphany | ||
| "ST_PATRICKS": (3, 17), # March 17 | ||
| "ALL_FOOLS": (4, 1), # All / April fools day, April 1 | ||
| "ST_CYPRIANS": (9, 16), # St. Cyprian's Feast day: September 16 | ||
| } |
There was a problem hiding this comment.
Typo: "CANDLEMASS" should be "CANDLEMAS".
The feast is spelled Candlemas (single "s"). This terminal name is likely mirrored in holidays.lark, so both the grammar terminal and this key need updating to match.
✏️ Proposed fix
FIXED_HOLIDAYS = {
"EPIPHANY": (1, 6), # January 6
- "CANDLEMASS": (2, 2), # February 2; 40th day & end of epiphany
+ "CANDLEMAS": (2, 2), # February 2; 40th day & end of epiphany
"ST_PATRICKS": (3, 17), # March 17📝 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.
| FIXED_HOLIDAYS = { | |
| "EPIPHANY": (1, 6), # January 6 | |
| "CANDLEMASS": (2, 2), # February 2; 40th day & end of epiphany | |
| "ST_PATRICKS": (3, 17), # March 17 | |
| "ALL_FOOLS": (4, 1), # All / April fools day, April 1 | |
| "ST_CYPRIANS": (9, 16), # St. Cyprian's Feast day: September 16 | |
| } | |
| FIXED_HOLIDAYS = { | |
| "EPIPHANY": (1, 6), # January 6 | |
| "CANDLEMAS": (2, 2), # February 2; 40th day & end of epiphany | |
| "ST_PATRICKS": (3, 17), # March 17 | |
| "ALL_FOOLS": (4, 1), # All / April fools day, April 1 | |
| "ST_CYPRIANS": (9, 16), # St. Cyprian's Feast day: September 16 | |
| } |
🤖 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 22 - 28, Rename the
misspelled holiday key "CANDLEMASS" to "CANDLEMAS" in the FIXED_HOLIDAYS dict
and update all corresponding references (grammar terminal in holidays.lark and
any usages in code, tests, or parser rules) to the single‑s spelling so the
terminal name and dict key match; search for "CANDLEMASS" across the repo and
replace with "CANDLEMAS", then run relevant tests/parsing to ensure no remaining
references break.
Also fix spelling of moveable (movable) and Candlemas (Candlemass)
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