Update iCalendar generation tool: add output filename template support#3679
Update iCalendar generation tool: add output filename template support#3679KJhellico wants to merge 5 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Summary by CodeRabbit
WalkthroughAdds ChangesOutput filename templating
Estimated code review effort: 2 (Simple) | ~15 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 @@
## dev #3679 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 318 318
Lines 19167 19210 +43
Branches 2457 2471 +14
=========================================
+ Hits 19167 19210 +43 ☔ View full report in Codecov by Harness. 🚀 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: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/examples.md`:
- Around line 719-723: The CLI example contains a typo in the holidays-ics
command: the output template flag is written with three leading dashes instead
of the standard flag form. Update the example in the docs snippet so the
`holidays-ics` invocation uses the correct `--output-template` option, matching
the other arguments like `--subdiv` and `--language`.
In `@holidays/generate_ics.py`:
- Around line 227-250: The output template handling in generate_ics.py only
catches KeyError, so malformed templates can still crash with an unhandled
traceback. Update the template.format(**values) exception handling in the
output-path logic to also catch format parsing errors such as ValueError and
IndexError, and raise the same SystemExit with a friendly message from the
original exception using exception chaining. Use the existing generate_ics path
that builds output_path and the values/template variables to keep the fix
localized.
In `@tests/test_generate_ics.py`:
- Around line 459-507: Add a test for a malformed output template with an
unmatched brace (for example in IcsGenerator().run via the existing
test_output_template_* patterns) to cover the uncaught ValueError/IndexError
path in output template parsing. Keep the test in unittest.TestCase style using
self.assertRaises/SystemExit conventions, and place it alongside the existing
output template tests so it exercises the same IcsGenerator and
template-handling code path.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: eb08fb84-093b-41ca-8cbd-7af7d73dc23b
📒 Files selected for processing (3)
docs/examples.mdholidays/generate_ics.pytests/test_generate_ics.py
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 3 files
Confidence score: 3/5
- In
holidays/generate_ics.py, malformed--output-templatevalues can raise an uncaught formatting exception because onlyKeyErroris handled in the new formatter path, so users may see a crash instead of a clear CLI validation message. Catch the relevant format-string exceptions and return a user-friendly argparse-style error before merging. - In
docs/examples.md, the example uses---output-template(three dashes), which will fail when copied because the CLI flag is invalid. Update the docs to--output-templateso users can run the example successfully.
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
There was a problem hiding this comment.
♻️ Duplicate comments (2)
holidays/generate_ics.py (2)
196-197: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winChain the exception with
from e.Ruff B904 flags this: re-raising
SystemExitfrom within theexcept ValueErrorblock loses the original traceback context.🛠️ Proposed fix
except ValueError as e: - raise SystemExit(f"Invalid output template: {e}") + raise SystemExit(f"Invalid output template: {e}") from e🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@holidays/generate_ics.py` around lines 196 - 197, The exception handling in the output template validation block loses traceback context when `ValueError` is re-raised as `SystemExit`. Update the `except ValueError as e` path in `generate_ics.py` to chain the new `SystemExit` from the original exception using `from e` so the original error context is preserved.Source: Linters/SAST tools
265-267: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
template.format(**values)is unguarded — malformed format specs still crash ugly.
validate_output_templateonly callsFormatter().parse(), which tokenizes the template but does not validate format specs against the field's__format__implementation. A template like"{code:d}"or"{categories:s}"(mismatched spec/type) passes validation cleanly but raises an unhandledValueErrorat line 267, since only the outerentity_loader/save_icsblock (lines 269-279) has a try/except. This is the same root concern from the earlier review (onlyKeyError/parse-time errors are handled) that wasn't fully closed off by adding theValueErrorcatch insidevalidate_output_template.🛡️ Proposed fix
self.validate_output_template(set(values)) template = self.args.output_template or self.get_default_output_template() - output_path = template.format(**values) + try: + output_path = template.format(**values) + except (KeyError, IndexError, ValueError) as e: + raise SystemExit(f"Invalid output template {template!r}: {e}") from e🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@holidays/generate_ics.py` around lines 265 - 267, The output template rendering in generate_ics is still unguarded because validate_output_template only parses the template and does not catch format-spec mismatches that fail inside template.format(**values). Update generate_ics so the formatting step around template.format(**values) is protected against ValueError and similar template rendering errors, and surface a clear validation/error path instead of letting the exception escape; use the existing validate_output_template and output_template flow as the entry point for the fix.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@holidays/generate_ics.py`:
- Around line 196-197: The exception handling in the output template validation
block loses traceback context when `ValueError` is re-raised as `SystemExit`.
Update the `except ValueError as e` path in `generate_ics.py` to chain the new
`SystemExit` from the original exception using `from e` so the original error
context is preserved.
- Around line 265-267: The output template rendering in generate_ics is still
unguarded because validate_output_template only parses the template and does not
catch format-spec mismatches that fail inside template.format(**values). Update
generate_ics so the formatting step around template.format(**values) is
protected against ValueError and similar template rendering errors, and surface
a clear validation/error path instead of letting the exception escape; use the
existing validate_output_template and output_template flow as the entry point
for the fix.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6ec01a97-1fe7-4e66-8906-b9a9798e8aa2
📒 Files selected for processing (3)
docs/examples.mdholidays/generate_ics.pytests/test_generate_ics.py
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="holidays/generate_ics.py">
<violation number="1" location="holidays/generate_ics.py:186">
P2: Some invalid output templates still crash with an uncaught formatting exception instead of the new validation error. `Formatter().parse()` exposes nested placeholders in `format_spec`, so validating only `field_name` misses cases like `{code:{bad}}`; consider validating nested format specs or catching `KeyError`/`ValueError` around the final `template.format(**values)`.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/test_generate_ics.py (1)
459-507: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMissing coverage for pure-literal escaped braces.
Good coverage of custom templates, defaults,
{today}, and{{{code}}}}escaping-around-a-real-placeholder. Consider adding a case like--output-template "{{literal}}.ics"(escaped braces with no real placeholder inside) to catch the field-extraction bug flagged inholidays/generate_ics.py(Line 188), where such templates incorrectly raise "Unknown placeholder".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_generate_ics.py` around lines 459 - 507, Add a test in test_generate_ics for a pure-literal escaped-brace template such as "{{literal}}.ics" to cover the field-extraction path in IcsGenerator.run. The issue is that generate_ics currently treats escaped braces with no real placeholder as an unknown placeholder, so the new test should assert the template is accepted and the expected literal-named file is created. Use the existing test_output_template_with_braces and test_output_template_unknown_placeholder cases as nearby references when adding the new coverage.holidays/generate_ics.py (1)
180-199: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winEscaped literal braces are parsed as placeholders.
re.findall(r"\{([a-z_]+)\}", template)will treat{{report}}.icsas containing{report}, so a valid literal template is rejected as an unknown placeholder; derive placeholder names from the same token grammar used in thefullmatchcheck.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@holidays/generate_ics.py` around lines 180 - 199, The output template validation in validate_output_template is misclassifying escaped literal braces as real placeholders. Update the placeholder extraction logic to use the same token grammar as the re.fullmatch check, so templates like double-braced literals are ignored instead of being passed into the unknown-placeholder validation. Keep the existing supported-placeholder error path, but ensure only actual {name} tokens are collected from the template.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@holidays/generate_ics.py`:
- Around line 180-199: The output template validation in
validate_output_template is misclassifying escaped literal braces as real
placeholders. Update the placeholder extraction logic to use the same token
grammar as the re.fullmatch check, so templates like double-braced literals are
ignored instead of being passed into the unknown-placeholder validation. Keep
the existing supported-placeholder error path, but ensure only actual {name}
tokens are collected from the template.
In `@tests/test_generate_ics.py`:
- Around line 459-507: Add a test in test_generate_ics for a pure-literal
escaped-brace template such as "{{literal}}.ics" to cover the field-extraction
path in IcsGenerator.run. The issue is that generate_ics currently treats
escaped braces with no real placeholder as an unknown placeholder, so the new
test should assert the template is accepted and the expected literal-named file
is created. Use the existing test_output_template_with_braces and
test_output_template_unknown_placeholder cases as nearby references when adding
the new coverage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d074857a-45ea-49fa-9d08-7585fe19a116
📒 Files selected for processing (2)
holidays/generate_ics.pytests/test_generate_ics.py
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@holidays/generate_ics.py`:
- Around line 185-189: The placeholder-name regex is duplicated in the template
parsing logic, so update the `generate_ics.py` template handling to define a
single shared module-level constant for the `{name}` pattern and reuse it in
both the `re.findall` tokenizer and the `fields` extraction. Keep the `tokens`
validation and `fields` parsing behavior in sync by referencing that shared
regex from the existing template-processing code.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: fbc311ec-04f2-409a-95e1-9f2ada6f6768
📒 Files selected for processing (1)
holidays/generate_ics.py
|
3 participants
Proposed change
Add
--output-templateto generate output filenames dynamically (e.g.,"{code}_{subdiv}_{language}_{start_year}_{end_year}_{categories}.ics"){code}{subdiv}ALLif not specified{language}DEFAULTif not specified{categories}_, orPUBLICif not specified{start_year}{end_year}{today}YYYYMMDDformatCloses #3642, #3645.
Type of change
holidaysfunctionality in general)Checklist
make checklocally; all checks and tests passed.