Conversation
|
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 18 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 (2)
WalkthroughInfrastructure modernization across CI/CD pipelines and dependency management. Migrates from pip/setup-python to uv package manager, replaces PEP 621 optional-dependencies with Hatch dependency-groups in pyproject.toml, adds workflow concurrency control, updates pytest configuration structure, enhances pre-commit hooks, and corrects spelling in comments and test data. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/unit_tests.yml (1)
32-54:⚠️ Potential issue | 🔴 Critical
uvis never installed in the job — workflow will fail.The steps invoke
uv sync --extra test(line 44) anduv run pytest(lines 48, 53), but the job only runsactions/checkoutandactions/setup-python. There is noastral-sh/setup-uvstep, souvwon't be available and these commands will fail immediately.Also:
- The
cache: 'pip'configuration onsetup-python(lines 40–41) is incompatible withuv. Sinceuvmaintains its own cache and doesn't populate pip's wheel cache, these settings have no effect. Useastral-sh/setup-uvwithenable-cache: trueinstead.- The comment on line 35 ("use github python action instead of uv to take advantage of caching") contradicts the new run commands and should be removed.
🔧 Suggested fix — install uv before running it
- uses: actions/checkout@v5 - # use github python action instead of uv to take advantage of caching - name: Set up Python ${{ matrix.python }} uses: actions/setup-python@v6 with: python-version: ${{ matrix.python }} - cache: 'pip' - cache-dependency-path: '**/pyproject.toml' + + - name: Install uv + uses: astral-sh/setup-uv@v6 + with: + enable-cache: true + cache-dependency-glob: '**/pyproject.toml' - name: Install package with check dependencies run: uv sync --extra test🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/unit_tests.yml around lines 32 - 54, Add an installation step for uv before any uv commands: insert a step using the astral-sh/setup-uv action (e.g., uses: astral-sh/setup-uv with enable-cache: true) prior to running "uv sync --extra test" and "uv run pytest", and remove or update the misleading comment about using actions/setup-python instead of uv; also remove or stop using cache: 'pip' on actions/setup-python since uv manages its own cache, and keep the existing conditional test runs that reference env.COV_PYTHON_VERSION unchanged.
🧹 Nitpick comments (16)
CHANGELOG.md (1)
4-8: Consider mentioning the new Holidays converter in the0.7entry.The PR adds a full
HolidayDateConverterplus grammar and tests (fixed-date + movable feasts), and a newWeekdayenum onundate.date. These are user-visible additions worth a bullet here so downstream users notice them at release time.📝 Suggested addition
## 0.7 - Add parsing to Gregorian date converter; supports month names (full or abbreviated) in English, French, German, Spanish, Kinyarwanda, Ganda, and Tigrinya - Include Gregorian dates in omnibus parser +- New `HolidayDateConverter` for parsing Christian liturgical dates, including + fixed-date holidays and Easter-relative movable feasts +- New `Weekday` IntEnum in `undate.date`, compatible with `datetime.date.weekday`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CHANGELOG.md` around lines 4 - 8, Update the 0.7 CHANGELOG entry to mention the new HolidayDateConverter and the Weekday enum: add a bullet noting the addition of HolidayDateConverter (with grammar and tests for fixed-date and movable feasts) and another bullet that the new Weekday enum was added to undate.date so downstream users are aware of these user-visible API additions..github/workflows/check.yml (1)
20-28: Intentional divergence fromunit_tests.yml— worth a one-line comment.
unit_tests.ymlstill usesactions/setup-python@v6(with an inline comment explaining pip caching), while this workflow now usesastral-sh/setup-uv@v6for Python selection. The split looks deliberate, but a brief comment here (mirroring the one inunit_tests.yml) would make the rationale obvious to future maintainers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/check.yml around lines 20 - 28, Add a one-line comment above the "Install uv" step explaining why this workflow uses astral-sh/setup-uv@v6 instead of actions/setup-python@v6 (e.g., to enable uv-managed dependency caching and the uv sync workflow, mirroring the explanatory comment in unit_tests.yml) so readers immediately understand the intentional divergence between the "Install uv" and "Install package with check dependencies" steps and the unit_tests.yml usage.src/undate/date.py (2)
247-266: Optional: returnWeekdayinstead of bareint.Since
Weekdayis now defined andIntEnumvalues compare/coerce as plainint(so existing tests that compare againstdatetime.date.weekday()and integer literals still pass), returningWeekday((days_from_thursday + 3) % 7)would give callers self-describing values and keep consistency with the new enum. Only the return annotation would need to widen toOptional[Weekday]. Skip if you prefer to keep the enum strictly as a caller-side convenience.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/undate/date.py` around lines 247 - 266, The weekday property currently returns a plain int; change its return annotation from Optional[int] to Optional[Weekday] and return a Weekday instance instead of an int by wrapping the computed value with Weekday(...). Update the return expression in the weekday property (the calculation using thursday_week and days_from_thursday) to return Weekday((days_from_thursday + 3) % 7) and ensure Weekday is imported or available in this module so the property returns the enum value while remaining compatible with int comparisons.
283-293: Consider exportingWeekdayfrom the package root.
Weekdayis a useful public symbol (already consumed in tests such astests/test_converters/test_holidays.pyviaWeekday.SUNDAY, etc.), but it is not re-exported fromsrc/undate/__init__.pyalongsideDatePrecisionandUnDelta. Adding it to the top-level import and__all__would make it a first-class API surface.♻️ Suggested change in
src/undate/__init__.py-from undate.date import DatePrecision, UnDelta +from undate.date import DatePrecision, UnDelta, Weekday from undate.undate import Undate, Calendar from undate.interval import UndateInterval __all__ = [ "Undate", "UndateInterval", "Calendar", "DatePrecision", "UnDelta", + "Weekday", "__version__", ]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/undate/date.py` around lines 283 - 293, Weekday is a public symbol used by tests but isn’t re-exported from the package root; update the package __init__ to import Weekday from its module (the Weekday IntEnum) alongside DatePrecision and UnDelta and add "Weekday" to the module's __all__ so callers can access Weekday.SUNDAY etc. directly from the package root.src/undate/converters/grammars/islamic.lark (1)
38-39: Deadcommarule now that punctuation is globally ignored.With
%ignore DATE_PUNCTUATION(which includes,) now active, thecommaterminal inweekdayis unreachable — commas are stripped during lexing before they can matchcomma?. The Hebrew grammar was simplified to drop the explicitcommain the equivalent refactor; consider mirroring that here for consistency.♻️ Proposed simplification
-comma: "," -weekday: ("Monday" | "Tuesday" | "Wednesday" | "Thursday" | "Friday" | "Saturday" | "Sunday") comma? +weekday: "Monday" | "Tuesday" | "Wednesday" | "Thursday" | "Friday" | "Saturday" | "Sunday"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/undate/converters/grammars/islamic.lark` around lines 38 - 39, The weekday rule currently references the terminal "comma" which is unreachable because DATE_PUNCTUATION (including ",") is ignored; update the grammar by removing the optional comma from the weekday rule (change weekday to ("Monday" | "Tuesday" | ... | "Sunday") without comma?) and delete the now-dead "comma" terminal definition so there are no unused terminals; ensure DATE_PUNCTUATION remains the mechanism that strips punctuation.src/undate/converters/combined.py (1)
84-87: Preserve the original parse error via exception chaining.All sibling converters (
GregorianDateConverter,HebrewDateConverter,IslamicDateConverter,HolidayConverter) useexcept UnexpectedInput as err: ... raise ValueError(...) from err. The omnibus path is arguably the one where the underlying Lark error is most useful for diagnosis (multiple grammars contribute), so it's worth keeping the chain explicit here rather than relying on implicit “during handling of” chaining.♻️ Proposed change
- except UnexpectedInput: - raise ValueError( - "Parsing failed: '%s' is not in a recognized date format" % value - ) + except UnexpectedInput as err: + raise ValueError( + f"Parsing failed: '{value}' is not in a recognized date format" + ) from err🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/undate/converters/combined.py` around lines 84 - 87, The except block catching UnexpectedInput should preserve the original parse error via exception chaining: in the handler in combined.py (the except UnexpectedInput block used by the combined converter) bind the exception (e.g., `except UnexpectedInput as err:`) and re-raise the ValueError with `from err` so the original Lark parse error is chained (consistent with GregorianDateConverter, HebrewDateConverter, IslamicDateConverter, HolidayConverter).src/undate/converters/grammars/gregorian.lark (1)
20-20: Nit:yearregex allows arbitrarily long integers.
/\b\d{3,}\b/will happily match a 20-digit "year". Not a correctness bug (downstreamUndatewill likely balk on truly absurd values), but if you want to keep the parser honest you could cap it (e.g.\d{3,5}or similar), or leave a comment noting that year-range validation happens downstream.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/undate/converters/grammars/gregorian.lark` at line 20, The grammar rule year currently uses /\b\d{3,}\b/ which permits arbitrarily long integers; update the year rule (symbol: year) to cap digit length (for example change to a bounded regex such as /\b\d{3,5}\b/ or another appropriate range) to prevent absurdly long matches, or alternatively add a clear comment immediately above the year rule stating that year-range validation is intentionally deferred to downstream Undate logic; apply one of these fixes to keep the parser's intent explicit.src/undate/converters/holidays.py (3)
165-166: PreferNotImplementedErroroverValueErrorfor unsupported serialization.Python convention reserves
NotImplementedErrorfor "this operation is intentionally not supported by this subclass" — it makes the intent clearer to callers doingtry/exceptaround generic conversion paths than a genericValueErrormessage.- def to_string(self, undate: Undate) -> str: - raise ValueError("Holiday converter does not support serialization") + def to_string(self, undate: Undate) -> str: + raise NotImplementedError("Holiday converter does not support serialization")🤖 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 165 - 166, The to_string method in the holidays converter currently raises ValueError; change it to raise NotImplementedError instead to signal intentional unsupported serialization. Locate the to_string(self, undate: Undate) method in src/undate/converters/holidays.py and replace the raised exception so it raises NotImplementedError("Holiday converter does not support serialization") (preserving the message) to follow Python conventions for unsupported subclass operations.
52-55: Remove dead commented-out return.Line 55's commented-out
return Tree(...)is a leftover from iteration; since the active implementation usesToken, the comment is just noise.def year(self, items): value = "".join([str(i) for i in items]) return Token("year", value) - # return Tree(data="year", children=[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 52 - 55, The commented-out legacy return in the year function is dead code and should be removed; in the year(items) method replace/remove the inactive comment line "return Tree(...)" so the function only builds the value and returns Token("year", value) (refer to the year function and the commented `return Tree(data="year", children=[value])` for location).
118-119: Chain the exception foryear-missing movable feasts.Same pattern as line 70 — converting
KeyErrortoValueErrorinside anexceptblock withoutfrom errorfrom Noneproduces a noisy double traceback.- except KeyError: - raise ValueError("Year is required for movable feasts") + except KeyError as err: + raise ValueError("Year is required for movable feasts") from err🤖 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 118 - 119, The except KeyError block that currently does "except KeyError: raise ValueError('Year is required for movable feasts')" should capture the original exception (e.g., "except KeyError as err:") and re-raise the ValueError using exception chaining ("raise ValueError('Year is required for movable feasts') from err") so the original KeyError context is preserved; update the handler in the movable feasts code path accordingly (mirror the pattern used at the earlier handling on line 70).src/undate/converters/calendars/gregorian/transformer.py (1)
31-37: Minor robustness: casttree.datatostrbefore splitting.In recent Lark versions,
Tree.datacan be aTokeninstance rather than a plain string..splitstill works, but being explicit avoids surprises (and matches thestr(child.data)used ingregorian_dateon line 20).- tree = items[0] - month_n = tree.data.split("_")[-1] - return Tree(data="month", children=[month_n]) + tree = items[0] + month_n = str(tree.data).split("_")[-1] + return Tree(data="month", children=[month_n])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/undate/converters/calendars/gregorian/transformer.py` around lines 31 - 37, The month method uses tree.data.split("_") but tree.data can be a Token; cast it to str first to match the style used in gregorian_date: replace the split call with str(tree.data).split("_") and ensure the extracted month_n (as a string) is passed into the returned Tree (Tree(data="month", children=[month_n])) so the code is robust against Token instances.pyproject.toml (1)
82-86: Nit: pinbabelfor reproducible codegen.Since
gregorian_multilang.larkis a generated artifact committed to source, the month-name set is effectively tied to a Babel CLDR snapshot. Pinningbabel(or at least a floor) in thecodegenenv makes regenerations reproducible across contributors and CI.[tool.hatch.envs.codegen] -dependencies = ["babel"] +dependencies = ["babel>=2.14"]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pyproject.toml` around lines 82 - 86, The codegen hatch env currently lists dependencies = ["babel"] which is unpinned; update the [tool.hatch.envs.codegen] dependencies to pin Babel to a specific version (or at least a minimum version) to ensure reproducible generation of gregorian_multilang.lark; change the "babel" entry in dependencies for the codegen env so it includes a version specifier (e.g., "babel>=X.Y.Z" or "babel==X.Y.Z") and keep the generate script entry (generate = "python scripts/generate_gregorian_grammar.py") unchanged.scripts/generate_gregorian_grammar.py (2)
58-67: Iterate months 1..12 explicitly instead of relying on dict insertion order.
all_month_namesis adefaultdict(list); the order and completeness of.items()depend entirely on what Babel yields for the first locale(s). If a future locale is added that doesn't include all 12 months (or yields them in a different order), the generated.larkfile will silently emit rules out of order or drop months — the Gregorian grammar import ofmonth_1..month_12would then break at parser-build time. Iterating explicitly overrange(1, 13)also validates coverage.♻️ Proposed change
- # for each numeric month, generate a rule with all variant names: - # month_1: /January|Jan/i - for i, names in all_month_names.items(): - # combine all names in a case-insensitive OR regex - # sort shortest variants last to avoid partial matches hitting first - or_names = "|".join(sorted(names, key=len, reverse=True)) - outfile.write(f"month_{i}: /({or_names})/i\n") + # for each numeric month, generate a rule with all variant names: + # month_1: /January|Jan/i + for i in range(1, 13): + names = all_month_names.get(i) + if not names: + raise RuntimeError(f"No month names collected for month {i}") + # combine all names in a case-insensitive OR regex + # longest variants first so partial matches don't win + or_names = "|".join(sorted(names, key=len, reverse=True)) + outfile.write(f"month_{i}: /({or_names})/i\n")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/generate_gregorian_grammar.py` around lines 58 - 67, The loop that writes month rules currently iterates over all_month_names.items() which can miss or reorder months; change it to explicitly iterate for i in range(1, 13) inside the MONTH_GRAMMAR_FILE.open(...) block, retrieve names = all_month_names.get(i) and validate presence (raise a clear exception or log+exit if names is missing or empty) before generating the OR regex and calling outfile.write(f"month_{i}: /({or_names})/i\n"); keep the same sorting of names by length (key=len, reverse=True) and reuse warning_text and outfile.write as-is.
66-67: Consider escaping regex metacharacters in month aliases.Month names are written directly into a regex alternation. Today's seven locales only produce letters and
.(which is stripped), so this works, but adding a locale whose abbreviation contains a regex metacharacter (e.g.,(,),*,+,/,\) would silently generate a malformed or mis-matching pattern. A one-linere.escapeon each name closes this off.- or_names = "|".join(sorted(names, key=len, reverse=True)) + or_names = "|".join(re.escape(n) for n in sorted(names, key=len, reverse=True))(requires
import reat the top.)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/generate_gregorian_grammar.py` around lines 66 - 67, The regex alternation built for month names (variable or_names used when writing month_{i} in the outfile.write call) should escape regex metacharacters in each alias; update the construction of or_names to apply re.escape to every name (e.g., map or generator using re.escape over names) and keep the existing sorting by length, and add import re at the top of the file so re.escape is available.tests/test_converters/test_holidays.py (2)
79-82: Orphaned comment placement.The comment
# edge cases - should not happen from parser input but possiblesits between the two test classes with blank lines on both sides, so it reads as a standalone remark rather than documentation forTestHolidayTransformer. Consider moving it inside the class (or into its docstring) to make the intent explicit.✏️ Suggested change
-# edge cases - should not happen from parser input but possible - - class TestHolidayTransformer: + """Edge cases — should not happen from parser input but possible.""" + def test_fixed_date(self):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_converters/test_holidays.py` around lines 79 - 82, The orphaned comment should be moved so its intent is clearly associated with the TestHolidayTransformer test class: relocate the line "# edge cases - should not happen from parser input but possible" inside the TestHolidayTransformer class body (or convert it into a class docstring) and remove the extra blank lines so the comment/docstring sits directly under the class definition; update TestHolidayTransformer accordingly so the comment documents that class's edge-case tests.
45-45: Nit: spelling "moveable" vs "movable".Test names use
moveable, while the PR's AI summary and the more standard English spelling ismovable. Worth aligning with whatever spelling is used in the converter/grammar (movable_feastper the summary) so the test names mirror the production symbol.✏️ Suggested rename
- def test_moveable_feasts(self, input_string, expected, expected_weekday): + def test_movable_feasts(self, input_string, expected, expected_weekday):- def test_moveable_without_year(self): + def test_movable_without_year(self):Also applies to: 70-70
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_converters/test_holidays.py` at line 45, Rename the test function and any related references from the nonstandard spelling test_moveable_feasts to test_movable_feasts so the test name matches the production symbol movable_feast; update the function definition and all occurrences (e.g., parametrization, fixtures or calls) to the new name to keep tests and grammar aligned.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@DEVELOPER_NOTES.md`:
- Around line 46-48: Replace the invalid pip command string "pip install -e
--group=dev" with a valid, compatible installation command: either supply a
target for -e (e.g., "pip install -e . --group=dev") if you require editable
install and a pip ≥ 25.1, or use the legacy extras form for broader
compatibility such as "pip install -e .[dev]" (or "pip install .[dev]" if not
editable). Also consider adding a note about pip version requirements if you
keep the --group form.
In `@pyproject.toml`:
- Around line 53-66: Update the workflow invocations that still use the old
extras flag: replace any occurrence of the command string "uv sync --extra test"
with "uv sync --group test" and replace "uv sync --extra check" with "uv sync
--group check" (and similarly change any other "uv sync --extra <name>" usages
to "uv sync --group <name>") so the CI uses PEP 735 dependency groups defined in
[dependency-groups] (look for the exact command lines in the workflow YAMLs).
In `@scripts/generate_gregorian_grammar.py`:
- Around line 69-71: The success print uses
MONTH_GRAMMAR_FILE.relative_to(pathlib.Path.cwd()) which will raise ValueError
if the script is run from a different cwd; change the print to safely resolve
the path by attempting MONTH_GRAMMAR_FILE.relative_to(pathlib.Path.cwd()) inside
a try/except and falling back to str(MONTH_GRAMMAR_FILE) (absolute/path) on
exception, or simply print the absolute path directly — update the print
statement that references MONTH_GRAMMAR_FILE to use this safe fallback.
In `@src/undate/converters/grammars/combined.lark`:
- Around line 43-45: The combined grammar allows a bare year to be parsed via
both edtf__start and gregorian__gregorian_date because the override for
gregorian__gregorian_date is commented out; either uncomment/enable the override
line ("%override gregorian__gregorian_date: day month year | month day year |
year month day | month year | year month | day month | month day") so
gregorian_date no longer exposes the bare year alternative, or delete the
commented-out override entirely to avoid shipping dead config; update the
grammar so the start rule (which accepts edtf__start and
gregorian__gregorian_date) has an unambiguous gregorian path.
In `@src/undate/converters/grammars/gregorian.lark`:
- Line 13: The grammar currently allows a bare year via gregorian_date which
conflicts with edtf__start for inputs like "1984"; either uncomment the
Gregorian override that removes the year-only alternative from
gregorian__gregorian_date (matching the explicit overrides used for
Hebrew/Islamic) so year-only inputs are parsed by edtf__start, or add a clear
comment next to start/edtf__start explaining why relying on alternation order is
safe; update the gregorian.lark rules (gregorian_date and
gregorian__gregorian_date) accordingly to remove the ambiguous "year"
alternative or document the intentional overlap.
In `@src/undate/converters/grammars/holidays.lark`:
- Line 17: The year lexer rule `year` in `holidays.lark` currently uses
`/\d{4}/` which can match the first four digits of longer digit sequences;
update the `year` pattern to include word boundaries (use `/\b\d{4}\b/`) so only
standalone four-digit years are matched, preventing partial matches like `12345`
being parsed as `1234` + `5` and causing downstream errors.
In `@src/undate/converters/holidays.py`:
- Around line 62-71: In fixed_date, make the KeyError handling explicit and make
Tokens carry strings: when catching the KeyError from FIXED_HOLIDAYS lookup in
fixed_date, re-raise the ValueError using exception chaining (e.g., raise
ValueError(...) from err) or suppress chaining (from None) as appropriate; and
construct Token("month", str(month)) and Token("day", str(day)) so Token.value
is a string (matching Lark conventions and downstream _get_date_parts that calls
int(value)).
- Around line 109-112: The condition "if field and value:" incorrectly drops
legitimate zero values; update the check in the block that reads child.value
(the code that populates parts[str(field)] = int(value)) to use explicit None
(and empty-string) checks — e.g. only skip when field is None or value is None
or value == '' — so numeric zero (0 or "0") is preserved before calling
int(value); keep using parts, field, value and the same integer conversion.
---
Outside diff comments:
In @.github/workflows/unit_tests.yml:
- Around line 32-54: Add an installation step for uv before any uv commands:
insert a step using the astral-sh/setup-uv action (e.g., uses:
astral-sh/setup-uv with enable-cache: true) prior to running "uv sync --extra
test" and "uv run pytest", and remove or update the misleading comment about
using actions/setup-python instead of uv; also remove or stop using cache: 'pip'
on actions/setup-python since uv manages its own cache, and keep the existing
conditional test runs that reference env.COV_PYTHON_VERSION unchanged.
---
Nitpick comments:
In @.github/workflows/check.yml:
- Around line 20-28: Add a one-line comment above the "Install uv" step
explaining why this workflow uses astral-sh/setup-uv@v6 instead of
actions/setup-python@v6 (e.g., to enable uv-managed dependency caching and the
uv sync workflow, mirroring the explanatory comment in unit_tests.yml) so
readers immediately understand the intentional divergence between the "Install
uv" and "Install package with check dependencies" steps and the unit_tests.yml
usage.
In `@CHANGELOG.md`:
- Around line 4-8: Update the 0.7 CHANGELOG entry to mention the new
HolidayDateConverter and the Weekday enum: add a bullet noting the addition of
HolidayDateConverter (with grammar and tests for fixed-date and movable feasts)
and another bullet that the new Weekday enum was added to undate.date so
downstream users are aware of these user-visible API additions.
In `@pyproject.toml`:
- Around line 82-86: The codegen hatch env currently lists dependencies =
["babel"] which is unpinned; update the [tool.hatch.envs.codegen] dependencies
to pin Babel to a specific version (or at least a minimum version) to ensure
reproducible generation of gregorian_multilang.lark; change the "babel" entry in
dependencies for the codegen env so it includes a version specifier (e.g.,
"babel>=X.Y.Z" or "babel==X.Y.Z") and keep the generate script entry (generate =
"python scripts/generate_gregorian_grammar.py") unchanged.
In `@scripts/generate_gregorian_grammar.py`:
- Around line 58-67: The loop that writes month rules currently iterates over
all_month_names.items() which can miss or reorder months; change it to
explicitly iterate for i in range(1, 13) inside the MONTH_GRAMMAR_FILE.open(...)
block, retrieve names = all_month_names.get(i) and validate presence (raise a
clear exception or log+exit if names is missing or empty) before generating the
OR regex and calling outfile.write(f"month_{i}: /({or_names})/i\n"); keep the
same sorting of names by length (key=len, reverse=True) and reuse warning_text
and outfile.write as-is.
- Around line 66-67: The regex alternation built for month names (variable
or_names used when writing month_{i} in the outfile.write call) should escape
regex metacharacters in each alias; update the construction of or_names to apply
re.escape to every name (e.g., map or generator using re.escape over names) and
keep the existing sorting by length, and add import re at the top of the file so
re.escape is available.
In `@src/undate/converters/calendars/gregorian/transformer.py`:
- Around line 31-37: The month method uses tree.data.split("_") but tree.data
can be a Token; cast it to str first to match the style used in gregorian_date:
replace the split call with str(tree.data).split("_") and ensure the extracted
month_n (as a string) is passed into the returned Tree (Tree(data="month",
children=[month_n])) so the code is robust against Token instances.
In `@src/undate/converters/combined.py`:
- Around line 84-87: The except block catching UnexpectedInput should preserve
the original parse error via exception chaining: in the handler in combined.py
(the except UnexpectedInput block used by the combined converter) bind the
exception (e.g., `except UnexpectedInput as err:`) and re-raise the ValueError
with `from err` so the original Lark parse error is chained (consistent with
GregorianDateConverter, HebrewDateConverter, IslamicDateConverter,
HolidayConverter).
In `@src/undate/converters/grammars/gregorian.lark`:
- Line 20: The grammar rule year currently uses /\b\d{3,}\b/ which permits
arbitrarily long integers; update the year rule (symbol: year) to cap digit
length (for example change to a bounded regex such as /\b\d{3,5}\b/ or another
appropriate range) to prevent absurdly long matches, or alternatively add a
clear comment immediately above the year rule stating that year-range validation
is intentionally deferred to downstream Undate logic; apply one of these fixes
to keep the parser's intent explicit.
In `@src/undate/converters/grammars/islamic.lark`:
- Around line 38-39: The weekday rule currently references the terminal "comma"
which is unreachable because DATE_PUNCTUATION (including ",") is ignored; update
the grammar by removing the optional comma from the weekday rule (change weekday
to ("Monday" | "Tuesday" | ... | "Sunday") without comma?) and delete the
now-dead "comma" terminal definition so there are no unused terminals; ensure
DATE_PUNCTUATION remains the mechanism that strips punctuation.
In `@src/undate/converters/holidays.py`:
- Around line 165-166: The to_string method in the holidays converter currently
raises ValueError; change it to raise NotImplementedError instead to signal
intentional unsupported serialization. Locate the to_string(self, undate:
Undate) method in src/undate/converters/holidays.py and replace the raised
exception so it raises NotImplementedError("Holiday converter does not support
serialization") (preserving the message) to follow Python conventions for
unsupported subclass operations.
- Around line 52-55: The commented-out legacy return in the year function is
dead code and should be removed; in the year(items) method replace/remove the
inactive comment line "return Tree(...)" so the function only builds the value
and returns Token("year", value) (refer to the year function and the commented
`return Tree(data="year", children=[value])` for location).
- Around line 118-119: The except KeyError block that currently does "except
KeyError: raise ValueError('Year is required for movable feasts')" should
capture the original exception (e.g., "except KeyError as err:") and re-raise
the ValueError using exception chaining ("raise ValueError('Year is required for
movable feasts') from err") so the original KeyError context is preserved;
update the handler in the movable feasts code path accordingly (mirror the
pattern used at the earlier handling on line 70).
In `@src/undate/date.py`:
- Around line 247-266: The weekday property currently returns a plain int;
change its return annotation from Optional[int] to Optional[Weekday] and return
a Weekday instance instead of an int by wrapping the computed value with
Weekday(...). Update the return expression in the weekday property (the
calculation using thursday_week and days_from_thursday) to return
Weekday((days_from_thursday + 3) % 7) and ensure Weekday is imported or
available in this module so the property returns the enum value while remaining
compatible with int comparisons.
- Around line 283-293: Weekday is a public symbol used by tests but isn’t
re-exported from the package root; update the package __init__ to import Weekday
from its module (the Weekday IntEnum) alongside DatePrecision and UnDelta and
add "Weekday" to the module's __all__ so callers can access Weekday.SUNDAY etc.
directly from the package root.
In `@tests/test_converters/test_holidays.py`:
- Around line 79-82: The orphaned comment should be moved so its intent is
clearly associated with the TestHolidayTransformer test class: relocate the line
"# edge cases - should not happen from parser input but possible" inside the
TestHolidayTransformer class body (or convert it into a class docstring) and
remove the extra blank lines so the comment/docstring sits directly under the
class definition; update TestHolidayTransformer accordingly so the comment
documents that class's edge-case tests.
- Line 45: Rename the test function and any related references from the
nonstandard spelling test_moveable_feasts to test_movable_feasts so the test
name matches the production symbol movable_feast; update the function definition
and all occurrences (e.g., parametrization, fixtures or calls) to the new name
to keep tests and grammar aligned.
🪄 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: b264cdbd-651c-4712-a6a2-279d8fdb28b3
📒 Files selected for processing (33)
.github/workflows/check.yml.github/workflows/unit_tests.ymlCHANGELOG.mdDEVELOPER_NOTES.mdpyproject.tomlscripts/generate_gregorian_grammar.pysrc/undate/__init__.pysrc/undate/converters/calendars/gregorian/__init__.pysrc/undate/converters/calendars/gregorian/converter.pysrc/undate/converters/calendars/gregorian/parser.pysrc/undate/converters/calendars/gregorian/transformer.pysrc/undate/converters/calendars/hebrew/converter.pysrc/undate/converters/calendars/hebrew/parser.pysrc/undate/converters/calendars/hebrew/transformer.pysrc/undate/converters/calendars/islamic/converter.pysrc/undate/converters/calendars/islamic/parser.pysrc/undate/converters/calendars/islamic/transformer.pysrc/undate/converters/combined.pysrc/undate/converters/edtf/converter.pysrc/undate/converters/grammars/combined.larksrc/undate/converters/grammars/gregorian.larksrc/undate/converters/grammars/gregorian_multilang.larksrc/undate/converters/grammars/hebrew.larksrc/undate/converters/grammars/holidays.larksrc/undate/converters/grammars/islamic.larksrc/undate/converters/grammars/undate_common.larksrc/undate/converters/holidays.pysrc/undate/date.pytests/test_converters/test_calendars/test_gregorian/test_gregorian_converter.pytests/test_converters/test_calendars/test_gregorian/test_gregorian_parser.pytests/test_converters/test_calendars/test_gregorian/test_gregorian_transformer.pytests/test_converters/test_combined_parser.pytests/test_converters/test_holidays.py
Adapting from work updating pre-comit hooks by @laurejt on viapy Co-authored-by: Laure Thompson <602628+laurejt@users.noreply.github.com>
Allow manual publication for pypi package
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.pre-commit-config.yaml:
- Line 1: The top-level "files: \.py" entry in .pre-commit-config.yaml is
restricting all hooks (including check-merge-conflict, check-yaml, yamlfmt,
codespell, action-validator) to Python files only; remove the global "files:
\.py" key or delete that line and instead add per-hook "files:" filters only on
the specific hooks that truly need Python-only filtering (or leave hooks without
a files filter so they respect their own types/types_or), ensuring
YAML/workflow/text hooks run on non-.py files.
In `@pyproject.toml`:
- Line 93: The current pyproject.toml setting filterwarnings = ["error"]
escalates all warnings (including third-party DeprecationWarnings) to errors;
update the filterwarnings entry to keep tests strict for your project-only
warnings while whitelisting noisy third-party modules (e.g., scope "error" to
your package's warnings and add default::DeprecationWarning:<module_pattern>
entries for known noisy libs like convertdate and lark) so pytest>=9 upgrades
don’t break CI from transitive warnings.
🪄 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: 534b29c5-4dfb-4e70-a6f5-957717c7375e
📒 Files selected for processing (16)
.github/workflows/check.yml.github/workflows/python-publish.yml.github/workflows/unit_tests.yml.pre-commit-config.yaml.readthedocs.yamlDEVELOPER_NOTES.mdpyproject.tomlsrc/undate/converters/base.pysrc/undate/converters/calendars/__init__.pysrc/undate/converters/calendars/hebrew/converter.pysrc/undate/converters/iso8601.pysrc/undate/date.pysrc/undate/undate.pytests/test_converters/test_calendars/test_islamic/test_islamic_transformer.pytests/test_converters/test_combined_parser.pytests/test_converters/test_holidays.py
✅ Files skipped from review due to trivial changes (10)
- src/undate/converters/base.py
- src/undate/undate.py
- tests/test_converters/test_calendars/test_islamic/test_islamic_transformer.py
- src/undate/converters/calendars/hebrew/converter.py
- .github/workflows/python-publish.yml
- src/undate/converters/calendars/init.py
- src/undate/converters/iso8601.py
- tests/test_converters/test_holidays.py
- tests/test_converters/test_combined_parser.py
- src/undate/date.py
🚧 Files skipped from review as they are similar to previous changes (3)
- DEVELOPER_NOTES.md
- .github/workflows/check.yml
- .github/workflows/unit_tests.yml
| @@ -1,22 +1,46 @@ | |||
| files: \.py | |||
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
pre-commit config top-level files regex semantics default applied to hooks
💡 Result:
In pre-commit's .pre-commit-config.yaml, top-level 'files' and 'exclude' options are Python regular expressions matched against file paths relative to the repository root using re.search semantics (substring match anywhere in the path, not full path anchored unless ^/$ used). - 'files': optional global include pattern (default: empty string ''). An empty regex matches all files (re.search('', path) succeeds for any path). - 'exclude': optional global exclude pattern (default: '^$', which matches only empty strings, excluding nothing). These apply before hook-level filters. Global 'files' selects candidates, then global 'exclude' removes matches from them, then hook-specific 'files'/'exclude' etc. apply. Hook-level 'files'/'exclude' use the same regex semantics. Source: Official pre-commit docs confirm defaults and top-level application; regex section explicitly states "The patterns for files and exclude are python regular expressions and are matched with re.search." Examples show multiline (?x) regexes for complex patterns.
Citations:
- 1: https://pre-commit.com/
- 2: https://pre-commit.com/index.html
- 3: exclude and files filters pre-commit/pre-commit#1470
🏁 Script executed:
cat -n .pre-commit-config.yamlRepository: dh-tech/undate-python
Length of output: 1716
Critical: top-level files: \.py silently disables every new non-Python hook.
The global files regex on line 1 applies to every hook that does not override it. check-merge-conflict, check-yaml, yamlfmt, codespell, and action-validator have no local files overrides and will only match files ending in .py, rendering them no-ops—these tools are meant to validate YAML, workflows, text, and merge conflicts, not Python source.
Remove the top-level filter (recommended—individual hooks already declare types/types_or appropriately) or override files per-hook that needs non-Python coverage.
Proposed fix
-files: \.py
repos:If a subset of existing hooks really did need to be restricted to *.py, re-declare files: on those individual hooks instead of globally.
📝 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.
| files: \.py | |
| repos: |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.pre-commit-config.yaml at line 1, The top-level "files: \.py" entry in
.pre-commit-config.yaml is restricting all hooks (including
check-merge-conflict, check-yaml, yamlfmt, codespell, action-validator) to
Python files only; remove the global "files: \.py" key or delete that line and
instead add per-hook "files:" filters only on the specific hooks that truly need
Python-only filtering (or leave hooks without a files filter so they respect
their own types/types_or), ensuring YAML/workflow/text hooks run on non-.py
files.
| log_level = "INFO" | ||
| strict = true | ||
| addopts = ["-ra"] | ||
| filterwarnings = ["error"] |
There was a problem hiding this comment.
filterwarnings = ["error"] can make CI brittle against transitive warnings.
Combined with pytest>=9, escalating every warning to an error means any DeprecationWarning from numpy, convertdate, lark, or Python itself (e.g. on minor version bumps) will fail the suite even when the project code is fine. Consider scoping to your own warning categories and whitelisting known-noisy third-party ones, e.g.:
filterwarnings = [
"error",
"default::DeprecationWarning:convertdate.*",
"default::DeprecationWarning:lark.*",
]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pyproject.toml` at line 93, The current pyproject.toml setting filterwarnings
= ["error"] escalates all warnings (including third-party DeprecationWarnings)
to errors; update the filterwarnings entry to keep tests strict for your
project-only warnings while whitelisting noisy third-party modules (e.g., scope
"error" to your package's warnings and add
default::DeprecationWarning:<module_pattern> entries for known noisy libs like
convertdate and lark) so pytest>=9 upgrades don’t break CI from transitive
warnings.
rlskoeser
force-pushed
the
feature/repo-review-cleanup
branch
from
d68fdbf to
c14fb37
Compare
Copied from work by @laurejt on piffle Co-authored-by: Laure Thompson <602628+laurejt@users.noreply.github.com>
1 participant
Summary by CodeRabbit
Chores
Documentation
Bug Fixes