Repository navigation
feat: add dry_run validation mode to batch_edit() - #28
Conversation
|
Warning Review limit reached
Next review available in: 48 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds dry-run batch edit validation that returns per-operation results without mutating the document, reuses shared action-target validation in the edit pipeline, and expands tests to cover valid and invalid dry-run cases. ChangesDry-run validation flow
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Poem
🚥 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 @@
## main #28 +/- ##
=====================================
Coverage 94.5% 94.6%
=====================================
Files 10 10
Lines 2488 2526 +38
Branches 560 565 +5
=====================================
+ Hits 2353 2391 +38
Misses 36 36
Partials 99 99 ☔ 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: 2
🤖 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 `@docx_editor/track_changes.py`:
- Around line 262-297: The per-action argument validation in _validate_single
duplicates the same checks already present in _apply_single_edit, so the dry-run
path can drift from the real apply path. Extract the shared
action-to-target/required-fields logic into a helper such as
_resolve_action_target(op) and have both _validate_single and _apply_single_edit
use it for replace/delete/insert_before/insert_after before proceeding with
paragraph lookup or apply semantics. Keep the existing error messages and action
handling behavior consistent by centralizing the checks in one place.
- Around line 298-304: The dry-run validation in find_in_text_map is catching
all exceptions from _find_in_paragraph, which can mask unrelated bugs as
validation failures. Narrow this logic by explicitly handling a negative
op.occurrence before calling _find_in_paragraph, or catch only the specific
error raised for that case, and leave other exceptions to surface normally. Keep
the existing preview/error message behavior for the validation path while
avoiding a broad except Exception around _find_in_paragraph.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 08399aee-f962-41fc-87ca-196c52c01135
📒 Files selected for processing (4)
docx_editor/__init__.pydocx_editor/document.pydocx_editor/track_changes.pytests/test_batch_edit.py
Adds Document.batch_edit(ops, dry_run=True) which validates every operation without mutating the document and returns one EditValidationResult per op (in input order). Validation reuses the same paragraph-resolution and text-search helpers as the real apply path so it cannot drift from execution semantics. - @overload signatures narrow the return type by the dry_run literal - validate_batch never raises: malformed ops (e.g. negative occurrence) are reported as invalid rather than propagating - not-found errors include the paragraph preview for easier debugging - docstrings note that same-paragraph sequential effects are not simulated (each op is validated against the current document)
Adds dry_run tests for missing paragraph ref, malformed ref, per-action missing-argument errors, and unknown action so codecov/patch sees the new validation branches in _validate_single as covered.
1f22480 to
75f44d6
Compare
The per-action required-argument checks were duplicated between _apply_single_edit and _validate_single. Extract them into a single _resolve_action_target helper so the dry-run validation path provably cannot drift from the apply path (addresses PR review feedback).
Replace the broad 'except Exception' in _validate_single with an
explicit op.occurrence < 0 guard. That is the only non-well-formed
input find_in_text_map chokes on, so guarding it up front gives a
clearer message ('occurrence must be >= 0') and avoids a blanket catch
that could mask unrelated bugs (addresses PR review feedback).
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/test_batch_edit.py (1)
480-501: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winConsider adding a companion apply-path test for negative occurrence.
Once
_apply_single_edit/_resolve_action_targetgains the negative-occurrence guard (see track_changes.py comment), add adry_run=Falsecounterpart totest_bad_occurrence_reports_instead_of_raisingassertingBatchOperationErroris raised (not an unhandled exception), to lock in the fix and prevent regression.🤖 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_batch_edit.py` around lines 480 - 501, Add a companion apply-path test for negative occurrence handling in batch editing. Extend the existing test around test_bad_occurrence_reports_instead_of_raising to cover dry_run=False and assert that doc.batch_edit raises BatchOperationError rather than an unhandled exception when EditOperation uses occurrence=-1. Use the same multi_para_doc setup and keep the assertions aligned with the _apply_single_edit/_resolve_action_target negative-occurrence guard so the regression stays covered.
🤖 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 `@docx_editor/track_changes.py`:
- Around line 182-230: The negative occurrence check is missing from the shared
edit path, so `_apply_single_edit` can still pass an invalid `op.occurrence`
into `_find_in_paragraph` and trigger an internal error instead of the intended
validation flow. Add the `occurrence < 0` guard to the shared
validation/resolution path used by `_resolve_action_target` and
`_apply_single_edit` (or check it directly in `_apply_single_edit`) so all
actions fail consistently before calling `_find_in_paragraph`.
---
Outside diff comments:
In `@tests/test_batch_edit.py`:
- Around line 480-501: Add a companion apply-path test for negative occurrence
handling in batch editing. Extend the existing test around
test_bad_occurrence_reports_instead_of_raising to cover dry_run=False and assert
that doc.batch_edit raises BatchOperationError rather than an unhandled
exception when EditOperation uses occurrence=-1. Use the same multi_para_doc
setup and keep the assertions aligned with the
_apply_single_edit/_resolve_action_target negative-occurrence guard so the
regression stays covered.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: fe1d49c3-67c5-4303-a9dc-6bd6c82994b7
📒 Files selected for processing (2)
docx_editor/track_changes.pytests/test_batch_edit.py
The occurrence < 0 guard lived only in _validate_single, so the apply path (_apply_single_edit) could still pass a negative occurrence into _find_in_paragraph and raise a raw UnboundLocalError instead of a clean BatchOperationError. Move the guard into the shared _resolve_action_target helper both paths call, and drop the now-duplicate check in _validate_single so the two paths stay consistent.
Add a 1-based section field to ParagraphLocation, computed by walking paragraph-level w:pPr/w:sectPr boundaries in document order. A sectPr-carrying paragraph belongs to the section it closes; the next paragraph starts the following one, and the body-level sectPr defines the final section. Direct-child walks keep w:pPrChange revision records from spoofing section boundaries. Both accessors report it: get_paragraph_location() via a prefix scan (same pattern as heading_path) and list_paragraph_locations() via a single precomputed pass. Documented in docs/api.md and SKILL.md.
1 participant
Summary
Adds a non-mutating validation mode to
Document.batch_edit()(ISSUES.md #10):dry_run=Truevalidates every operation without applying any edits and returns oneEditValidationResult(index, paragraph, valid, error)per operation, in input order. The document is left unchanged. Validation reuses the same_resolve_paragraph/_find_in_paragraphhelpers as the real apply path, so it cannot drift from execution semantics.What it checks (per op, mirroring
_apply_single_edit)replaceneeds find/replace_with, etc.)Details
@overloadsignatures narrow the return type by thedry_runliteral (list[str]vslist[EditValidationResult]), so callers don't need anisinstanceguard.validate_batchnever raises: malformed ops (e.g. a negativeoccurrence) are reported asvalid=Falserather than propagating an exception.TextNotFoundErrorexposes.Testing
TestBatchEditDryRunsuite: all-valid, stale-hash, missing-text, mixed valid/invalid, empty batch, preview-in-error, and never-raises-on-bad-occurrence.make checkgreen (ruff lint + format +tystatic type check).Summary by CodeRabbit
New Features
dry_runoption for batch edits to validate changes before applying them.Bug Fixes
Refactor