Update iCalendar generation tool: add output suffix option#3647
Update iCalendar generation tool: add output suffix option#3647sanmaxdev wants to merge 7 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds Changes--output-suffix CLI feature
Estimated code review effort: 2 (Simple) | ~12 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 3 files
Confidence score: 3/5
- In
holidays/generate_ics.py, the--output-suffixnormalization can treat following CLI flags as suffix text, which can silently drop valid options and produce incorrect output/behavior for users running the command. Tighten parsing so--output-suffixonly consumes its intended value (and add a CLI regression test for adjacent flags) before merging.
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
sanmaxdev
force-pushed
the
fix/ics-output-suffix
branch
from
570537c to
b4331ed
Compare
![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
🤖 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 `@tests/test_generate_ics.py`:
- Around line 470-474: The test method
test_filename_output_and_suffix_are_mutually_exclusive has two nested with
statements that can be combined into a single with statement for cleaner code.
Combine the self.argv context manager and self.assertRaises context manager into
one with statement by separating them with a comma on the same line, then dedent
the IcsGenerator() call to align with the combined with statement.
🪄 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: 60edb8d5-d1f5-4a27-8c3b-558c1d9686ec
📒 Files selected for processing (3)
docs/examples.mdholidays/generate_ics.pytests/test_generate_ics.py
sanmaxdev
force-pushed
the
fix/ics-output-suffix
branch
from
b4331ed to
1352035
Compare
|
Follow-up pushed after review:
Re-validated locally:
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #3647 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 318 318
Lines 19167 19183 +16
Branches 2457 2459 +2
=========================================
+ Hits 19167 19183 +16 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Pushed a small follow-up for the Validation:
|
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 `@tests/test_generate_ics.py`:
- Around line 477-486: The two new tests in
test_output_suffix_does_not_consume_long_options and
test_output_suffix_missing_value_uses_argparse_error should follow the existing
SIM117 style by combining the nested argv and assertRaises context managers into
a single with block. Update the IcsGenerator test cases to use one combined
context manager statement, matching the already-fixed pattern in the same test
class, while keeping the unittest-style assertRaises convention.
🪄 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: f1c99f8e-9548-42fa-81f7-b793c83ea3a8
📒 Files selected for processing (1)
tests/test_generate_ics.py
There was a problem hiding this comment.
Pull request overview
Adds a new --output-suffix CLI option to holidays-ics (implemented in holidays.generate_ics.IcsGenerator) to let users keep the smart generated filename base and append a custom suffix (including values that start with -) without reconstructing --output.
Changes:
- Add
--output-suffixas a mutually exclusive alternative to-o/--output, plus argv normalization to support dash-prefixed suffix values. - Update filename generation to append the provided suffix exactly (or default to
.icswhen no suffix is provided). - Add tests and documentation examples covering suffix behavior, mutual exclusion, and parsing edge cases.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| holidays/generate_ics.py | Adds --output-suffix, normalizes argv to support --prefixed values, and updates output filename composition. |
| tests/test_generate_ics.py | Adds unit tests for suffix parsing/normalization and filename generation behavior. |
| docs/examples.md | Documents --output-suffix usage in the holidays-ics examples section. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| output_group.add_argument( | ||
| "--output-suffix", | ||
| help="Suffix to append to the generated output filename base (e.g., -MYNOTE.ics)", | ||
| ) |
KJhellico
changed the title
|
Addressed the latest review notes:
Verification:
|
There was a problem hiding this comment.
1 issue found across 2 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:221">
P2: The `or` fallback on the added line overrides an explicitly empty suffix. If a user passes `--output-suffix ''`, `argparse` sets the value to `''` (empty string, not `None`). Because `''` is falsy, `or ".ics"` replaces it, forcing an unwanted extension and ignoring the explicit input. This breaks the stated contract that the suffix is "appended exactly as provided (no implicit .ics added by the code)." The original `is not None` check correctly distinguished between "option omitted" (`None`) and "option given as empty string" (`''`). Restoring `self.args.output_suffix if self.args.output_suffix is not None else ".ics"` preserves both the default behavior and the explicit empty-string edge case.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
|
Removed the duplicate output suffix test from the latest review. Verification:
|
# Conflicts: # docs/examples.md
|
Pushed a small follow-up for the empty suffix case. Verification:
|
|
Proposed change
Adds
--output-suffixtoholidays-icsso users can keep the generated smart filename base and append their own suffix without manually reconstructing the whole--outputvalue.Closes #3645.
Implementation details:
--output-suffixoption alongside-o/--output..icsbehavior when no output option is passed..ics.-, matching the issue example:--output-suffix -MYNOTE.ics.holidays-icsexamples.Type of change
holidaysfunctionality in general)Checklist
make checklocally; all checks and tests passed.