Repository navigation
Commit a5a54a5
authored
* feat: find_all enumeration, occurrence ambiguity guard, batch operation_index (ISSUES.md #34)
- Document.find_all(text, paragraph=None) enumerates every match as
SearchResults whose paragraph_ref/paragraph_occurrence plug directly
into follow-up edits.
- Omitted occurrence now means the target must be unique in the search
scope; ambiguous targets raise AmbiguousTextError (new exception with
search_text, paragraph_ref, paragraph_preview, total_occurrences)
instead of silently editing the first match. Explicit occurrence=0
keeps the old behavior per call.
- Out-of-range occurrences in scoped edits report occurrence and
total_occurrences instead of a self-contradicting 'not found'.
- batch_edit/batch_rewrite raise only BatchOperationError for per-op
failures (stale hash included), with operation_index and the typed
underlying exception on original/__cause__. Batches stay atomic.
- Occurrence handling consolidated into _locate_in_paragraph, replacing
four duplicated find-or-raise paths; dry-run validation shares it.
- Docs (SKILL.md, api.md) and benchmarks updated; 14 new find_all tests
plus ambiguity, out-of-range, and single-exception-contract suites.
* review: fix same-paragraph batch advice, dedupe occurrence counting
Multi-reviewer findings on PR #53:
- The find_all docs recommended batch_edit for several matches in one
paragraph, but ops in find_all (ascending) order silently mis-target
or fail: each edit shifts the occurrence numbering of the matches
after it in the accepted text view. Corrected all three doc spots
(Document.find_all docstring, docs/api.md, SKILL.md) to require
descending occurrence order (an edit never shifts the matches before
it) with the self-overlapping-string caveat, and added
TestSameParagraphBatch locking in both the working descending pattern
and the atomic failure of the ascending one.
- Consolidated the two match-counting idioms into one
count_in_text_map() helper in xml_editor.py, now used by
_locate_in_paragraph, count_matches, and CommentManager._find_anchor
(also drops count_matches' quadratic inner loop).
- Dropped the one-off 'Note:' docstring heading in Document.find_all.
Skipped (reviewed, deliberate): the extra count_matches pass on the
document-wide occurrence=None success path — short-circuiting at the
second match would break the exact total_occurrences the
AmbiguousTextError contract promises.
* review: batch_rewrite rollback, locate naming, exception message dedup
Round-2 multi-reviewer findings on PR #53:
- batch_rewrite now snapshots the DOM and rolls back on mid-apply
failure, matching batch_edit — the documented 'nothing is applied on
failure' contract previously rested entirely on upfront validation
precluding every apply-phase error. Added a rollback test that
simulates a mid-loop failure (none is reachable today).
- Renamed _find_document_wide to _locate_document_wide: the _find_*
helpers return None on miss, while this one raises like
_locate_in_paragraph — the name now signals the find-or-raise
contract.
- TextNotFoundError docstring now covers the out-of-range occurrence
case that docs/api.md already documented.
- Factored the 'Current content' message suffix into _append_preview so
TextNotFoundError and AmbiguousTextError build it identically instead
of one being period-safe by coincidence.
- Documented why _locate_document_wide counts via count_matches rather
than count_in_text_map (text maps are per-paragraph; exact total is
part of the error contract).
Rejected finding: a claim that _apply_single_edit re-validates hashes
mid-batch, breaking same-paragraph descending-order batches — refuted
by the code (apply resolves by index only), the passing
TestSameParagraphBatch, and an end-to-end repro.
* review: complete locate naming, copy-paste-safe find_all examples
Round-3 multi-reviewer findings on PR #53 (correctness reviewer CLEAN):
- Renamed CommentManager._find_anchor to _locate_anchor, completing the
find-or-raise naming convention round 2 introduced (_find_* helpers
return None on miss; this one always raises).
- The find_all examples in the docstring, api.md, and SKILL.md now show
the pattern that is safe for any match distribution — one batch_edit
over reversed(find_all(...)), which yields the required descending
occurrence order for same-paragraph ops — instead of the sequential
loop that only works with one match per paragraph.
- api.md and SKILL.md now note BatchOperationError.original is None for
batch-level rule violations (missing paragraph ref, duplicate
paragraph) that have no underlying exception.
* review: reject negative occurrence before the text-map search
CodeRabbit findings on PR #53:
- A negative occurrence passed to the direct (non-batch) edit methods
reached find_in_text_map, where range(occurrence + 1) never runs and
the scoped paths crashed with UnboundLocalError (document-wide paths
produced a nonsensical 'occurrence=-1 requested' message). All three
locate paths (_locate_in_paragraph, _locate_document_wide,
CommentManager._locate_anchor) now reject negatives up front with the
same ValueError the EditOperation constructors raise; regression test
covers all four public paths.
- Added the missing return annotation on the _edit_calls test helper.
Skipped (deliberate, settled in earlier rounds): the second
count_matches scan on the document-wide occurrence=None path — the
exact total is part of the AmbiguousTextError contract and the
trade-off is documented in _locate_document_wide's docstring.
1 parent d9833a1 commit a5a54a5
14 files changed
Lines changed: 1248 additions & 202 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
228 | 228 | | |
229 | 229 | | |
230 | 230 | | |
231 | | - | |
| 231 | + | |
| 232 | + | |
232 | 233 | | |
233 | | - | |
234 | | - | |
| 234 | + | |
| 235 | + | |
| 236 | + | |
| 237 | + | |
235 | 238 | | |
236 | 239 | | |
237 | 240 | | |
| |||
283 | 286 | | |
284 | 287 | | |
285 | 288 | | |
286 | | - | |
287 | | - | |
| 289 | + | |
| 290 | + | |
| 291 | + | |
| 292 | + | |
288 | 293 | | |
289 | 294 | | |
290 | 295 | | |
291 | 296 | | |
292 | 297 | | |
293 | 298 | | |
294 | | - | |
| 299 | + | |
295 | 300 | | |
296 | 301 | | |
297 | 302 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
222 | 222 | | |
223 | 223 | | |
224 | 224 | | |
| 225 | + | |
| 226 | + | |
| 227 | + | |
| 228 | + | |
| 229 | + | |
| 230 | + | |
| 231 | + | |
| 232 | + | |
| 233 | + | |
| 234 | + | |
| 235 | + | |
| 236 | + | |
| 237 | + | |
| 238 | + | |
| 239 | + | |
| 240 | + | |
| 241 | + | |
| 242 | + | |
| 243 | + | |
| 244 | + | |
| 245 | + | |
| 246 | + | |
| 247 | + | |
| 248 | + | |
| 249 | + | |
| 250 | + | |
| 251 | + | |
| 252 | + | |
| 253 | + | |
| 254 | + | |
| 255 | + | |
| 256 | + | |
| 257 | + | |
| 258 | + | |
| 259 | + | |
| 260 | + | |
| 261 | + | |
| 262 | + | |
225 | 263 | | |
226 | 264 | | |
227 | 265 | | |
| |||
239 | 277 | | |
240 | 278 | | |
241 | 279 | | |
242 | | - | |
| 280 | + | |
243 | 281 | | |
244 | 282 | | |
245 | 283 | | |
| |||
248 | 286 | | |
249 | 287 | | |
250 | 288 | | |
251 | | - | |
| 289 | + | |
252 | 290 | | |
253 | 291 | | |
254 | 292 | | |
| |||
259 | 297 | | |
260 | 298 | | |
261 | 299 | | |
262 | | - | |
| 300 | + | |
263 | 301 | | |
264 | 302 | | |
265 | 303 | | |
266 | 304 | | |
267 | 305 | | |
268 | 306 | | |
269 | 307 | | |
270 | | - | |
| 308 | + | |
271 | 309 | | |
272 | 310 | | |
273 | 311 | | |
| |||
277 | 315 | | |
278 | 316 | | |
279 | 317 | | |
280 | | - | |
| 318 | + | |
281 | 319 | | |
282 | 320 | | |
283 | 321 | | |
| |||
286 | 324 | | |
287 | 325 | | |
288 | 326 | | |
289 | | - | |
| 327 | + | |
290 | 328 | | |
291 | 329 | | |
292 | 330 | | |
| |||
296 | 334 | | |
297 | 335 | | |
298 | 336 | | |
299 | | - | |
| 337 | + | |
300 | 338 | | |
301 | 339 | | |
302 | 340 | | |
| |||
305 | 343 | | |
306 | 344 | | |
307 | 345 | | |
308 | | - | |
| 346 | + | |
309 | 347 | | |
310 | 348 | | |
311 | 349 | | |
| |||
344 | 382 | | |
345 | 383 | | |
346 | 384 | | |
| 385 | + | |
| 386 | + | |
347 | 387 | | |
348 | 388 | | |
349 | 389 | | |
| |||
380 | 420 | | |
381 | 421 | | |
382 | 422 | | |
| 423 | + | |
| 424 | + | |
383 | 425 | | |
384 | 426 | | |
385 | 427 | | |
| |||
391 | 433 | | |
392 | 434 | | |
393 | 435 | | |
394 | | - | |
| 436 | + | |
395 | 437 | | |
396 | 438 | | |
397 | 439 | | |
| |||
403 | 445 | | |
404 | 446 | | |
405 | 447 | | |
406 | | - | |
| 448 | + | |
407 | 449 | | |
408 | 450 | | |
409 | 451 | | |
| |||
697 | 739 | | |
698 | 740 | | |
699 | 741 | | |
700 | | - | |
| 742 | + | |
701 | 743 | | |
702 | 744 | | |
703 | 745 | | |
704 | 746 | | |
705 | 747 | | |
706 | | - | |
| 748 | + | |
707 | 749 | | |
708 | 750 | | |
709 | 751 | | |
710 | | - | |
| 752 | + | |
711 | 753 | | |
712 | | - | |
| 754 | + | |
713 | 755 | | |
714 | 756 | | |
715 | 757 | | |
716 | 758 | | |
717 | 759 | | |
718 | 760 | | |
719 | 761 | | |
720 | | - | |
| 762 | + | |
721 | 763 | | |
722 | 764 | | |
723 | 765 | | |
| |||
777 | 819 | | |
778 | 820 | | |
779 | 821 | | |
780 | | - | |
| 822 | + | |
| 823 | + | |
781 | 824 | | |
782 | 825 | | |
783 | 826 | | |
| |||
833 | 876 | | |
834 | 877 | | |
835 | 878 | | |
836 | | - | |
| 879 | + | |
| 880 | + | |
| 881 | + | |
| 882 | + | |
| 883 | + | |
837 | 884 | | |
838 | 885 | | |
839 | 886 | | |
| |||
844 | 891 | | |
845 | 892 | | |
846 | 893 | | |
| 894 | + | |
| 895 | + | |
| 896 | + | |
| 897 | + | |
| 898 | + | |
| 899 | + | |
| 900 | + | |
| 901 | + | |
| 902 | + | |
| 903 | + | |
| 904 | + | |
| 905 | + | |
| 906 | + | |
| 907 | + | |
| 908 | + | |
| 909 | + | |
| 910 | + | |
| 911 | + | |
| 912 | + | |
| 913 | + | |
| 914 | + | |
| 915 | + | |
| 916 | + | |
| 917 | + | |
| 918 | + | |
| 919 | + | |
| 920 | + | |
| 921 | + | |
| 922 | + | |
| 923 | + | |
| 924 | + | |
| 925 | + | |
| 926 | + | |
| 927 | + | |
| 928 | + | |
| 929 | + | |
| 930 | + | |
| 931 | + | |
| 932 | + | |
| 933 | + | |
| 934 | + | |
| 935 | + | |
| 936 | + | |
847 | 937 | | |
848 | 938 | | |
849 | 939 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
37 | 37 | | |
38 | 38 | | |
39 | 39 | | |
| 40 | + | |
40 | 41 | | |
41 | 42 | | |
42 | 43 | | |
| |||
88 | 89 | | |
89 | 90 | | |
90 | 91 | | |
| 92 | + | |
91 | 93 | | |
92 | 94 | | |
93 | 95 | | |
| |||
0 commit comments