diff --git a/README.md b/README.md index 3bac976..6f9a5d0 100644 --- a/README.md +++ b/README.md @@ -133,7 +133,9 @@ with Document.open("reviewed.docx", author="Editor") as doc: if match and match.spans_revision: print("Text spans a revision boundary") - # Replace works even across revision boundaries + # Replace works even across revision boundaries. Instead of a ref, you can + # pass the match itself — it carries the text, paragraph and occurrence: + # doc.replace(match, "Goal: To") doc.replace("Aim: To", "Goal: To", paragraph="P1#a7b2") doc.save() diff --git a/docs/api.md b/docs/api.md index 558143a..1240c8a 100644 --- a/docs/api.md +++ b/docs/api.md @@ -36,6 +36,40 @@ doc = Document.open("contract.docx") doc = Document.open("contract.docx", author="Legal Team") ``` +#### `Document.discard_workspace(path, *, workspace_dir=None)` + +Delete a document's workspace so the next `open()` starts clean — the recovery +call for a **crashed or killed script**. That session's workspace stays behind +flagged as holding unsaved changes, so every later `open()` of the document +raises [`WorkspaceSyncError`](#workspacesyncerror) (or +[`WorkspaceLockedError`](#workspacelockederror) if its advisory lock is still on +disk) until the workspace is dealt with. One call resets that state, instead of +passing `force_recreate=True` on every open or deleting the cache directory by +hand. + +**This discards whatever unsaved edits the workspace holds** — the same +destruction as `open(force_recreate=True)`, without opening. To rescue them +first, save the orphaned workspace elsewhere (see +[`WorkspaceSyncError`](#workspacesyncerror)). The advisory lock sidecar goes with +the workspace, **including a lock held by a live session**, so make sure that +process is really gone: two sessions sharing one workspace silently overwrite +each other's saves. + +**Parameters:** + +- `path` (str | Path): The .docx whose workspace should be deleted. Need not exist — a workspace outlives its source, so this still cleans up after a moved or deleted document. +- `workspace_dir` (str | Path, optional): Base directory override, matching `open()`'s argument. Must be the same value the workspace was created with, or a different workspace is targeted (and nothing is found). + +**Returns:** `True` if a workspace was deleted, `False` if there was none — so it is safe to call unconditionally. + +**Example:** + +```python +# Idempotent reset before a fresh run: +Document.discard_workspace("contract.docx") +doc = Document.open("contract.docx") +``` + #### Workspace location When you open a document, its unpacked OOXML contents are stored in a workspace directory. By default this lives under the platform user cache, in a subfolder `docx-editor/` where `` is derived from the document's absolute path: @@ -108,7 +142,7 @@ List paragraphs with hash-anchored references. Refs are **1-based global** index **Parameters:** - `max_chars` (int): Maximum preview length (must be `>= 0`). Use `0` for refs only (e.g. `P1#a7b2`), with no preview or `| ` separator. -- `start` (int): 1-based index of the first paragraph to return (default 1). A `start` beyond the last paragraph yields an empty list. +- `start` (int): 1-based index of the first paragraph to return (default 1). A `start` beyond the last paragraph is a valid request with an empty answer — it yields `[]` and does **not** raise, exactly like `lst[500:]` on a shorter list, so a loop-until-empty walk terminates cleanly. (`start=0` or negative *does* raise `ValueError`: those are invalid inputs, not empty ranges.) To know where the end is up front, use `paragraph_count()` or follow the truncation notice's `start=` hint. - `limit` (int | None): Maximum number of paragraphs to return (default 200), or `None` for all paragraphs from `start` onward. **Returns:** List of strings in the form `P{index}#{hash}| preview text`, or bare `P{index}#{hash}` (no `| ` separator) when `max_chars=0` — plus one trailing `... N more paragraphs; use start=… or limit=None` notice when the window did not reach the end of the document. @@ -122,25 +156,28 @@ refs = [e for e in page1 if not e.startswith("...")] # drop the notice line everything = doc.list_paragraphs(limit=None) # uncapped, never a notice ``` -`list_paragraphs_structured()` (same `start`/`limit` semantics, returns typed `ParagraphInfo` records with full untruncated text) shares the 200-record default cap but appends **no notice** — every entry stays a `ParagraphInfo`. Detect truncation by checking whether the last record's `index` is still below `paragraph_count()` (robust for any `start`), or pass `limit=None`. +`list_paragraphs_structured()` (same `start`/`limit` semantics, returns typed [`ParagraphInfo`](#paragraphinfo) records with full untruncated text plus `style`/`outline_level`/`in_table`) shares the 200-record default cap but appends **no notice** — every entry stays a `ParagraphInfo`. Detect truncation by checking whether the last record's `index` is still below `paragraph_count()` (robust for any `start`), or pass `limit=None`. #### `get_paragraph(index)` -Return one paragraph as a structured `ParagraphInfo` record — the single-item counterpart to `list_paragraphs_structured()`. The returned record is identical to the one that method would emit for the same paragraph, without building a list. +Return one paragraph as a structured [`ParagraphInfo`](#paragraphinfo) record — the single-item counterpart to `list_paragraphs_structured()`. The returned record is identical to the one that method would emit for the same paragraph, without building a list. **Parameters:** - `index` (int): 1-based paragraph index (`P1` is `index=1`). Must be in `1 .. paragraph_count()`. -**Returns:** `ParagraphInfo` (index, hash-anchored ref, full untruncated text) for the paragraph at `index`. +**Returns:** `ParagraphInfo` (index, hash-anchored ref, full untruncated text, `style`, `outline_level`, `in_table`) for the paragraph at `index`. **Raises:** [`ParagraphIndexError`](#paragraphindexerror) if `index` is out of range (`< 1` or greater than `paragraph_count()`). +**Performance:** fixed-size output, but **O(document)** work — every call walks all `` elements to reach one. (`word/styles.xml` is parsed once per open `Document`, not per call.) Fine for a one-off lookup; for many paragraphs call `list_paragraphs_structured(limit=None)` once and index that list instead of looping over this. The same applies to `context()`. + **Example:** ```python info = doc.get_paragraph(1) print(info.ref, info.text) # "P1#a7b2" "Full paragraph text..." +print(info.style, info.outline_level, info.in_table) # "Heading1" 0 False ``` #### `context(ref, window=2)` @@ -152,10 +189,12 @@ Return the paragraphs surrounding `ref`, in document order — the "show me the - `ref` (str): Paragraph reference (e.g. `P3#a7b2`) from `list_paragraphs()`, `find_text()`/`find_all()`, or an edit result. - `window` (int): Paragraphs to include on *each side* of the referenced one (default 2, so up to 5 records). Must be `>= 0`; `0` returns just the referenced paragraph. -**Returns:** List of `ParagraphInfo` records (index, ref, full text) — identical to what `list_paragraphs_structured()` would emit for the same span. +**Returns:** List of [`ParagraphInfo`](#paragraphinfo) records — identical to what `list_paragraphs_structured()` would emit for the same span, structural fields included. **Raises:** `ValueError` if `ref` is malformed or `window < 0`; [`ParagraphIndexError`](#paragraphindexerror) / [`HashMismatchError`](#hashmismatcherror) for an out-of-range or stale `ref`. +**Performance:** O(document) despite the fixed-size output — see `get_paragraph()` above. + **Example:** ```python @@ -255,11 +294,8 @@ match = doc.find_text("Aim: To") if match: if match.spans_revision: print("Text spans a tracked-revision boundary") - # The ref + in-paragraph occurrence pin the exact match for a follow-up edit - doc.replace( - "Aim: To", "Goal: To", - paragraph=match.paragraph_ref, occurrence=match.paragraph_occurrence, - ) + # Pass the match itself: it pins the text, the paragraph and the occurrence + doc.replace(match, "Goal: To") ``` #### `find_all(text, paragraph=None)` @@ -283,11 +319,7 @@ what a follow-up edit needs. # Edit every match in one atomic batch. reversed() puts same-paragraph ops in # the required descending occurrence order, so this is safe however the # matches are distributed: -ops = [ - EditOperation.replace(r.text, "60 days", - paragraph=r.paragraph_ref, occurrence=r.paragraph_occurrence) - for r in reversed(doc.find_all("30 days")) -] +ops = [EditOperation.replace(r, "60 days") for r in reversed(doc.find_all("30 days"))] doc.batch_edit(ops) ``` @@ -317,17 +349,17 @@ if doc.count_matches("Section 5") > 1: print("Use paragraph refs and occurrence to target the intended match") ``` -#### `replace(find, replace_with, *, paragraph, occurrence=None)` +#### `replace(find, replace_with, *, paragraph=None, occurrence=None)` Replace text with tracked changes. When the target sits inside another author's pending insertion, that insertion is preserved: the matched text gets a nested `` under your authorship and the replacement lands in your own sibling `` (Word's behavior), instead of silently rewriting the other author's proposal. -Words shared by `find` and `replace_with` at either end are trimmed first, so only the changed words become revisions — a replace that only adds or only removes words is written as a pure insertion or deletion. The replacement insertion carries the formatting (`rPr`) that covers the most characters of the replaced span (runs sharing identical formatting tally together), ties breaking to the earliest-seen formatting. **Accepting** a replace that straddled mixed formatting therefore leaves the replacement uniformly in that one majority format, while each deletion run keeps its own original formatting — so a **reject** restores the pre-edit mix. When `replace_with` equals the found text, the call is a **no-op**: no revisions are created and the returned `EditResult` equals the input `paragraph` ref with `group_id=None` and `revision_ids=()` — that triple is how callers detect the no-op. +Words shared by `find` and `replace_with` at either end are trimmed first, so only the changed words become revisions — a replace that only adds or only removes words is written as a pure insertion or deletion. When the trimmed spans are **whitespace only** on both sides, trimming continues character by character, so a spacing fix (`"clause 2"` → `"clause 2"`) becomes a pure deletion rather than an invisible deletion/insertion pair. Word spans are never trimmed character-wise: `"30 days"` → `"60 days"` stays a whole-word replacement. The replacement insertion carries the formatting (`rPr`) that covers the most characters of the replaced span (runs sharing identical formatting tally together), ties breaking to the earliest-seen formatting. **Accepting** a replace that straddled mixed formatting therefore leaves the replacement uniformly in that one majority format, while each deletion run keeps its own original formatting — so a **reject** restores the pre-edit mix. When `replace_with` equals the found text, the call is a **no-op**: no revisions are created and the returned `EditResult` equals the input `paragraph` ref with `group_id=None` and `revision_ids=()` — that triple is how callers detect the no-op. **Parameters:** -- `find` (str): Text to find and replace +- `find` (str | [`SearchResult`](#searchresult)): Text to find and replace, or a match from `find_text()`/`find_all()` — which also supplies `paragraph` and `occurrence` (pass neither with it) - `replace_with` (str): Replacement text -- `paragraph` (str): Paragraph reference from `list_paragraphs()`, such as `P2#f3c1` +- `paragraph` (str): Paragraph reference from `list_paragraphs()`, such as `P2#f3c1`. Required unless `find` is a `SearchResult`. - `occurrence` (int | None): Which occurrence within the paragraph, 0-based (0 = first). Omitted → the target must be unique within the paragraph; if it matches more than once, [`AmbiguousTextError`](#ambiguoustexterror) is raised instead of silently editing the first match. **Returns:** Updated paragraph reference ([`EditResult`](#editresult) — a `str` subclass also carrying the edit's `group_id`/`changeset_id`/`revision_ids`) @@ -337,16 +369,18 @@ Words shared by `find` and `replace_with` at either end are trimmed first, so on ```python ref = doc.replace("30 days", "60 days", paragraph="P2#f3c1") doc.replace("net", "gross", paragraph=ref) +if match := doc.find_text("Manager"): + doc.replace(match, "Director") # straight from a search ``` -#### `delete(text, *, paragraph, occurrence=None)` +#### `delete(text, *, paragraph=None, occurrence=None)` Mark text as deleted with tracked changes. Deleting text inside another author's pending insertion nests a `` under your authorship inside their ``, preserving their proposal; only your own pending insertions are edited in place. **Parameters:** -- `text` (str): Text to mark as deleted -- `paragraph` (str): Paragraph reference from `list_paragraphs()`, such as `P2#f3c1` +- `text` (str | [`SearchResult`](#searchresult)): Text to mark as deleted, or a match from `find_text()`/`find_all()` — which also supplies `paragraph` and `occurrence` (pass neither with it) +- `paragraph` (str): Paragraph reference from `list_paragraphs()`, such as `P2#f3c1`. Required unless `text` is a `SearchResult`. - `occurrence` (int | None): Which occurrence within the paragraph, 0-based (0 = first). Omitted → the target must be unique within the paragraph; if it matches more than once, [`AmbiguousTextError`](#ambiguoustexterror) is raised instead of silently editing the first match. **Returns:** Updated paragraph reference ([`EditResult`](#editresult) — a `str` subclass also carrying the edit's `group_id`/`changeset_id`/`revision_ids`) @@ -355,17 +389,19 @@ Mark text as deleted with tracked changes. Deleting text inside another author's ```python ref = doc.delete("obsolete clause", paragraph="P5#d4e5") +if match := doc.find_text("obsolete clause"): + doc.delete(match) # same edit, from a search ``` -#### `insert_after(anchor, text, *, paragraph, occurrence=None)` +#### `insert_after(anchor, text, *, paragraph=None, occurrence=None)` Insert text after anchor with tracked changes. An anchor inside another author's pending insertion produces your own sibling `` (splitting theirs when the anchor falls mid-content) rather than splicing your words into their proposal. **Parameters:** -- `anchor` (str): Text to find as insertion point +- `anchor` (str | [`SearchResult`](#searchresult)): Text to find as insertion point, or a match from `find_text()`/`find_all()` — which also supplies `paragraph` and `occurrence` (pass neither with it) - `text` (str): Text to insert after the anchor -- `paragraph` (str): Paragraph reference from `list_paragraphs()`, such as `P2#f3c1` +- `paragraph` (str): Paragraph reference from `list_paragraphs()`, such as `P2#f3c1`. Required unless `anchor` is a `SearchResult`. - `occurrence` (int | None): Which occurrence within the paragraph, 0-based (0 = first). Omitted → the target must be unique within the paragraph; if it matches more than once, [`AmbiguousTextError`](#ambiguoustexterror) is raised instead of silently editing the first match. **Returns:** Updated paragraph reference ([`EditResult`](#editresult) — a `str` subclass also carrying the edit's `group_id`/`changeset_id`/`revision_ids`) @@ -374,17 +410,19 @@ Insert text after anchor with tracked changes. An anchor inside another author's ```python ref = doc.insert_after("Section 5", " (as amended)", paragraph="P3#b2c4") +if match := doc.find_text("Section 5"): + doc.insert_after(match, " (as amended)") ``` -#### `insert_before(anchor, text, *, paragraph, occurrence=None)` +#### `insert_before(anchor, text, *, paragraph=None, occurrence=None)` Insert text before anchor with tracked changes. Foreign pending insertions are treated the same as in `insert_after()`. **Parameters:** -- `anchor` (str): Text to find as insertion point +- `anchor` (str | [`SearchResult`](#searchresult)): Text to find as insertion point, or a match from `find_text()`/`find_all()` — which also supplies `paragraph` and `occurrence` (pass neither with it) - `text` (str): Text to insert before the anchor -- `paragraph` (str): Paragraph reference from `list_paragraphs()`, such as `P2#f3c1` +- `paragraph` (str): Paragraph reference from `list_paragraphs()`, such as `P2#f3c1`. Required unless `anchor` is a `SearchResult`. - `occurrence` (int | None): Which occurrence within the paragraph, 0-based (0 = first). Omitted → the target must be unique within the paragraph; if it matches more than once, [`AmbiguousTextError`](#ambiguoustexterror) is raised instead of silently editing the first match. **Returns:** Updated paragraph reference ([`EditResult`](#editresult) — a `str` subclass also carrying the edit's `group_id`/`changeset_id`/`revision_ids`) @@ -393,8 +431,12 @@ Insert text before anchor with tracked changes. Foreign pending insertions are t ```python ref = doc.insert_before("Section 6", "New clause: ", paragraph="P4#a7b2") +if match := doc.find_text("Section 6"): + doc.insert_before(match, "New clause: ") ``` +> **Passing a `SearchResult`** to `replace`/`delete`/`insert_after`/`insert_before`/`add_comment` (or an `EditOperation` constructor) is exactly equivalent to spelling out its `text`, `paragraph_ref` and `paragraph_occurrence`. Supplying `paragraph=` or `occurrence=` *as well* raises `ValueError` rather than picking a winner, and a match whose paragraph was edited in between raises [`HashMismatchError`](#hashmismatcherror) like any other stale ref. + > **Newlines split paragraphs.** A `\n` in any content text (`replace`'s > `replace_with`, `insert_after`/`insert_before`'s `text`, `rewrite_paragraph`'s > `new_text`, or a `batch_edit` op) is a **tracked paragraph split**: the first @@ -523,7 +565,7 @@ smart-quote splits, `w:ins` wrappers) are found. **Parameters:** -- `anchor_text` (str): Text to attach the comment to +- `anchor_text` (str | [`SearchResult`](#searchresult)): Text to attach the comment to, or a match from `find_text()`/`find_all()` — which also supplies `paragraph` and `occurrence` (pass neither with it) - `comment` (str): The comment content - `paragraph` (str, optional): Paragraph reference (e.g. `P3#a7b2`) to scope the search. `None` searches the whole document. Defaults to None. - `occurrence` (int | None): Which occurrence to anchor to, 0-based (0 = first), counted within `paragraph` when given and document-wide otherwise. Omitted → the anchor must be unique in the search scope, else [`AmbiguousTextError`](#ambiguoustexterror). @@ -535,6 +577,7 @@ smart-quote splits, `w:ins` wrappers) are found. ```python cid = doc.add_comment("Section 5", "Please review this section") doc.add_comment("term", "Note on the 2nd 'term'", paragraph="P3#a7b2", occurrence=1) +doc.add_comment(doc.find_all("term")[1], "Same 2nd 'term', from the search") ``` #### `reply_to_comment(comment_id, reply)` @@ -862,7 +905,7 @@ Save the document. **Returns:** Path to the saved document (Path) -After saving to a different path (or a save that fails), the workspace is flagged as holding unsaved changes; a later `Document.open()` of the source raises `WorkspaceSyncError` until the workspace is saved back to the source or discarded with `force_recreate=True`. See [`WorkspaceSyncError`](#workspacesyncerror) below. +After saving to a different path (or a save that fails), the workspace is flagged as holding unsaved changes; a later `Document.open()` of the source raises `WorkspaceSyncError` until the workspace is saved back to the source or discarded — with `force_recreate=True` on the next open, or once with [`Document.discard_workspace(path)`](#documentdiscard_workspacepath-workspace_dirnone). See [`WorkspaceSyncError`](#workspacesyncerror) below. **Example:** @@ -981,35 +1024,39 @@ from docx_editor import EditOperation ### Constructors -#### `EditOperation.replace(find, replace_with, *, paragraph, occurrence=None)` +#### `EditOperation.replace(find, replace_with, *, paragraph=None, occurrence=None)` -- `find` (str): Text to find and replace. Must be non-empty. +- `find` (str | [`SearchResult`](#searchresult)): Text to find and replace. Must be non-empty. - `replace_with` (str): Replacement text. Empty string is allowed (replacing with nothing is a valid tracked deletion). -#### `EditOperation.delete(text, *, paragraph, occurrence=None)` +#### `EditOperation.delete(text, *, paragraph=None, occurrence=None)` -- `text` (str): Text to mark as deleted. Must be non-empty. +- `text` (str | [`SearchResult`](#searchresult)): Text to mark as deleted. Must be non-empty. -#### `EditOperation.insert_after(anchor, text, *, paragraph, occurrence=None)` +#### `EditOperation.insert_after(anchor, text, *, paragraph=None, occurrence=None)` -#### `EditOperation.insert_before(anchor, text, *, paragraph, occurrence=None)` +#### `EditOperation.insert_before(anchor, text, *, paragraph=None, occurrence=None)` -- `anchor` (str): Text to find as insertion point. Must be non-empty. +- `anchor` (str | [`SearchResult`](#searchresult)): Text to find as insertion point. Must be non-empty. - `text` (str): Text to insert. All constructors also take: -- `paragraph` (str): Paragraph reference from `list_paragraphs()`, such as `P2#f3c1` +- `paragraph` (str): Paragraph reference from `list_paragraphs()`, such as `P2#f3c1`. Required unless the search target is a `SearchResult`, which supplies it. - `occurrence` (int | None): Which occurrence within the paragraph, 0-based (0 = first). Omitted → the target must be unique within the paragraph at apply time; if it matches more than once, the batch fails with a [`BatchOperationError`](#batchoperationerror) wrapping an [`AmbiguousTextError`](#ambiguoustexterror). Must be >= 0 when given. -**Raises:** `ValueError` at construction time if the paragraph ref is malformed, -`occurrence` is negative, a search-target argument (`find`, delete `text`, -`anchor`) is empty, or a payload argument (`replace_with`, insert `text`) is -`None` — payloads may be empty strings, search targets may not. Each -signature mirrors the corresponding `Document` method 1:1, so +**Raises:** `ValueError` at construction time if the paragraph ref is missing or +malformed, `occurrence` is negative, a search-target argument (`find`, delete +`text`, `anchor`) is empty, a payload argument (`replace_with`, insert `text`) +is `None` — payloads may be empty strings, search targets may not — or the +search target is a `SearchResult` and `paragraph`/`occurrence` was given too. +Each signature mirrors the corresponding `Document` method 1:1, so `doc.replace(...)` translates mechanically to `EditOperation.replace(...)`. +The dataclass fields stay plain strings whichever form you use, so an operation +built from a match is indistinguishable from a hand-written one. + ### Example ```python @@ -1018,6 +1065,9 @@ new_refs = doc.batch_edit([ EditOperation.delete("obsolete clause", paragraph="P5#d4e5"), EditOperation.insert_after("Section 5", " (as amended)", paragraph="P7#b1c2"), ]) + +# Or straight from a search — no refs or occurrences to carry: +doc.batch_edit([EditOperation.replace(m, "60 days") for m in reversed(doc.find_all("30 days"))]) ``` --- @@ -1081,6 +1131,24 @@ from docx_editor import EditValidationResult | `paragraph` | str or None | The operation's paragraph ref (`None` if it was missing) | | `valid` | bool | True if the operation would apply cleanly | | `error` | str or None | Human-readable reason when not valid | +| `current_ref` | str or None | Ref for the paragraph's **current** content — set only when the row failed on a stale hash | + +`current_ref` is the recovery field for the one failure a caller can fix +mechanically. When an operation carries `"P7#a7b2"` but the paragraph now +hashes to `c4d8`, the row reports `current_ref="P7#c4d8"` — rebuild the +operation with it instead of parsing the hash out of `error`. It is `None` for +every other outcome: valid rows, malformed refs, out-of-range indexes, missing +or ambiguous target text, and elements that are not an `EditOperation`. So +`if row.current_ref:` is how you spot the stale-hash rows. + +It clears the **hash** check only. Validation stops at the first failure, so a +row can report a `current_ref` *and* have a target that has since moved or +become ambiguous — the rebuilt operation is re-validated like any other, and a +target that no longer fits fails loudly and atomically (a +[`BatchOperationError`](#batchoperationerror) wrapping `TextNotFoundError` or +`AmbiguousTextError`, with nothing applied). Dry-run the repaired batch, or +re-search the paragraph, when the content may have changed materially rather +than just shifted. ### Example @@ -1099,6 +1167,15 @@ if all(r.valid for r in results): new_refs = doc.batch_edit(ops) ``` +Repair the stale-hash rows and retry, with no message parsing: + +```python +for row in doc.batch_edit(ops, dry_run=True): + if row.current_ref: + ops[row.index] = EditOperation.replace("old", "new", paragraph=row.current_ref) +new_refs = doc.batch_edit(ops) +``` + --- ## SearchResult @@ -1127,9 +1204,24 @@ document-wide offsets. Coordinate systems differ between search and edit: `find_text`'s `occurrence` counts matches document-wide (unless scoped with `paragraph=`), while edit methods count within one paragraph — `paragraph_occurrence` bridges the two, so always pass it alongside -`paragraph_ref` when chaining into an edit. `paragraph_ref` -is computed at search time and — like refs from `list_paragraphs()` — goes -stale once that paragraph is edited. +`paragraph_ref` when chaining into an edit — or skip the bookkeeping entirely +and pass the `SearchResult` itself as the edit's target (see below). +`paragraph_ref` is computed at search time and — like refs from +`list_paragraphs()` — goes stale once that paragraph is edited. + +**A SearchResult can stand in for its own three fields.** Every +text-targeting method — `replace`, `delete`, `insert_after`, `insert_before`, +`add_comment`, and the `EditOperation` constructors — accepts a `SearchResult` +where it expects the target text, and takes `paragraph` and `occurrence` from +the match. Passing `paragraph=`/`occurrence=` *as well* raises `ValueError` +(it would contradict the match), and a stale match raises +[`HashMismatchError`](#hashmismatcherror) like any other stale ref. + +`find_text()` returns `None` when there is no match, and that `None` is **not** +accepted as a target (a missing match must not become a silent no-op) — it +raises `ValueError` (`CommentError` from `add_comment`, which validates its +anchor before the ref). Check the result first, as the examples below do, rather +than piping `find_text()` straight into an edit in code that must not crash. `repr()`/`str()` are compact one-liners — `SearchResult(P3#a7b2 occ=0 '30 days')`, with a trailing `spans_rev` marker @@ -1143,10 +1235,71 @@ an attribute. ```python match = doc.find_text("30 days") if match: - doc.replace( - "30 days", "60 days", - paragraph=match.paragraph_ref, occurrence=match.paragraph_occurrence, - ) + doc.replace(match, "60 days") # the match pins paragraph + occurrence + + # The SAME single edit, spelled out — not a follow-up. Running both raises + # HashMismatchError, because the first one made `match` stale: + # doc.replace(match.text, "60 days", + # paragraph=match.paragraph_ref, + # occurrence=match.paragraph_occurrence) + +# Sweep every match in one atomic batch (reversed() keeps same-paragraph ops in +# the required descending-occurrence order): +from docx_editor import EditOperation + +doc.batch_edit([EditOperation.replace(m, "60 days") for m in reversed(doc.find_all("30 days"))]) +``` + +--- + +## ParagraphInfo + +The structured paragraph record returned by +`Document.list_paragraphs_structured()`, `Document.get_paragraph()` and +`Document.context()`. All three emit identical records for the same paragraph. + +```python +from docx_editor import ParagraphInfo +``` + +### Attributes + +| Attribute | Type | Description | +|-----------|------|-------------| +| `index` | int | 1-based paragraph index (`P1` is `index=1`) | +| `ref` | str | Hash-anchored ref `P{index}#{hash}`, usable as any `paragraph=` argument | +| `text` | str | Full visible text, never truncated | +| `in_table` | bool | True when the paragraph sits inside a `` table cell | +| `style` | str or None | Raw `w:pPr/w:pStyle/@w:val` style id (e.g. `"Heading1"`), None when unstyled | +| `outline_level` | int or None | 0-based outline level (0 == Heading 1), None for body text | + +`in_table`, `style` and `outline_level` carry exactly the meanings +[`ParagraphLocation`](#get_paragraph_locationref) defines for them — a direct +`w:outlineLvl` wins over the style's level, `w:val="9"` means body text, and a +`w:pPrChange` revision record is never read as current formatting. They are the +*cheap* structural facts: list numbering, table coordinates, heading paths and +section indexes still need `get_paragraph_location()` / +`list_paragraph_locations()`. + +`in_table` also makes the paragraph-index divergence self-describing: table-cell +paragraphs do get `P{i}` refs, and they are exactly the ones python-docx's +`doc.paragraphs` skips. + +`str(info)` renders `"P{i}#{hash}| {text}"` — the same delimiter format as +`list_paragraphs()`, always with the full text. + +### Example + +```python +# Table of contents from one call — no list_paragraph_locations() needed: +toc = [ + (info.outline_level, info.text) + for info in doc.list_paragraphs_structured(limit=None) + if info.outline_level is not None +] + +# Skip table-cell paragraphs while editing body prose: +body = [info for info in doc.list_paragraphs_structured(limit=None) if not info.in_table] ``` --- @@ -1329,7 +1482,11 @@ from docx_editor.exceptions import WorkspaceExistsError Raised when the workspace is out of sync with the source document: the source changed on disk since the workspace was created, or the workspace holds unsaved changes from a previous session that the source never received. Structured fields: `workspace_path` and `source_path`. -`Document.open(path, force_recreate=True)` recovers but discards the workspace's unsaved edits. To rescue them first, save the orphaned workspace to a new file (`Workspace` is not exported at the package root — use the deep import): +`Document.open(path, force_recreate=True)` and +[`Document.discard_workspace(path)`](#documentdiscard_workspacepath-workspace_dirnone) +both recover but discard the workspace's unsaved edits. To rescue them first, +save the orphaned workspace to a new file (`Workspace` is not exported at the +package root — use the deep import): ```python from docx_editor.exceptions import WorkspaceSyncError @@ -1342,6 +1499,10 @@ except WorkspaceSyncError: doc = Document.open("contract.docx", force_recreate=True) ``` +After a crashed script, one `Document.discard_workspace("contract.docx")` is +usually the better fix than carrying `force_recreate=True` on every subsequent +open — it also clears a lock left behind by the dead process. + ### `DocumentOpenError` Raised by `save()` when the destination appears open in another program. Word writes a `~$` owner (lock) file next to any document it has open; if that stub exists at save time, saving would race Word's own writes, so `save()` refuses unless `force=True`. Also raised when the OS denies the final replace (on Windows, Word holding the file open is exactly this case) — `force=True` cannot suppress that one. The exception carries `path` (the destination) and `owner_file` (the `~$` file that triggered the guard, or `None` when the OS denied the replace) attributes. diff --git a/docs/quickstart.md b/docs/quickstart.md index e1adcfc..e810054 100644 --- a/docs/quickstart.md +++ b/docs/quickstart.md @@ -23,7 +23,7 @@ with Document.open("contract.docx", author="Legal Team") as doc: doc.save() ``` -If the source `.docx` was modified outside this library since the workspace was created, `Document.open()` raises `WorkspaceSyncError` instead of silently discarding the workspace. The same error is raised when a leftover workspace holds unsaved changes from a previous session — for example, one that saved to a different path (or whose save failed) and never called `close()` — since adopting it would carry those edits into the new session. The error message includes the workspace path. Pass `force_recreate=True` to acknowledge the divergence and re-unpack from the current source: +If the source `.docx` was modified outside this library since the workspace was created, `Document.open()` raises `WorkspaceSyncError` instead of silently discarding the workspace. The same error is raised when a leftover workspace holds unsaved changes from a previous session — for example, one that saved to a different path (or whose save failed) and never called `close()` — since adopting it would carry those edits into the new session. The error message includes the workspace path. Pass `force_recreate=True` to acknowledge the divergence and re-unpack from the current source (after a crashed script, one `Document.discard_workspace("contract.docx")` beats carrying the flag on every open — it also clears a lock the dead process left behind): ```python doc = Document.open("contract.docx", force_recreate=True) @@ -74,6 +74,18 @@ Use the `P{index}#{hash}` part as the `paragraph=` argument. Edit methods return When the search text appears more than once in the target paragraph, pass `occurrence=` (0-based, so `occurrence=1` is the second match) to `replace()`, `delete()`, `insert_after()`, `insert_before()`, and `add_comment()`. Omitting it requires a unique match — otherwise the call raises `AmbiguousTextError`. +Better still, let a search do that bookkeeping: those five methods (and the `EditOperation` constructors) accept a `SearchResult` in place of the target text, taking the paragraph and occurrence from the match. + +```python +with Document.open("contract.docx") as doc: + if match := doc.find_text("30 days"): # or find_all(...)[1] for the 2nd + doc.replace(match, "60 days") # no paragraph=, no occurrence= +``` + +Check the result first: `find_text()` returns `None` when there is no match, and +passing that `None` as a target raises `ValueError` (`CommentError` from +`add_comment`) rather than quietly doing nothing. + ## Track Changes ### Replace Text diff --git a/docx_editor/document.py b/docx_editor/document.py index b08bad2..d3bfb4b 100644 --- a/docx_editor/document.py +++ b/docx_editor/document.py @@ -11,6 +11,8 @@ from typing import Literal, overload from xml.dom.minidom import Element +import defusedxml.minidom + from .comments import Comment, CommentManager from .exceptions import DocumentClosedError, HashMismatchError, ParagraphIndexError from .track_changes import ( @@ -21,6 +23,8 @@ RevisionManager, SearchResult, _ancestor_paragraph, + _paragraph_hint, + _resolve_search_target, ) from .workspace import Workspace from .xml_editor import ( @@ -29,7 +33,7 @@ ParagraphInfo, ParagraphLocation, ParagraphRef, - XMLEditor, + _build_paragraph_info, _build_style_numbering_map, _build_style_outline_map, _build_table_index, @@ -45,12 +49,21 @@ _DEFAULT_LIST_LIMIT = 200 -def _require_ref_string(paragraph: str) -> None: - """Reject non-string paragraph refs before they can silently select the - RevisionManager's document-wide search branch (its ``paragraph=None`` - mode is intentional at that layer, not at this one).""" +def _require_ref_string(paragraph: str | None, field: str | None = None) -> str: + """Reject a missing or non-string paragraph ref before it can silently select + the RevisionManager's document-wide search branch (its ``paragraph=None`` + mode is intentional at that layer, not at this one). + + ``paragraph`` is Optional in the edit methods' signatures only so a + SearchResult target can supply it; those callers pass ``field`` (the name of + their target parameter) to get the "or pass a SearchResult as …" hint. + Callers whose ref is genuinely required and positional (split_paragraph) + omit it. Returns the ref, narrowed to ``str`` for the call that follows. + """ if not isinstance(paragraph, str): - raise ValueError(f"'paragraph' must be a paragraph ref string like 'P3#a7b2', got {type(paragraph).__name__}") + message = f"'paragraph' must be a paragraph ref string like 'P3#a7b2', got {type(paragraph).__name__}" + raise ValueError(message if field is None else f"{message} — {_paragraph_hint(field)}") + return paragraph class Document: @@ -91,6 +104,8 @@ def __init__(self, workspace: Workspace): """ self._workspace = workspace self._closed = False + # Lazily parsed word/styles.xml maps (see _style_maps). + self._style_maps_cache: tuple[dict[str, int], dict[str, ListItem]] | None = None # Create the document editor. Every post-open workspace write goes # through an editor (or the comment manager's template copies), so @@ -192,6 +207,54 @@ def open( workspace = Workspace(path, author=author, create=True, workspace_dir=workspace_dir) return cls(workspace) + @classmethod + def discard_workspace(cls, path: str | Path, *, workspace_dir: str | Path | None = None) -> bool: + """Delete a document's workspace so the next :meth:`open` starts clean. + + The recovery tool for a crashed or killed script: that session's + workspace stays behind flagged as holding unsaved changes, and every + later ``open()`` of the same document raises + :class:`~docx_editor.exceptions.WorkspaceSyncError` (or + :class:`~docx_editor.exceptions.WorkspaceLockedError` if its advisory + lock is still on disk) until the workspace is dealt with. One call here + resets that state, instead of passing ``force_recreate=True`` on every + open from then on or deleting the cache directory by hand. + + **This discards whatever unsaved edits the workspace holds** — same + destruction as ``open(force_recreate=True)``, just without opening. To + rescue those edits first, save the orphaned workspace elsewhere and + inspect it before discarding (see the Example below). + + The workspace's advisory lock sidecar goes with it, including a lock + held by a live session — so a wedged process cannot keep the document + unopenable. Make sure that process is really gone first: two sessions + sharing one workspace silently overwrite each other's saves. + + Args: + path: Path to the .docx file whose workspace should be deleted. Not + required to exist — a workspace outlives its source, so this + still cleans up after a moved or deleted document. + workspace_dir: Base directory override, matching :meth:`open`'s + argument. Must be the same value the workspace was created + with, or a different workspace is targeted (and nothing is + found). + + Returns: + True if a workspace was deleted, False if there was none — so this + is safe to call unconditionally at the start of a script. + + Example: + # Idempotent reset before a fresh run: + Document.discard_workspace("contract.docx") + doc = Document.open("contract.docx") + + # Rescue the orphaned workspace's edits first: + from docx_editor.workspace import Workspace + Workspace("contract.docx", create=False).save("rescued.docx") + Document.discard_workspace("contract.docx") + """ + return Workspace.delete(path, workspace_dir=workspace_dir) + @property def author(self) -> str: """Get the author name for tracked changes.""" @@ -548,14 +611,20 @@ def list_paragraphs_structured( ) -> list[ParagraphInfo]: """List paragraphs as structured :class:`ParagraphInfo` records. - Like :meth:`list_paragraphs`, but returns named records (index, ref, - full text) instead of pipe-delimited preview strings. The ``text`` - field is always the full, untruncated paragraph text — there is no - ``max_chars`` parameter. ``str(info)`` uses the same - ``"P{i}#{hash}| {text}"`` delimiter format as :meth:`list_paragraphs`, - but always with the full text (it matches :meth:`list_paragraphs` - output only when that call's ``max_chars`` is large enough to avoid - truncation). + Like :meth:`list_paragraphs`, but returns named records instead of + pipe-delimited preview strings. The ``text`` field is always the full, + untruncated paragraph text — there is no ``max_chars`` parameter. + ``str(info)`` uses the same ``"P{i}#{hash}| {text}"`` delimiter format + as :meth:`list_paragraphs`, but always with the full text (it matches + :meth:`list_paragraphs` output only when that call's ``max_chars`` is + large enough to avoid truncation). + + Each record also carries the paragraph's cheap structural facts — + ``in_table``, ``style`` and ``outline_level`` (see + :class:`ParagraphInfo`) — so building a table of contents or skipping + table-cell paragraphs needs no second pass: + :meth:`list_paragraph_locations` is only necessary for table + coordinates, list numbering, heading paths and section indexes. Refs are **1-based global** indexes (P1, P2, …) and stay correct across slices, with the same ``start``/``limit`` semantics as @@ -591,21 +660,30 @@ def list_paragraphs_structured( if infos and infos[-1].index < doc.paragraph_count(): ... # truncated — continue from start=infos[-1].index + 1 everything = doc.list_paragraphs_structured(limit=None) + + # Table of contents, one pass, no locations call: + toc = [(i.outline_level, i.text) for i in everything if i.outline_level is not None] """ self._ensure_open() - result = [] - for i, p in self._iter_paragraph_slice(start, limit): - h = compute_paragraph_hash(p) - text = build_text_map(p).text - result.append(ParagraphInfo(index=i, ref=f"P{i}#{h}", text=text)) - return result + # One styles.xml parse for the whole slice resolves style-defined outline + # levels; the per-paragraph work stays a single text map (ISSUES.md #52). + style_outlines, _ = self._style_maps() + return [_build_paragraph_info(p, i, style_outlines) for i, p in self._iter_paragraph_slice(start, limit)] def get_paragraph(self, index: int) -> ParagraphInfo: """Return one paragraph as a structured :class:`ParagraphInfo` record. Single-item counterpart to :meth:`list_paragraphs_structured`. The - returned record (index, hash-anchored ref, full untruncated text) is - identical to the one that method would emit for the same paragraph. + returned record — index, hash-anchored ref, full untruncated text, plus + ``in_table``/``style``/``outline_level`` — is identical to the one that + method would emit for the same paragraph. + + Note: + Fixed-size output, but O(document) work: every call walks all + ```` elements to reach one. (``word/styles.xml`` is parsed once + per open Document, not per call.) Fine for a one-off lookup; for + many paragraphs call ``list_paragraphs_structured(limit=None)`` once + and index the result instead of looping over this. Args: index: 1-based paragraph index (P1 is ``index=1``). Must be in @@ -626,9 +704,8 @@ def get_paragraph(self, index: int) -> ParagraphInfo: paragraphs = self._document_editor.dom.getElementsByTagName("w:p") if index < 1 or index > len(paragraphs): raise ParagraphIndexError(index, len(paragraphs)) - p = paragraphs[index - 1] - h = compute_paragraph_hash(p) - return ParagraphInfo(index=index, ref=f"P{index}#{h}", text=build_text_map(p).text) + style_outlines, _ = self._style_maps() + return _build_paragraph_info(paragraphs[index - 1], index, style_outlines) def context(self, ref: str, window: int = 2) -> list[ParagraphInfo]: """Return the paragraphs surrounding ``ref``, in document order. @@ -640,6 +717,13 @@ def context(self, ref: str, window: int = 2) -> list[ParagraphInfo]: straight in. The records are identical to what :meth:`list_paragraphs_structured` would emit for the same indexes. + Note: + Fixed-size output, but O(document) work: resolving the ref walks all + ```` elements, twice over (hash check, then the slice). Fine per + match; to annotate many matches, call + ``list_paragraphs_structured(limit=None)`` once and slice around + each ``paragraph_index`` instead. + Args: ref: Paragraph reference (e.g., "P3#a7b2") from :meth:`list_paragraphs`, :meth:`find_text`/:meth:`find_all`, @@ -711,15 +795,38 @@ def _resolve_validated_ref(self, ref: str) -> tuple[int, list[Element]]: def _style_maps(self) -> tuple[dict[str, int], dict[str, ListItem]]: """Outline-level and numbering maps defined by paragraph styles. - One ``word/styles.xml`` parse serves both maps. Parsed per call - (no caching, matching the per-call table rescan philosophy); a - document without a styles part degrades to ``({}, {})``. + One ``word/styles.xml`` parse serves both maps, memoized for the life of + this Document: nothing in this library ever writes ``styles.xml`` (it is + read-only workspace content), so the maps cannot go stale within a + session. The parse is expensive enough to matter — ~340 ms on a + 3300-paragraph document — which is what makes memoizing it necessary now + that :meth:`get_paragraph` needs it too (ISSUES.md #52); the location + APIs get the same win for free. A document without a styles part + degrades to ``({}, {})``. + + Table indexes, heading paths and section indexes are deliberately *not* + cached: those derive from ``document.xml``, which edits do change. + + The returned maps are the cached objects themselves, not copies — + callers must treat them as read-only, since a mutation would outlive + the call and be seen by every later consumer. + + The parse deliberately skips :class:`XMLEditor`'s line-tracking parser + (halving the cost on a large styles part): the map builders read style + ids and attributes only, and nothing ever edits ``styles.xml``, so + ``parse_position`` metadata would go unused. """ - styles_path = self._workspace.word_path / "styles.xml" - if not styles_path.exists(): - return {}, {} - styles_dom = XMLEditor(styles_path).dom - return _build_style_outline_map(styles_dom), _build_style_numbering_map(styles_dom) + if self._style_maps_cache is None: + styles_path = self._workspace.word_path / "styles.xml" + if not styles_path.exists(): + self._style_maps_cache = ({}, {}) + else: + styles_dom = defusedxml.minidom.parse(str(styles_path)) + self._style_maps_cache = ( + _build_style_outline_map(styles_dom), + _build_style_numbering_map(styles_dom), + ) + return self._style_maps_cache def get_paragraph_location(self, ref: str) -> ParagraphLocation: """Return the structural location of the paragraph identified by ``ref``. @@ -934,7 +1041,14 @@ def get_markup_text(self) -> str: self._ensure_open() return self._revision_manager.get_markup_text() - def replace(self, find: str, replace_with: str, *, paragraph: str, occurrence: int | None = None) -> EditResult: + def replace( + self, + find: str | SearchResult, + replace_with: str, + *, + paragraph: str | None = None, + occurrence: int | None = None, + ) -> EditResult: """Replace text with tracked changes. Creates a tracked deletion of the old text and insertion of the new @@ -952,9 +1066,14 @@ def replace(self, find: str, replace_with: str, *, paragraph: str, occurrence: i that triple is how callers detect the no-op. Args: - find: Text to find and replace + find: Text to find and replace, or a + :class:`~docx_editor.track_changes.SearchResult` from + :meth:`find_text`/:meth:`find_all`. A SearchResult already + carries the matched text, its paragraph and its occurrence, so + pass neither ``paragraph`` nor ``occurrence`` with one. replace_with: Replacement text - paragraph: Paragraph reference from list_paragraphs() (e.g., "P2#f3c1") + paragraph: Paragraph reference from list_paragraphs() (e.g., + "P2#f3c1"). Required unless ``find`` is a SearchResult. occurrence: Which occurrence within the paragraph (0 = first, 1 = second, etc.). Omitted → ``find`` must be unique in the paragraph, else AmbiguousTextError (use find_all() to @@ -968,29 +1087,46 @@ def replace(self, find: str, replace_with: str, *, paragraph: str, occurrence: i Raises: ValueError: If ``find`` is not a non-empty string, - ``replace_with`` is not a string, ``paragraph`` is not a ref - string, or ``occurrence`` is negative or not an integer. + ``replace_with`` is not a string, ``paragraph`` is missing or + not a ref string, ``occurrence`` is negative or not an integer, + or ``find`` is a SearchResult and ``paragraph``/``occurrence`` + was given too. TextNotFoundError: If ``find`` is absent or ``occurrence`` is out of range for the paragraph. AmbiguousTextError: If ``occurrence`` is omitted and ``find`` matches more than once in the paragraph. - HashMismatchError: If the paragraph hash is stale. + HashMismatchError: If the paragraph hash is stale — including a + SearchResult whose paragraph was edited after the search. Example: new_ref = doc.replace("30 days", "60 days", paragraph="P2#f3c1") doc.replace("other text", "new text", paragraph=new_ref) + + # Straight from a search — no ref or occurrence bookkeeping: + match = doc.find_text("30 days") + doc.replace(match, "60 days") """ self._ensure_open() - _require_ref_string(paragraph) + find, paragraph, occurrence = _resolve_search_target( + find, paragraph, occurrence, ctx="replace(): ", field="'find'" + ) + paragraph = _require_ref_string(paragraph, "'find'") change_id = self._revision_manager.replace_text(find, replace_with, occurrence=occurrence, paragraph=paragraph) return self._edit_result(paragraph, self._revision_manager.group_id_of(change_id)) - def delete(self, text: str, *, paragraph: str, occurrence: int | None = None) -> EditResult: + def delete( + self, text: str | SearchResult, *, paragraph: str | None = None, occurrence: int | None = None + ) -> EditResult: """Mark text as deleted with tracked changes. Args: - text: Text to mark as deleted - paragraph: Paragraph reference from list_paragraphs() (e.g., "P2#f3c1") + text: Text to mark as deleted, or a + :class:`~docx_editor.track_changes.SearchResult` from + :meth:`find_text`/:meth:`find_all` — which supplies the text, + the paragraph and the occurrence, so pass neither + ``paragraph`` nor ``occurrence`` with one. + paragraph: Paragraph reference from list_paragraphs() (e.g., + "P2#f3c1"). Required unless ``text`` is a SearchResult. occurrence: Which occurrence within the paragraph (0 = first, 1 = second, etc.). Omitted → ``text`` must be unique in the paragraph, else AmbiguousTextError. @@ -1002,7 +1138,9 @@ def delete(self, text: str, *, paragraph: str, occurrence: int | None = None) -> Raises: ValueError: If ``text`` is not a non-empty string, ``paragraph`` - is not a ref string, or ``occurrence`` is negative or not an integer. + is missing or not a ref string, ``occurrence`` is negative or + not an integer, or ``text`` is a SearchResult and + ``paragraph``/``occurrence`` was given too. TextNotFoundError: If ``text`` is absent or ``occurrence`` is out of range for the paragraph. AmbiguousTextError: If ``occurrence`` is omitted and ``text`` @@ -1011,19 +1149,35 @@ def delete(self, text: str, *, paragraph: str, occurrence: int | None = None) -> Example: new_ref = doc.delete("obsolete clause", paragraph="P2#f3c1") + doc.delete(doc.find_text("obsolete clause")) # same edit, from a search """ self._ensure_open() - _require_ref_string(paragraph) + text, paragraph, occurrence = _resolve_search_target( + text, paragraph, occurrence, ctx="delete(): ", field="'text'" + ) + paragraph = _require_ref_string(paragraph, "'text'") change_id = self._revision_manager.suggest_deletion(text, occurrence=occurrence, paragraph=paragraph) return self._edit_result(paragraph, self._revision_manager.group_id_of(change_id)) - def insert_after(self, anchor: str, text: str, *, paragraph: str, occurrence: int | None = None) -> EditResult: + def insert_after( + self, + anchor: str | SearchResult, + text: str, + *, + paragraph: str | None = None, + occurrence: int | None = None, + ) -> EditResult: """Insert text after anchor with tracked changes. Args: - anchor: Text to find as insertion point + anchor: Text to find as insertion point, or a + :class:`~docx_editor.track_changes.SearchResult` from + :meth:`find_text`/:meth:`find_all` — which supplies the anchor + text, its paragraph and its occurrence, so pass neither + ``paragraph`` nor ``occurrence`` with one. text: Text to insert after the anchor - paragraph: Paragraph reference from list_paragraphs() (e.g., "P2#f3c1") + paragraph: Paragraph reference from list_paragraphs() (e.g., + "P2#f3c1"). Required unless ``anchor`` is a SearchResult. occurrence: Which occurrence of anchor within the paragraph (0 = first). Omitted → ``anchor`` must be unique in the paragraph, else AmbiguousTextError. @@ -1035,8 +1189,9 @@ def insert_after(self, anchor: str, text: str, *, paragraph: str, occurrence: in Raises: ValueError: If ``anchor`` is not a non-empty string, ``text`` is - not a string, ``paragraph`` is not a ref string, or - ``occurrence`` is negative or not an integer. + not a string, ``paragraph`` is missing or not a ref string, + ``occurrence`` is negative or not an integer, or ``anchor`` is + a SearchResult and ``paragraph``/``occurrence`` was given too. TextNotFoundError: If ``anchor`` is absent or ``occurrence`` is out of range for the paragraph. AmbiguousTextError: If ``occurrence`` is omitted and ``anchor`` @@ -1045,19 +1200,35 @@ def insert_after(self, anchor: str, text: str, *, paragraph: str, occurrence: in Example: new_ref = doc.insert_after("Section 5", " (as amended)", paragraph="P2#f3c1") + doc.insert_after(doc.find_text("Section 5"), " (as amended)") """ self._ensure_open() - _require_ref_string(paragraph) + anchor, paragraph, occurrence = _resolve_search_target( + anchor, paragraph, occurrence, ctx="insert_after(): ", field="'anchor'" + ) + paragraph = _require_ref_string(paragraph, "'anchor'") change_id = self._revision_manager.insert_text_after(anchor, text, occurrence=occurrence, paragraph=paragraph) return self._edit_result(paragraph, self._revision_manager.group_id_of(change_id)) - def insert_before(self, anchor: str, text: str, *, paragraph: str, occurrence: int | None = None) -> EditResult: + def insert_before( + self, + anchor: str | SearchResult, + text: str, + *, + paragraph: str | None = None, + occurrence: int | None = None, + ) -> EditResult: """Insert text before anchor with tracked changes. Args: - anchor: Text to find as insertion point + anchor: Text to find as insertion point, or a + :class:`~docx_editor.track_changes.SearchResult` from + :meth:`find_text`/:meth:`find_all` — which supplies the anchor + text, its paragraph and its occurrence, so pass neither + ``paragraph`` nor ``occurrence`` with one. text: Text to insert before the anchor - paragraph: Paragraph reference from list_paragraphs() (e.g., "P2#f3c1") + paragraph: Paragraph reference from list_paragraphs() (e.g., + "P2#f3c1"). Required unless ``anchor`` is a SearchResult. occurrence: Which occurrence of anchor within the paragraph (0 = first). Omitted → ``anchor`` must be unique in the paragraph, else AmbiguousTextError. @@ -1069,8 +1240,9 @@ def insert_before(self, anchor: str, text: str, *, paragraph: str, occurrence: i Raises: ValueError: If ``anchor`` is not a non-empty string, ``text`` is - not a string, ``paragraph`` is not a ref string, or - ``occurrence`` is negative or not an integer. + not a string, ``paragraph`` is missing or not a ref string, + ``occurrence`` is negative or not an integer, or ``anchor`` is + a SearchResult and ``paragraph``/``occurrence`` was given too. TextNotFoundError: If ``anchor`` is absent or ``occurrence`` is out of range for the paragraph. AmbiguousTextError: If ``occurrence`` is omitted and ``anchor`` @@ -1079,9 +1251,13 @@ def insert_before(self, anchor: str, text: str, *, paragraph: str, occurrence: i Example: new_ref = doc.insert_before("Section 6", "New clause: ", paragraph="P2#f3c1") + doc.insert_before(doc.find_text("Section 6"), "New clause: ") """ self._ensure_open() - _require_ref_string(paragraph) + anchor, paragraph, occurrence = _resolve_search_target( + anchor, paragraph, occurrence, ctx="insert_before(): ", field="'anchor'" + ) + paragraph = _require_ref_string(paragraph, "'anchor'") change_id = self._revision_manager.insert_text_before(anchor, text, occurrence=occurrence, paragraph=paragraph) return self._edit_result(paragraph, self._revision_manager.group_id_of(change_id)) @@ -1157,7 +1333,10 @@ def batch_edit( with updated hashes), in input order. Each operation gets its own revision group — accept one op and reject another via accept_group()/reject_group(). - When dry_run is True: list of EditValidationResult, one per operation. + When dry_run is True: list of EditValidationResult, one per + operation. A row that failed on a stale hash carries + ``current_ref``, the ref for that paragraph's current content — + rebuild the operation with it instead of parsing ``error``. Raises: ValueError: If ``operations`` is not a list at all (e.g. None or @@ -1183,6 +1362,13 @@ def batch_edit( results = doc.batch_edit(ops, dry_run=True) if all(r.valid for r in results): doc.batch_edit(ops) + + # Repair the stale-hash rows and retry, no message parsing: + for row in results: + if row.current_ref: + ops[row.index] = EditOperation.replace( + "old", "new", paragraph=row.current_ref + ) """ self._ensure_open() if not isinstance(operations, list): @@ -1293,7 +1479,7 @@ def batch_rewrite(self, rewrites: list[tuple[str, str]]) -> list[EditResult]: def add_comment( self, - anchor_text: str, + anchor_text: str | SearchResult, comment: str, *, paragraph: str | None = None, @@ -1307,7 +1493,11 @@ def add_comment( splits, ``w:ins`` wrappers) are found. Args: - anchor_text: Text to attach the comment to. + anchor_text: Text to attach the comment to, or a + :class:`~docx_editor.track_changes.SearchResult` from + :meth:`find_text`/:meth:`find_all` — which supplies the anchor + text, its paragraph and its occurrence, so pass neither + ``paragraph`` nor ``occurrence`` with one. comment: The comment content. paragraph: Optional paragraph reference (e.g., ``"P3#a7b2"``) to scope the search. ``None`` searches the whole document. @@ -1326,13 +1516,21 @@ def add_comment( HashMismatchError: If ``paragraph``'s hash is stale. CommentError: If ``anchor_text`` is not a non-empty string, or ``comment`` is not a string. - ValueError: If ``occurrence`` is negative or not an integer. + ValueError: If ``occurrence`` is negative or not an integer, or + ``anchor_text`` is a SearchResult and + ``paragraph``/``occurrence`` was given too. Example: doc.add_comment("Section 5", "Please review this section") doc.add_comment("foo", "Note", paragraph="P3#a7b2", occurrence=1) + + # Comment exactly the match a search found (2nd "foo", say): + doc.add_comment(doc.find_all("foo")[1], "Note") """ self._ensure_open() + anchor_text, paragraph, occurrence = _resolve_search_target( + anchor_text, paragraph, occurrence, ctx="add_comment(): ", field="'anchor_text'" + ) return self._comment_manager.add_comment(anchor_text, comment, paragraph=paragraph, occurrence=occurrence) def reply_to_comment(self, comment_id: int, reply: str) -> int: diff --git a/docx_editor/session.py b/docx_editor/session.py index 2cd41aa..c8d72e4 100644 --- a/docx_editor/session.py +++ b/docx_editor/session.py @@ -345,20 +345,39 @@ def _strip_internal_paths(text: str) -> str: @dataclass class ExecResult: - """Outcome of one exec_code() call against the session kernel.""" + """Outcome of one exec_code() call against the session kernel. + + ``started`` records whether the kernel began executing this request at all, + which disambiguates two statuses: + + - ``"timeout"``: with ``started=True`` your code ran and was still running + when the clock ran out (raise ``timeout``, or make the code faster); with + ``started=False`` the request never left the queue, because the kernel was + still busy with an earlier command — nothing of yours executed, so wait or + check what is hogging the session. + - ``"ok"`` with ``started=False``: the kernel **discarded** the request + without running it. ipykernel aborts everything still queued behind a + command that raises, so a request sent while an earlier one was failing + comes back clean and empty having done nothing. Re-send it. + + An ``"error"`` result, and any ``"ok"`` with ``started=True``, did execute. + """ status: ExecStatus stdout: str = "" stderr: str = "" result: str | None = None # repr of the last expression, if any traceback: str | None = None # ANSI-stripped traceback when status == "error" + started: bool = False # True once the kernel began executing this request def exec_code(code: str, connection_file: Path = DEFAULT_CONNECTION_FILE, timeout: float = 120.0) -> ExecResult: """Execute code in the session kernel and collect its output. If the kernel is busy, the request queues behind the running one; `timeout` - covers the whole wait. + covers the whole wait. A timeout therefore has two flavours, told apart by + the result's ``started`` flag: your own code ran too long (True), or it never + got out of the queue (False). Args: code: Python source to execute in the session @@ -367,7 +386,10 @@ def exec_code(code: str, connection_file: Path = DEFAULT_CONNECTION_FILE, timeou Returns: ExecResult with status "ok", "error", "timeout", or "dead" (the kernel - died mid-execution — its state is lost). + died mid-execution — its state is lost), and ``started`` telling whether + the kernel began executing this request at all. Check ``started`` on an + "ok" result too: ``ok`` with ``started=False`` means the kernel aborted + the request behind a failing predecessor and ran nothing. Raises: FileNotFoundError: If no session connection file exists. @@ -398,6 +420,7 @@ def exec_code(code: str, connection_file: Path = DEFAULT_CONNECTION_FILE, timeou result: str | None = None traceback: str | None = None status: ExecStatus = "ok" + started = False deadline = time.monotonic() + timeout last_activity = time.monotonic() @@ -405,7 +428,12 @@ def exec_code(code: str, connection_file: Path = DEFAULT_CONNECTION_FILE, timeou remaining = deadline - time.monotonic() if remaining <= 0: final: ExecStatus = "dead" if _kernel_dead(kc, connection_file) else "timeout" - return ExecResult(status=final, stdout="".join(stdout_parts), stderr="".join(stderr_parts)) + return ExecResult( + status=final, + stdout="".join(stdout_parts), + stderr="".join(stderr_parts), + started=started, + ) try: msg = kc.get_iopub_msg(timeout=min(remaining, 1.0)) except Empty: @@ -414,7 +442,12 @@ def exec_code(code: str, connection_file: Path = DEFAULT_CONNECTION_FILE, timeou if time.monotonic() - last_activity < _SILENCE_PROBE_AFTER: continue if _kernel_dead(kc, connection_file): - return ExecResult(status="dead", stdout="".join(stdout_parts), stderr="".join(stderr_parts)) + return ExecResult( + status="dead", + stdout="".join(stdout_parts), + stderr="".join(stderr_parts), + started=started, + ) last_activity = time.monotonic() continue # Any iopub traffic proves the kernel is alive, whoever it belongs to. @@ -424,7 +457,12 @@ def exec_code(code: str, connection_file: Path = DEFAULT_CONNECTION_FILE, timeou msg_type = msg["msg_type"] content = msg["content"] - if msg_type == "stream": + if msg_type == "execute_input": + # The kernel broadcasts this the moment it starts executing OUR + # request — the only reliable "left the queue" signal, since a + # queued request is otherwise indistinguishable from a slow one. + started = True + elif msg_type == "stream": target = stdout_parts if content["name"] == "stdout" else stderr_parts target.append(content["text"]) elif msg_type in ("execute_result", "display_data"): @@ -441,6 +479,7 @@ def exec_code(code: str, connection_file: Path = DEFAULT_CONNECTION_FILE, timeou stderr="".join(stderr_parts), result=result, traceback=traceback, + started=started, ) finally: kc.stop_channels() @@ -529,6 +568,7 @@ class EvalResult: stderr: str = "" traceback: str | None = None error: dict[str, Any] | None = None # {"type", "message", } when the expression raised + started: bool = False # as in ExecResult: False on a timeout means the request never left the queue def eval_code(expr: str, connection_file: Path = DEFAULT_CONNECTION_FILE, timeout: float = 120.0) -> EvalResult: @@ -564,8 +604,21 @@ def eval_code(expr: str, connection_file: Path = DEFAULT_CONNECTION_FILE, timeou """ res = exec_code(_EVAL_TEMPLATE % (repr(expr),), connection_file=connection_file, timeout=timeout) if res.status != "ok": - return EvalResult(status=res.status, stdout=res.stdout, stderr=res.stderr, traceback=res.traceback) + return EvalResult( + status=res.status, + stdout=res.stdout, + stderr=res.stderr, + traceback=res.traceback, + started=res.started, + ) if res.result is None: + if not res.started: + # Not a transport fault: the kernel discarded the request without + # running it (see ExecResult.started), so there is no reply to decode. + raise SessionError( + "The kernel discarded this request without running it — it was queued behind a command " + "that raised. Nothing was evaluated; re-send it." + ) raise SessionError("eval transport failed: kernel returned no result for the eval wrapper") try: payload = json.loads(ast.literal_eval(res.result)) @@ -578,6 +631,7 @@ def eval_code(expr: str, connection_file: Path = DEFAULT_CONNECTION_FILE, timeou stderr=res.stderr, traceback=_strip_internal_paths(payload["traceback"]), error=payload["error"], + started=res.started, ) return EvalResult( status="ok", @@ -585,6 +639,7 @@ def eval_code(expr: str, connection_file: Path = DEFAULT_CONNECTION_FILE, timeou serialized=payload["serialized"], stdout=res.stdout, stderr=res.stderr, + started=res.started, ) @@ -738,11 +793,25 @@ def _run(args: argparse.Namespace) -> int: print(_KERNEL_DIED_MSG, file=sys.stderr) return EXIT_KERNEL_DEAD if res.status == "timeout": + if res.started: + message = f"Timed out after {args.timeout}s (kernel still running; the command may still finish)." + else: + message = ( + f"Timed out after {args.timeout}s while still queued — your code never started. The kernel is " + "busy with an earlier command; wait for it, raise --timeout, or check 'docx-session status'." + ) + print(message, file=sys.stderr) + return EXIT_TIMEOUT + if not res.started: + # "ok" but never dequeued: the kernel discards everything queued behind + # a command that raises, so this returned clean without running. Exit + # code stays EXIT_OK (changing it is a contract change), which is + # exactly why the warning has to be loud. print( - f"Timed out after {args.timeout}s (kernel still running; the command may still finish).", + "Warning: the kernel discarded this request without running it — it was queued behind a " + "command that raised. Nothing was executed; re-send it.", file=sys.stderr, ) - return EXIT_TIMEOUT return EXIT_OK diff --git a/docx_editor/track_changes.py b/docx_editor/track_changes.py index a00727e..a32fc6d 100644 --- a/docx_editor/track_changes.py +++ b/docx_editor/track_changes.py @@ -70,6 +70,13 @@ class EditOperation: construction time with the same rules ``batch_edit`` applies, so mistakes surface immediately instead of at apply time. The raw ``EditOperation(action=..., ...)`` form remains supported. + + Each typed constructor also accepts a + :class:`~docx_editor.track_changes.SearchResult` in place of its target + text, which fills in ``paragraph`` and ``occurrence`` from the match — + ``[EditOperation.replace(m, "60 days") for m in doc.find_all("30 days")]`` + needs no ref or occurrence bookkeeping. The dataclass fields stay plain + strings either way. """ action: Literal["replace", "delete", "insert_after", "insert_before"] @@ -81,30 +88,58 @@ class EditOperation: occurrence: int | None = None # None = target must be unique in the paragraph @staticmethod - def _validate_common(constructor: str, paragraph: str, occurrence: int | None) -> None: - """Construction-time checks shared by all typed constructors.""" + def _validate_common(constructor: str, paragraph: str | None, occurrence: int | None, field: str) -> str: + """Construction-time checks shared by all typed constructors. + + Returns the validated ref, so callers can hand a narrowed ``str`` to the + dataclass field (``paragraph`` is Optional in the signatures only to + allow the SearchResult form). + """ + if paragraph is None: + # Mirrors ParagraphRef.parse's non-string wording (which handles every + # other bad ref below) so the None case reads identically, plus the + # hint that a SearchResult would have supplied the ref. + raise ValueError( + f"Invalid paragraph reference None: expected a string like 'P3#a7b2', got NoneType — " + f"{_paragraph_hint(field)}" + ) ParagraphRef.parse(paragraph) _require_valid_occurrence(occurrence, f"EditOperation.{constructor}(): ") + return paragraph @classmethod - def replace(cls, find: str, replace_with: str, *, paragraph: str, occurrence: int | None = None) -> "EditOperation": + def replace( + cls, + find: "str | SearchResult", + replace_with: str, + *, + paragraph: str | None = None, + occurrence: int | None = None, + ) -> "EditOperation": """Build a validated replace operation (mirrors ``Document.replace``). Args: - find: Text to find and replace (must be non-empty) + find: Text to find and replace (must be non-empty), or a + :class:`SearchResult` from find_text()/find_all() — which also + supplies ``paragraph`` and ``occurrence``, so pass neither. replace_with: Replacement text (empty string allowed — replacing with nothing is a valid tracked deletion) - paragraph: Paragraph reference from list_paragraphs() (e.g., "P2#f3c1") + paragraph: Paragraph reference from list_paragraphs() (e.g., "P2#f3c1"). + Required unless ``find`` is a SearchResult. occurrence: Which occurrence within the paragraph (0 = first). Omitted → ``find`` must be unique in the paragraph, else the batch fails with a wrapped AmbiguousTextError at apply time. Raises: - ValueError: If the paragraph ref is malformed, ``occurrence`` is - not a non-negative integer, ``find`` is not a non-empty - string, or ``replace_with`` is not a string. + ValueError: If ``paragraph`` is missing or malformed, ``occurrence`` + is not a non-negative integer, ``find`` is not a non-empty + string, ``replace_with`` is not a string, or ``find`` is a + SearchResult and ``paragraph``/``occurrence`` was given too. """ - cls._validate_common("replace", paragraph, occurrence) + find, paragraph, occurrence = _resolve_search_target( + find, paragraph, occurrence, ctx="EditOperation.replace(): ", field="'find'" + ) + paragraph = cls._validate_common("replace", paragraph, occurrence, "'find'") if not isinstance(find, str) or not find: raise ValueError( f"EditOperation.replace(): 'find' must be a non-empty string — the text to search for, got {find!r}" @@ -125,22 +160,31 @@ def replace(cls, find: str, replace_with: str, *, paragraph: str, occurrence: in ) @classmethod - def delete(cls, text: str, *, paragraph: str, occurrence: int | None = None) -> "EditOperation": + def delete( + cls, text: "str | SearchResult", *, paragraph: str | None = None, occurrence: int | None = None + ) -> "EditOperation": """Build a validated delete operation (mirrors ``Document.delete``). Args: - text: Text to mark as deleted (must be non-empty) - paragraph: Paragraph reference from list_paragraphs() (e.g., "P2#f3c1") + text: Text to mark as deleted (must be non-empty), or a + :class:`SearchResult` from find_text()/find_all() — which also + supplies ``paragraph`` and ``occurrence``, so pass neither. + paragraph: Paragraph reference from list_paragraphs() (e.g., "P2#f3c1"). + Required unless ``text`` is a SearchResult. occurrence: Which occurrence within the paragraph (0 = first). Omitted → ``text`` must be unique in the paragraph, else the batch fails with a wrapped AmbiguousTextError at apply time. Raises: - ValueError: If the paragraph ref is malformed, ``occurrence`` is - not a non-negative integer, or ``text`` is not a non-empty - string. + ValueError: If ``paragraph`` is missing or malformed, ``occurrence`` + is not a non-negative integer, ``text`` is not a non-empty + string, or ``text`` is a SearchResult and + ``paragraph``/``occurrence`` was given too. """ - cls._validate_common("delete", paragraph, occurrence) + text, paragraph, occurrence = _resolve_search_target( + text, paragraph, occurrence, ctx="EditOperation.delete(): ", field="'text'" + ) + paragraph = cls._validate_common("delete", paragraph, occurrence, "'text'") if not isinstance(text, str) or not text: raise ValueError( f"EditOperation.delete(): 'text' must be a non-empty string — the text to mark as deleted, got {text!r}" @@ -152,12 +196,15 @@ def delete(cls, text: str, *, paragraph: str, occurrence: int | None = None) -> def _insert( cls, action: Literal["insert_after", "insert_before"], - anchor: str, + anchor: "str | SearchResult", text: str, - paragraph: str, + paragraph: str | None, occurrence: int | None, ) -> "EditOperation": - cls._validate_common(action, paragraph, occurrence) + anchor, paragraph, occurrence = _resolve_search_target( + anchor, paragraph, occurrence, ctx=f"EditOperation.{action}(): ", field="'anchor'" + ) + paragraph = cls._validate_common(action, paragraph, occurrence, "'anchor'") if not isinstance(anchor, str) or not anchor: raise ValueError( f"EditOperation.{action}(): 'anchor' must be a non-empty string — the text to insert near, " @@ -172,46 +219,88 @@ def _insert( return cls(action=action, paragraph=paragraph, anchor=anchor, text=text, occurrence=occurrence) @classmethod - def insert_after(cls, anchor: str, text: str, *, paragraph: str, occurrence: int | None = None) -> "EditOperation": + def insert_after( + cls, + anchor: "str | SearchResult", + text: str, + *, + paragraph: str | None = None, + occurrence: int | None = None, + ) -> "EditOperation": """Build a validated insert_after operation (mirrors ``Document.insert_after``). Args: - anchor: Text to find as insertion point (must be non-empty) + anchor: Text to find as insertion point (must be non-empty), or a + :class:`SearchResult` from find_text()/find_all() — which also + supplies ``paragraph`` and ``occurrence``, so pass neither. text: Text to insert after the anchor - paragraph: Paragraph reference from list_paragraphs() (e.g., "P2#f3c1") + paragraph: Paragraph reference from list_paragraphs() (e.g., "P2#f3c1"). + Required unless ``anchor`` is a SearchResult. occurrence: Which occurrence of anchor within the paragraph (0 = first). Omitted → ``anchor`` must be unique in the paragraph, else the batch fails with a wrapped AmbiguousTextError at apply time. Raises: - ValueError: If the paragraph ref is malformed, ``occurrence`` is - not a non-negative integer, ``anchor`` is not a non-empty - string, or ``text`` is not a string. + ValueError: If ``paragraph`` is missing or malformed, ``occurrence`` + is not a non-negative integer, ``anchor`` is not a non-empty + string, ``text`` is not a string, or ``anchor`` is a + SearchResult and ``paragraph``/``occurrence`` was given too. """ return cls._insert("insert_after", anchor, text, paragraph, occurrence) @classmethod - def insert_before(cls, anchor: str, text: str, *, paragraph: str, occurrence: int | None = None) -> "EditOperation": + def insert_before( + cls, + anchor: "str | SearchResult", + text: str, + *, + paragraph: str | None = None, + occurrence: int | None = None, + ) -> "EditOperation": """Build a validated insert_before operation (mirrors ``Document.insert_before``). Args: - anchor: Text to find as insertion point (must be non-empty) + anchor: Text to find as insertion point (must be non-empty), or a + :class:`SearchResult` from find_text()/find_all() — which also + supplies ``paragraph`` and ``occurrence``, so pass neither. text: Text to insert before the anchor - paragraph: Paragraph reference from list_paragraphs() (e.g., "P2#f3c1") + paragraph: Paragraph reference from list_paragraphs() (e.g., "P2#f3c1"). + Required unless ``anchor`` is a SearchResult. occurrence: Which occurrence of anchor within the paragraph (0 = first). Omitted → ``anchor`` must be unique in the paragraph, else the batch fails with a wrapped AmbiguousTextError at apply time. Raises: - ValueError: If the paragraph ref is malformed, ``occurrence`` is - not a non-negative integer, ``anchor`` is not a non-empty - string, or ``text`` is not a string. + ValueError: If ``paragraph`` is missing or malformed, ``occurrence`` + is not a non-negative integer, ``anchor`` is not a non-empty + string, ``text`` is not a string, or ``anchor`` is a + SearchResult and ``paragraph``/``occurrence`` was given too. """ return cls._insert("insert_before", anchor, text, paragraph, occurrence) +@dataclass(frozen=True) +class _ValidationOutcome: + """Internal result of validating one operation: why it failed, and the ref + that re-points it at the paragraph's current content when the failure was a + stale hash. + + ``current_ref`` clears the *hash* check only — it says nothing about whether + the operation's target text still exists, or is still unique, in that + paragraph. A rebuilt operation is re-validated like any other (and fails + loudly, atomically, at apply time if the target moved on). + + Named rather than a bare 2-tuple because both fields are ``str | None`` — + positional unpacking would be one transposition away from reporting the + recovery ref as the error message. + """ + + error: str | None # None when the operation would apply cleanly + current_ref: str | None = None # set only for stale-hash failures + + def _not_an_edit_operation_message(op: object) -> str: """Shared batch_edit/validate_batch message for a non-EditOperation element, so the raising and never-raises paths cannot drift apart.""" @@ -223,12 +312,22 @@ def _not_an_edit_operation_message(op: object) -> str: @dataclass class EditValidationResult: - """Outcome of validating one EditOperation in a dry-run batch.""" + """Outcome of validating one EditOperation in a dry-run batch. + + ``current_ref`` is the recovery field for the one failure a caller can fix + mechanically: a stale hash. It holds the ref that targets the same paragraph + at its *current* content (e.g. ``"P7#c4d8"`` when the operation carried + ``"P7#a7b2"``), so a retry is ``EditOperation.replace(..., paragraph= + row.current_ref)`` — no parsing the hash out of ``error``'s prose. It is + ``None`` for every other outcome: valid rows, malformed refs, out-of-range + indexes, missing or ambiguous target text, and non-EditOperation elements. + """ index: int # 0-based position in the input operations list paragraph: str | None # the operation's paragraph ref (None if it was missing) valid: bool # True if the op would apply cleanly error: str | None = None # human-readable reason when not valid + current_ref: str | None = None # ref for the paragraph's current content, stale-hash rows only class EditResult(str): @@ -335,6 +434,74 @@ def __repr__(self) -> str: return f"SearchResult({self.paragraph_ref} occ={self.paragraph_occurrence} {text!r}{spans})" +def _resolve_search_target( + target: "str | SearchResult", + paragraph: str | None, + occurrence: int | None, + *, + ctx: str, + field: str, +) -> tuple[str, str | None, int | None]: + """Normalize a text-or-SearchResult edit target into ``(text, paragraph, occurrence)``. + + A plain string passes through untouched, so every existing call site keeps + its exact semantics. A :class:`SearchResult` supplies all three values — + its ``text``, its ``paragraph_ref`` and its ``paragraph_occurrence`` — + which is what makes ``doc.replace(match, "60 days")`` mean exactly what + spelling those three fields out by hand means. + + Passing a SearchResult *and* ``paragraph=``/``occurrence=`` is a + contradiction rather than a merge (which of the two paragraphs wins?), so + it raises instead of silently preferring one. + + Args: + target: Text to locate, or a SearchResult that already located it. + paragraph: The call's ``paragraph=`` argument (None when omitted). + occurrence: The call's ``occurrence=`` argument (None when omitted). + ctx: Message prefix naming the caller, e.g. ``"replace(): "``. + field: Display name of the target parameter, e.g. ``"'find'"``. + + Returns: + ``(text, paragraph, occurrence)``, for the caller to validate exactly + as it validates hand-written arguments. A plain tuple rather than a + named object (cf. :class:`_ValidationOutcome`) because it is never + carried: every call site spreads it straight back into the three + distinctly-named parameters it came from, on the next line. + + Raises: + ValueError: If ``target`` is a SearchResult and ``paragraph`` or + ``occurrence`` was given too. + """ + if not isinstance(target, SearchResult): + return target, paragraph, occurrence + redundant = [ + name + for name, given in (("paragraph=", paragraph is not None), ("occurrence=", occurrence is not None)) + if given + ] + if redundant: + raise ValueError( + f"{ctx}{field} is a SearchResult, which already pins the paragraph ({target.paragraph_ref!r}) " + f"and the occurrence ({target.paragraph_occurrence}) — drop {' and '.join(redundant)}" + ) + return target.text, target.paragraph_ref, target.paragraph_occurrence + + +def _paragraph_hint(field: str) -> str: + """Trailing hint shared by the "paragraph ref is missing" messages. + + ``paragraph`` is optional in the *signatures* only because a SearchResult + target supplies it (see :func:`_resolve_search_target`); an edit still has to + know which paragraph it targets. Document's methods and EditOperation's + constructors keep their own long-standing message prefixes (both are pinned + by tests) and share this recovery hint, so the advice cannot drift. + """ + return ( + f"pass a ref from list_paragraphs()/find_text(), or pass a SearchResult as {field} " + f"(it carries both the paragraph and the occurrence)" + ) + + @dataclass(frozen=True) class _LocatedMatch: """Internal: a text-map match plus the paragraph identity needed for refs.""" @@ -1278,7 +1445,10 @@ def validate_batch(self, operations: list[EditOperation]) -> list[EditValidation Returns: One EditValidationResult per operation, in input order. An element that is not an EditOperation at all comes back as an invalid - result (``paragraph=None``), never as an exception. + result (``paragraph=None``), never as an exception. Rows that failed + on a stale hash also carry ``current_ref`` — the ref for that + paragraph's current content — so the caller can retry without + parsing the message. """ if not operations: return [] @@ -1292,19 +1462,20 @@ def validate_batch(self, operations: list[EditOperation]) -> list[EditValidation EditValidationResult(index=i, paragraph=None, valid=False, error=_not_an_edit_operation_message(op)) ) continue - error = self._validate_single(op, paragraphs) + outcome = self._validate_single(op, paragraphs) results.append( EditValidationResult( index=i, paragraph=op.paragraph, - valid=error is None, - error=error, + valid=outcome.error is None, + error=outcome.error, + current_ref=outcome.current_ref, ) ) return results - def _validate_single(self, op: EditOperation, paragraphs: list[Element] | None = None) -> str | None: - """Return an error message if ``op`` would fail, or None if it is valid. + def _validate_single(self, op: EditOperation, paragraphs: list[Element] | None = None) -> "_ValidationOutcome": + """Return why ``op`` would fail, or an all-None outcome if it is valid. Reuses ``_resolve_paragraph``, ``_resolve_action_target``, and ``_locate_in_paragraph`` — the same helpers ``_apply_single_edit`` uses — @@ -1314,17 +1485,22 @@ def _validate_single(self, op: EditOperation, paragraphs: list[Element] | None = (see validate_batch); None fetches fresh. """ if not op.paragraph: - return "paragraph reference is required for batch mode" + return _ValidationOutcome(error="paragraph reference is required for batch mode") try: ref = ParagraphRef.parse(op.paragraph) except ValueError as e: - return str(e) + return _ValidationOutcome(error=str(e)) try: p = self._resolve_paragraph(ref, paragraphs) - except (ParagraphIndexError, HashMismatchError) as e: - return str(e) + except HashMismatchError as e: + # The one mechanically fixable failure: hand back the ref that + # targets this paragraph's current content (EditValidationResult + # .current_ref) so callers never regex the hash out of the prose. + return _ValidationOutcome(error=str(e), current_ref=f"P{e.paragraph_index}#{e.actual_hash}") + except ParagraphIndexError as e: + return _ValidationOutcome(error=str(e)) # Resolve required args + the text this op must locate via the same # helper _apply_single_edit uses (which also rejects a negative @@ -1333,14 +1509,14 @@ def _validate_single(self, op: EditOperation, paragraphs: list[Element] | None = try: target = self._resolve_action_target(op) except ValueError as e: - return str(e) + return _ValidationOutcome(error=str(e)) try: self._locate_in_paragraph(p, op.paragraph, target, op.occurrence) except (ValueError, DocxEditError) as e: - return str(e) + return _ValidationOutcome(error=str(e)) - return None + return _ValidationOutcome(error=None) def batch_rewrite(self, rewrites: list[tuple[str, str]]) -> list[int | None]: """Rewrite multiple paragraphs with upfront hash validation. @@ -3722,6 +3898,15 @@ def _trim_replace_affixes(find: str, replace_with: str) -> tuple[int, int]: remainder after the prefix, so a token shared at both ends is never consumed twice. + One exception is character-granular: when what remains on *both* sides after + the word-level trim is nothing but whitespace, the shared characters of that + whitespace are trimmed too. A single-to-double space edit then becomes a pure + one-space insertion instead of a ``del " "`` + ``ins " "`` pair that renders + as an invisible, unreviewable redline (ISSUES.md #60). The gate keeps this + away from word redlines, where character trimming would be actively harmful: + ``"30 days"`` → ``"60 days"`` must stay one whole-word replacement, not + ``del "3"`` + ``ins "6"``. + Returns: ``(prefix_len, suffix_len)`` in characters. """ @@ -3737,9 +3922,27 @@ def _trim_replace_affixes(find: str, replace_with: str) -> tuple[int, int]: prefix_len = sum(len(tok) for tok in f_toks[:i]) suffix_len = sum(len(tok) for tok in f_toks[len(f_toks) - j :]) + + f_rest = find[prefix_len : len(find) - suffix_len] + r_rest = replace_with[prefix_len : len(replace_with) - suffix_len] + if f_rest and r_rest and not f_rest.strip() and not r_rest.strip(): + prefix_len += _common_prefix_len(f_rest, r_rest) + # Re-slice: the prefix just grew, and the shared characters it consumed + # must not be counted again from the other end. + f_rest = find[prefix_len : len(find) - suffix_len] + r_rest = replace_with[prefix_len : len(replace_with) - suffix_len] + suffix_len += _common_prefix_len(f_rest[::-1], r_rest[::-1]) return prefix_len, suffix_len +def _common_prefix_len(a: str, b: str) -> int: + """Number of leading characters ``a`` and ``b`` share.""" + n = 0 + while n < len(a) and n < len(b) and a[n] == b[n]: + n += 1 + return n + + def _set_xml_space_preserve(wt_elem) -> None: """Set xml:space='preserve' on a w:t element to preserve whitespace.""" wt_elem.setAttribute("xml:space", "preserve") diff --git a/docx_editor/xml_editor.py b/docx_editor/xml_editor.py index 95c39b6..f274a36 100644 --- a/docx_editor/xml_editor.py +++ b/docx_editor/xml_editor.py @@ -216,20 +216,72 @@ def parse(cls, ref: str) -> "ParagraphRef": @dataclass(frozen=True) class ParagraphInfo: - """Structured record for a paragraph: 1-based index, hash-anchored ref, and full visible text. + """Structured record for a paragraph: 1-based index, hash-anchored ref, full + visible text, and the paragraph's cheap structural facts. ``str(info)`` uses the same ``"P{i}#{hash}| {text}"`` delimiter format as - :meth:`Document.list_paragraphs`, always with the full, untruncated text. + :meth:`Document.list_paragraphs`, always with the full, untruncated text + (the structural fields are not part of that rendering). + + ``in_table``, ``style`` and ``outline_level`` carry exactly the meanings + :class:`ParagraphLocation` defines for them — that class stays the single + source of the semantics: + + - ``in_table``: True iff the paragraph lives inside a ```` cell. It + answers "is this a table cell?" without coordinates; use + :meth:`Document.get_paragraph_location` when you need table/row/column. + It is also what makes the paragraph-index-vs-python-docx divergence + self-describing: table-cell paragraphs count towards ``P{i}`` indexes. + - ``style``: the raw ``w:pPr/w:pStyle/@w:val`` style id (e.g. + ``"Heading1"``), None when the paragraph carries no explicit style. + - ``outline_level``: 0-based outline level (0 == Heading 1), resolving a + direct ``w:outlineLvl`` first and the paragraph style's level otherwise; + None means body text. Together with ``text`` this is enough to build a + table of contents from one ``list_paragraphs_structured`` call. + + List membership, table coordinates, heading paths and section indexes are + deliberately *not* here — they need whole-document context or extra parses; + :meth:`Document.get_paragraph_location` / + :meth:`Document.list_paragraph_locations` remain their home. """ index: int # 1-based paragraph index ref: str # "P{index}#{hash}" text: str # full visible text, untruncated + in_table: bool = False # True when the paragraph sits inside a cell + style: str | None = None # raw w:pStyle/@w:val, None when unstyled + outline_level: int | None = None # 0-based (0 == Heading 1), None = body text def __str__(self) -> str: return f"{self.ref}| {self.text}" +def _build_paragraph_info(paragraph, index: int, style_outlines: dict[str, int]) -> ParagraphInfo: + """Build the :class:`ParagraphInfo` record for one ```` element. + + Single source of the record for every producer + (:meth:`Document.list_paragraphs_structured`, :meth:`Document.get_paragraph` + and, through the former, :meth:`Document.context`), so their output cannot + drift. One text map serves both the hash and the text. + + ``style_outlines`` is the ``{style_id: outline_level}`` map from + :func:`_build_style_outline_map`, needed only to resolve style-defined + outline levels. An empty map is the graceful degradation for a document + with no ``word/styles.xml`` part (a direct ``w:outlineLvl`` still resolves), + and is what :meth:`Document._style_maps` already returns in that case. + """ + text_map = build_text_map(paragraph) + style = _extract_style(paragraph) + return ParagraphInfo( + index=index, + ref=f"P{index}#{compute_text_hash(text_map.text)}", + text=text_map.text, + in_table=_innermost_ancestor(paragraph, "w:tc") is not None, + style=style, + outline_level=_extract_outline_level(paragraph, style, style_outlines), + ) + + def compute_text_hash(text: str) -> str: """4-char hex CRC32 hash of a paragraph's visible text. @@ -1540,8 +1592,13 @@ def add_comment_extensible_date(elem) -> None: self._ensure_w16cex_namespace() elem.setAttribute("w16cex:dateUtc", timestamp) - def add_xml_space_to_t(elem) -> None: - # Add xml:space="preserve" to w:t if text has leading/trailing whitespace. + def add_xml_space_to_text_elem(elem) -> None: + # Add xml:space="preserve" to w:t/w:delText if the text has + # leading/trailing whitespace. w:delText needs it for the same + # reason w:t does: without it a conforming consumer trims the + # edges, so rejecting a deletion of " " would restore nothing — + # and _restore_deletion copies these attributes onto the w:t it + # rebuilds, which is where that loss would land. text = get_text_node_data(elem) if text and (text[0].isspace() or text[-1].isspace()): if not elem.hasAttribute("xml:space"): @@ -1556,8 +1613,8 @@ def add_xml_space_to_t(elem) -> None: add_rsid_to_p(node) elif node.tagName == "w:r": add_rsid_to_r(node) - elif node.tagName == "w:t": - add_xml_space_to_t(node) + elif node.tagName in ("w:t", "w:delText"): + add_xml_space_to_text_elem(node) elif node.tagName in ("w:ins", "w:del"): add_tracked_change_attrs(node) elif node.tagName == "w:comment": @@ -1570,8 +1627,9 @@ def add_xml_space_to_t(elem) -> None: add_rsid_to_p(elem) for elem in node.getElementsByTagName("w:r"): add_rsid_to_r(elem) - for elem in node.getElementsByTagName("w:t"): - add_xml_space_to_t(elem) + for tag in ("w:t", "w:delText"): + for elem in node.getElementsByTagName(tag): + add_xml_space_to_text_elem(elem) for tag in ("w:ins", "w:del"): for elem in node.getElementsByTagName(tag): add_tracked_change_attrs(elem) diff --git a/skills/docx/SKILL.md b/skills/docx/SKILL.md index 408d8e5..3188e15 100644 --- a/skills/docx/SKILL.md +++ b/skills/docx/SKILL.md @@ -77,7 +77,9 @@ Use **python-docx** to explore document structure before editing. This is useful > `doc.paragraphs` lists body-level paragraphs only (table cells excluded); > docx_editor `P{i}` refs number every paragraph including table cells — the > numberings diverge at the first table (a 3000-paragraph doc can have 3027 -> `P` refs), so `P{i}` ≠ `paragraphs[i-1]`. +> `P` refs), so `P{i}` ≠ `paragraphs[i-1]`. Each `ParagraphInfo` says which +> side of that divergence it is on: `info.in_table` is `True` exactly for the +> paragraphs python-docx skips. ```python from docx import Document @@ -267,22 +269,29 @@ for p in doc.list_paragraphs(): # P3#b2c4| Section 3. Terms and conditions... # Find text (returns SearchResult or None, works across element boundaries). -# Chain into an edit with paragraph=match.paragraph_ref plus -# occurrence=match.paragraph_occurrence (edits count occurrences per paragraph). match = doc.find_text("30 days") # Optionally scope to one paragraph — occurrence then counts within it: match = doc.find_text("30 days", paragraph="P2#f3c1") +# Chain straight into an edit: pass the SearchResult itself as the target and it +# supplies the text, the paragraph AND the occurrence. Do NOT also pass +# paragraph=/occurrence= with it (that raises — it would contradict the match): +doc.replace(match, "60 days") +# Every text-targeting method takes one: replace, delete, insert_after, +# insert_before, add_comment, and the EditOperation constructors. The explicit +# spelling stays valid — it is the SAME single edit the line above just made, +# not a follow-up (re-running it would raise HashMismatchError on the now-stale +# match): +# doc.replace(match.text, "60 days", paragraph=match.paragraph_ref, +# occurrence=match.paragraph_occurrence) + # Just need the count? doc.count_matches("30 days") returns the document-wide # occurrence total (no paragraph scope) without building SearchResults. # Enumerate EVERY match in one call (returns list[SearchResult], [] if none). -# Each result plugs straight into a follow-up edit — no occurrence math needed. # Edit them all in one atomic batch; reversed() puts same-paragraph ops in the # required DESCENDING occurrence order, safe however the matches are spread: -ops = [EditOperation.replace(r.text, "60 days", paragraph=r.paragraph_ref, - occurrence=r.paragraph_occurrence) - for r in reversed(doc.find_all("30 days"))] +ops = [EditOperation.replace(r, "60 days") for r in reversed(doc.find_all("30 days"))] doc.batch_edit(ops) # Optionally scope to one paragraph: doc.find_all("term", paragraph="P2#f3c1") # One-at-a-time doc.replace() per result also works when every paragraph has @@ -298,15 +307,33 @@ doc.batch_edit(ops) # Show the paragraphs AROUND a match ("what's the surrounding section?"). # window=2 (default) returns up to 5 ParagraphInfo records, clamped at the -# document edges; each prints as "P{i}#{hash}| full text": +# document edges; each prints as "P{i}#{hash}| full text". Note the re-find: +# the edits above made the earlier `match` stale, and a stale ref raises +# HashMismatchError wherever it is used — search again after editing. +match = doc.find_text("60 days") for info in doc.context(match.paragraph_ref, window=2): print(info) # Fetch ONE paragraph by number — single-item counterpart to # list_paragraphs_structured(). 1-based (P1 is index=1), returns ParagraphInfo -# with full untruncated text; ParagraphIndexError when out of range: +# with full untruncated text; ParagraphIndexError when out of range. +# Both get_paragraph() and context() are O(document) despite their fixed-size +# output (they walk every w:p). Fine per lookup; for many paragraphs call +# list_paragraphs_structured(limit=None) ONCE and index that list instead: info = doc.get_paragraph(match.paragraph_index) +# Every ParagraphInfo also carries the paragraph's cheap structural facts: +print(info.style) # raw w:pStyle id, e.g. "Heading1" (None = unstyled) +print(info.outline_level) # 0-based; 0 == Heading 1; None == body text +print(info.in_table) # True = the paragraph lives in a table cell +# So a table of contents needs ONE call, no list_paragraph_locations(): +toc = [(i.outline_level, i.text) for i in doc.list_paragraphs_structured(limit=None) + if i.outline_level is not None] +# in_table also makes the P-index divergence self-describing: table-cell +# paragraphs DO get P{i} refs (python-docx's doc.paragraphs skips them). +# Table coordinates, list numbering, heading paths and section indexes still +# need get_paragraph_location()/list_paragraph_locations() below. + # Get all visible text (inserted text included, deleted text excluded) visible = doc.get_visible_text() @@ -355,6 +382,10 @@ doc.delete("unnecessary clause", paragraph="P5#d4e5") doc.insert_after("Section 3.", " Additional terms apply.", paragraph="P3#b2c4") doc.insert_before("Section 3.", "See also: ", paragraph="P3#b2c4") +# A whitespace-only fix is written as the minimal revision — a pure insertion +# here, a pure deletion the other way round, never an invisible del+ins pair: +doc.replace("clause 2", "clause 2", paragraph="P5#d4e5") + # To accept/reject a specific edit as a unit, use its revision group: result = doc.replace("30 days", "60 days", paragraph="P2#f3c1") doc.reject_group(result.group_id) # undo the whole edit (del + ins together) @@ -393,8 +424,7 @@ d.save("report.docx") from docx_editor import Document doc = Document.open("report.docx", author="Reviewer") for r in reversed(doc.find_all("thirty days")): - doc.replace(r.text, "sixty days", paragraph=r.paragraph_ref, - occurrence=r.paragraph_occurrence) + doc.replace(r, "sixty days") # the match pins ref+occurrence locs = dict(doc.list_paragraph_locations()) # snapshot AFTER the edits report = {} @@ -747,9 +777,11 @@ everything = doc.list_paragraphs(limit=None) # uncapped, never a notice refs_only = doc.list_paragraphs(max_chars=0, limit=None) # every "P1#a7b2", no preview, no notice ``` -(`list_paragraphs_structured()` has the same 200-record default cap but appends **no notice** — every entry stays a typed `ParagraphInfo`. Detect truncation by checking whether the last record's `index` is still below `paragraph_count()` (robust for any `start`), or pass `limit=None`.) +(`list_paragraphs_structured()` has the same 200-record default cap but appends **no notice** — every entry stays a typed `ParagraphInfo` carrying `index`, `ref`, full `text`, plus `style`/`outline_level`/`in_table`. Detect truncation by checking whether the last record's `index` is still below `paragraph_count()` (robust for any `start`), or pass `limit=None`.) + +**Reaching the end of the pagination:** a `start` past the last paragraph is a valid request with an empty answer — both listing methods return `[]`, exactly like `lst[500:]` on a 300-item list, and **do not raise**. So `while page := doc.list_paragraphs(start=s, limit=200)` terminates cleanly. (`start=0` or negative *does* raise `ValueError`: those are invalid inputs, not empty ranges.) To know where the end is rather than discovering it, use `paragraph_count()` up front or follow the truncation notice's `start=` hint. -The `paragraph` argument is **required** for all edit methods. If the paragraph content has changed since you called `list_paragraphs()`, a `HashMismatchError` is raised — preventing edits to the wrong location. +The `paragraph` argument is **required** whenever the target is a plain string — `replace`, `delete`, `insert_after`, `insert_before` (`add_comment` may search document-wide). Pass a `SearchResult` as the target instead and it supplies the ref itself. Either way, if the paragraph content has changed since the ref was minted, a `HashMismatchError` is raised — preventing edits to the wrong location. **Every edit method returns an `EditResult` — a `str` subclass whose value is the new paragraph ref (plus `group_id`/`revision_ids`).** Chain edits without calling `list_paragraphs()` again: @@ -780,7 +812,7 @@ with Document.open("file.docx", author=author) as doc: doc.save() ``` -Build operations with the typed constructors (`EditOperation.replace/.delete/.insert_after/.insert_before` — same signatures as the `Document` methods). They validate arguments immediately and raise `ValueError` with a field-specific message, instead of failing later at apply time. +Build operations with the typed constructors (`EditOperation.replace/.delete/.insert_after/.insert_before` — same signatures as the `Document` methods, including the SearchResult form: `EditOperation.replace(match, "60 days")`). They validate arguments immediately and raise `ValueError` with a field-specific message, instead of failing later at apply time. **Single-exception contract:** whatever goes wrong with an operation — stale hash, malformed ref, missing text, ambiguous target — `batch_edit` (and @@ -790,8 +822,8 @@ typed exception. The batch is atomic: nothing is applied on failure. **Pre-flight with `dry_run=True`** — validates every operation without applying anything and returns `list[EditValidationResult]` (fields: `index`, `paragraph`, -`valid`, `error`), one per operation in input order. The document is left -unchanged (`ops` is the `EditOperation` list built above): +`valid`, `error`, `current_ref`), one per operation in input order. The document +is left unchanged (`ops` is the `EditOperation` list built above): ```python results = doc.batch_edit(ops, dry_run=True) @@ -803,6 +835,20 @@ else: print(f"op {r.index} on {r.paragraph}: {r.error}") ``` +`current_ref` is the recovery field for the one failure you can fix +mechanically: a **stale hash**. It holds the ref for that paragraph's *current* +content, so rebuild the operation with it — never regex the hash out of `error`. +It is `None` on every other row (valid ones included), which is also how you +tell a stale hash apart from a missing target: + +```python +for r in doc.batch_edit(ops, dry_run=True): + if r.current_ref: # stale hash → retry + ops[r.index] = EditOperation.replace("30 days", "60 days", paragraph=r.current_ref) + elif not r.valid: # text/ref problem → re-search + print(f"op {r.index} needs attention: {r.error}") +``` + Each operation is validated independently against the current document — `dry_run` does **not** simulate the sequential effects of multiple operations on the same paragraph. @@ -874,8 +920,8 @@ All LLM-facing errors inherit from `DocxEditError` and carry structured fields s | `DocumentClosedError` | `path` | The `Document` was used after `close()`. Reopen with `Document.open(e.path)`; edits not saved before the `close()` were discarded with the workspace. | | `DocumentNotFoundError`| `path` | The file doesn't exist at `path` — fix the path (typo, wrong cwd) before retrying. | | `InvalidDocumentError` | `path` | The file at `path` is not a valid .docx — wrong suffix, a directory, empty/truncated/not a ZIP, missing word/document.xml, or malformed XML. Not an in-loop retry: the message names which check failed; fix or re-export the input file. | -| `WorkspaceSyncError` | `workspace_path`, `source_path` | Workspace and source disagree (unsaved edits from a previous session, or the source changed on disk). **Do not retry blindly** — `force_recreate=True` (open) / `force=True` (save) DISCARDS one side; to rescue the workspace's edits first, save them elsewhere: `Workspace(source, create=False).save("rescued.docx")`. | -| `WorkspaceLockedError` | `pid`, `lock_path` | A live session already holds this document's workspace — another process, or an unclosed `Document` in THIS one. Close it (or stop that process) and retry; `Document.open(path, force_recreate=True)` takes the workspace over but DISCARDS the holder's unsaved edits. Locks left by dead processes are reclaimed automatically and never raise. | +| `WorkspaceSyncError` | `workspace_path`, `source_path` | Workspace and source disagree (unsaved edits from a previous session, or the source changed on disk). **Do not retry blindly** — `force_recreate=True` (open) / `force=True` (save) / `Document.discard_workspace(path)` DISCARDS one side; to rescue the workspace's edits first, save them elsewhere: `Workspace(source, create=False).save("rescued.docx")`. After a crashed script, `Document.discard_workspace(path)` once beats `force_recreate=True` on every open. | +| `WorkspaceLockedError` | `pid`, `lock_path` | A live session already holds this document's workspace — another process, or an unclosed `Document` in THIS one. Close it (or stop that process) and retry; `Document.open(path, force_recreate=True)` or `Document.discard_workspace(path)` takes the workspace over but DISCARDS the holder's unsaved edits — confirm the holder is gone first. Locks left by dead processes are reclaimed automatically and never raise. | | `DocumentOpenError` | `path`, `owner_file` | **Do not retry blindly.** The destination is open in Word. Stop and tell the user to close it. Only pass `force=True` if the user confirms the `~$` lock is stale (crashed session). | ```python @@ -898,8 +944,7 @@ except TextNotFoundError as e: except AmbiguousTextError as e: # Target matched e.total_occurrences times — enumerate and pick r = doc.find_all("stale text", paragraph=e.paragraph_ref)[0] - doc.replace("stale text", "new text", paragraph=r.paragraph_ref, - occurrence=r.paragraph_occurrence) + doc.replace(r, "new text") # the picked match pins ref + occurrence except ParagraphIndexError as e: # Clamp to a valid 1-indexed paragraph number (guard the empty-doc case) if e.total_paragraphs == 0: @@ -1071,7 +1116,8 @@ Rules: - **Always `docx-session stop` when the editing task is done** — don't leave kernels running. - Exit code 1 means the code raised: the traceback is on stderr, the session survives — fix the call and continue (introspect with `docx-session exec "import inspect; print(inspect.signature(doc.replace))"` when unsure). -- Exit code 2 means timeout — the kernel is alive and your code is still running. Exit code 3 means no session is running. +- Exit code 2 means timeout, in one of two flavours the stderr line names (and `eval`'s JSON envelope reports as `"started"`): **your code ran too long** (`started: true` — "kernel still running"; raise `--timeout` or make the code faster), or **it never left the queue** (`started: false` — "still queued … never started"; the kernel is busy with an *earlier* command, and nothing of yours executed, so re-sending later is safe). Exit code 3 means no session is running. +- **Exit code 0 is not always "it ran".** If a previous command *raised* while your request was queued behind it, the kernel discards yours without executing it. `exec` then exits **0** with no output and `Warning: the kernel discarded this request without running it` on stderr — so read stderr on exit 0 (or check `started` on an `ExecResult`) before believing an edit applied. `eval` exits **1** with `The kernel discarded this request without running it` on stderr and **no JSON envelope**. Either way nothing of yours happened: re-send it. - Exit code 4 means the kernel died mid-exec or is unreachable: its state (open documents, variables) is lost. Recover with `docx-session stop` then `start`, and re-open your documents. - `eval` output: library dataclasses (SearchResult, ParagraphInfo, ParagraphLocation, Revision, Comment) arrive as real JSON objects (`"serialized": true`, datetimes as ISO strings, tuples as lists) — access fields directly, never string-parse a repr. `"serialized": false` means the value wasn't JSON-serializable and `value` holds its `repr` string. On exit 1 the envelope carries `"error": {"type", "message", }` plus a compact, machine-path-stripped `traceback` (library frames appear as relative `docx_editor/...` paths and the eval line as ``; only absolute machine paths are stripped) (`"error"` is null for the rare raise that bypasses the kernel-side capture, e.g. SystemExit — fall back to `traceback`). On exit 4 the envelope has `"status": "dead"`; on exit 3 (no session) there is no envelope at all. - `eval` of an edit call returns the `EditResult` as a plain JSON *string* (it is a `str` subclass, so its value is just the new ref) — `group_id`/`revision_ids` are lost in transit. Keep the result in a kernel-side variable and eval a dict projection when you need them: `docx-session exec "r = doc.replace(...)"` then `docx-session eval "{'ref': str(r), 'group_id': r.group_id}"`. @@ -1080,11 +1126,11 @@ Rules: - **Never one `exec` per edit.** Each `docx-session` call is a fresh CLI round-trip (~250 ms of subprocess + IPC overhead) that dwarfs the edit itself — 50 edits sent as 50 `exec` calls take ~12 s, the same 50 batched into one `exec` is a single round-trip (~0.3 s: the ~250 ms overhead plus the in-kernel loop), ~40x less overhead. Loop inside a single `exec`, or use `batch_edit`. - **Project fields kernel-side for large reads.** `eval` serializes the whole value to JSON (~150 chars per `SearchResult`), so a big `find_all` returns tens of KB of context. Return only the fields you need — e.g. `docx-session eval "[(r.paragraph_ref, r.paragraph_occurrence) for r in doc.find_all('30 days')]"` — instead of the full objects (same idea as the `EditResult` projection above). - Use absolute paths inside `exec` — the kernel's cwd is whatever `start` captured. -- A `exec` sent while the kernel is still busy **queues** behind the running one; `--timeout` covers the whole wait. A timeout does not cancel the running code. +- A `exec` sent while the kernel is still busy **queues** behind the running one; `--timeout` covers the whole wait. A timeout does not cancel the running code. Tell the two timeouts apart with `started` / the stderr line (see exit code 2 above) before deciding whether re-sending is safe. - The session is non-interactive: `input()` (and anything reading stdin) raises `StdinNotImplementedError` rather than hanging. - `doc.save()` raises `WorkspaceSyncError` if the file changed on disk while the session held it open (e.g. the user edited it in Word). Ask the user before retrying with `doc.save(force=True)` — force overwrites their changes. -- A session that saved to a different path (or whose save failed) and never called `doc.close()` leaves the workspace flagged as holding unsaved changes; the next `Document.open()` of the same source raises `WorkspaceSyncError` instead of silently carrying those edits over. `Document.open(path, force_recreate=True)` recovers but **discards** those edits — to rescue them first, save the orphaned workspace to a new file: `from docx_editor.workspace import Workspace; Workspace("contract.docx", create=False).save("rescued.docx")` (deep import — `Workspace` is not exported at package root), then reopen the source with `force_recreate=True`. -- `Document.open()` raises `WorkspaceLockedError` if a live session (another process, or an unclosed `Document` in this one) already holds the document's workspace. Close the other session, or use `Document.open(path, force_recreate=True)` to take the workspace over, discarding its unsaved edits. Stale locks from dead processes are reclaimed automatically. +- A session that saved to a different path (or whose save failed) and never called `doc.close()` leaves the workspace flagged as holding unsaved changes; the next `Document.open()` of the same source raises `WorkspaceSyncError` instead of silently carrying those edits over. Two recoveries, both **discarding** those edits: `Document.open(path, force_recreate=True)` (per-open) or `Document.discard_workspace(path)` (once, then open normally — the fix for a crashed script, since it also clears a leftover lock and returns `False` when there was nothing to delete, so it is safe to call unconditionally). To rescue the edits first, save the orphaned workspace to a new file: `from docx_editor.workspace import Workspace; Workspace("contract.docx", create=False).save("rescued.docx")` (deep import — `Workspace` is not exported at package root), then discard. +- `Document.open()` raises `WorkspaceLockedError` if a live session (another process, or an unclosed `Document` in this one) already holds the document's workspace. Close the other session, or use `Document.open(path, force_recreate=True)` / `Document.discard_workspace(path)` to take the workspace over, discarding its unsaved edits — make sure the holder is really gone first. Stale locks from dead processes are reclaimed automatically. - Concurrent sessions via `--session-file` must edit *different* documents — a second session opening the same document raises `WorkspaceLockedError` (see "Editing in parallel" below). - For a single edit, a one-off script is still fine — session mode pays off with repeated operations. diff --git a/tests/conftest.py b/tests/conftest.py index d30fee9..f5168b2 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -18,6 +18,19 @@ def find_ref(doc, text): raise ValueError(f"Paragraph containing '{text}' not found") +def match_for(doc, text, **kwargs): + """``find_text`` narrowed to a definite match. + + ``find_text`` returns ``SearchResult | None``, which the edit methods refuse + (a None target must not become a silent no-op). Tests that know the text is + present use this so the Optional is handled once, here, instead of asserting + in every case. + """ + match = doc.find_text(text, **kwargs) + assert match is not None, f"no match for {text!r}" + return match + + def replace_docx_parts(src: Path, dest: Path, parts: dict[str, str | None]) -> None: """Copy ``src`` to ``dest``, swapping each archive part named in ``parts``. diff --git a/tests/test_batch_edit.py b/tests/test_batch_edit.py index 1005159..896b3ea 100644 --- a/tests/test_batch_edit.py +++ b/tests/test_batch_edit.py @@ -3,9 +3,10 @@ import shutil import tempfile from pathlib import Path +from typing import Any import pytest -from conftest import count_dom_walks +from conftest import count_dom_walks, match_for from docx_editor import ( AmbiguousTextError, @@ -570,6 +571,147 @@ def test_unknown_action(self, multi_para_doc): assert "Unknown action" in results[0].error +class TestDryRunCurrentRef: + """A stale-hash row carries ``current_ref``, so recovery needs no message + parsing (ISSUES.md #52). + """ + + @staticmethod + def _stale_row(doc): + """Edit P5, then dry-run an op that still carries P5's old ref.""" + stale_ref = doc.list_paragraphs()[4].split("|")[0] + doc.replace("item 5", "CHANGED", paragraph=stale_ref) + ops = [EditOperation.replace("CHANGED", "EDIT_5", paragraph=stale_ref)] + return doc.batch_edit(ops, dry_run=True)[0] + + def test_stale_hash_row_reports_the_current_ref(self, multi_para_doc): + doc, _ = multi_para_doc + row = self._stale_row(doc) + + assert row.valid is False + assert row.current_ref == doc.list_paragraphs()[4].split("|")[0] + assert row.current_ref != row.paragraph # the point: a different hash + + def test_current_ref_is_the_hash_from_the_error_message(self, multi_para_doc): + """Structured field and prose agree — consumers can stop regexing.""" + doc, _ = multi_para_doc + row = self._stale_row(doc) + + assert row.current_ref is not None + assert row.current_ref.split("#")[1] in row.error + + def test_retrying_with_current_ref_validates_and_applies(self, multi_para_doc): + doc, _ = multi_para_doc + row = self._stale_row(doc) + + repaired = [EditOperation.replace("CHANGED", "EDIT_5", paragraph=row.current_ref)] + assert all(r.valid for r in doc.batch_edit(repaired, dry_run=True)) + doc.batch_edit(repaired) + assert "EDIT_5" in doc.get_visible_text() + + def test_valid_rows_have_no_current_ref(self, multi_para_doc): + doc, _ = multi_para_doc + ref = doc.list_paragraphs()[2].split("|")[0] + rows = doc.batch_edit([EditOperation.replace("item 3", "x", paragraph=ref)], dry_run=True) + + assert rows[0].valid is True + assert rows[0].current_ref is None + + def test_other_failures_have_no_current_ref(self, multi_para_doc): + """Only a stale hash is mechanically fixable; nothing else pretends to be.""" + doc, _ = multi_para_doc + refs = doc.list_paragraphs() + count = doc.paragraph_count() + ops = [ + EditOperation.replace("NONEXISTENT", "x", paragraph=refs[0].split("|")[0]), # missing text + EditOperation.replace("committee", "x", paragraph=refs[1].split("|")[0]), # ambiguous (appears twice) + EditOperation(action="replace", find="a", replace_with="b", paragraph="P9999#0000"), # bad index + EditOperation(action="replace", find="a", replace_with="b", paragraph="not-a-ref"), # malformed + "not an EditOperation", + ] + rows = doc.batch_edit(ops, dry_run=True) + + assert count == doc.paragraph_count() + assert [r.valid for r in rows] == [False] * 5 + assert [r.current_ref for r in rows] == [None] * 5 + + +class TestEditOperationSearchResultTarget: + """EditOperation constructors accept a SearchResult, so a find_all() sweep + becomes a batch with no ref/occurrence bookkeeping (ISSUES.md #52). + """ + + def test_replace_from_a_match_matches_the_explicit_spelling(self, multi_para_doc): + doc, _ = multi_para_doc + match = match_for(doc, "item 3") + + assert EditOperation.replace(match, "ITEM_THREE") == EditOperation.replace( + match.text, + "ITEM_THREE", + paragraph=match.paragraph_ref, + occurrence=match.paragraph_occurrence, + ) + + def test_every_constructor_accepts_a_match(self, multi_para_doc): + doc, _ = multi_para_doc + match = match_for(doc, "item 4") + + built = [ + EditOperation.replace(match, "x"), + EditOperation.delete(match), + EditOperation.insert_after(match, " after"), + EditOperation.insert_before(match, "before "), + ] + assert [op.action for op in built] == ["replace", "delete", "insert_after", "insert_before"] + for op in built: + assert op.paragraph == match.paragraph_ref + assert op.occurrence == match.paragraph_occurrence + assert built[0].find == built[1].text == built[2].anchor == built[3].anchor == match.text + + def test_find_all_sweep_applies_as_one_batch(self, multi_para_doc): + """The papercut end to end: search results straight into batch_edit. + + reversed() keeps same-paragraph ops in descending occurrence order, as + the batch contract requires. + """ + doc, _ = multi_para_doc + matches = doc.find_all("item 7") + assert matches + + results = doc.batch_edit([EditOperation.replace(m, "SEVEN") for m in reversed(matches)]) + + assert len(results) == len(matches) + assert "item 7" not in doc.get_visible_text() + assert "SEVEN" in doc.get_visible_text() + + def test_paragraph_or_occurrence_with_a_match_is_refused(self, multi_para_doc): + doc, _ = multi_para_doc + match = match_for(doc, "item 3") + + # dict[str, Any]: the point is that ANY of these extra kwargs is refused. + extras: list[dict[str, Any]] = [{"paragraph": match.paragraph_ref}, {"occurrence": 0}] + for kwargs in extras: + with pytest.raises(ValueError, match="already pins the paragraph"): + EditOperation.replace(match, "x", **kwargs) + with pytest.raises(ValueError, match="already pins the paragraph"): + EditOperation.delete(match, **kwargs) + with pytest.raises(ValueError, match="already pins the paragraph"): + EditOperation.insert_after(match, "x", **kwargs) + with pytest.raises(ValueError, match="already pins the paragraph"): + EditOperation.insert_before(match, "x", **kwargs) + + def test_missing_paragraph_teaches_the_search_result_form(self): + """The long-standing message is kept; the hint is appended to it.""" + with pytest.raises(ValueError, match="Invalid paragraph reference None"): + EditOperation.replace("a", "b") + with pytest.raises(ValueError, match="or pass a SearchResult as 'find'"): + EditOperation.replace("a", "b") + with pytest.raises(ValueError, match="or pass a SearchResult as 'text'"): + EditOperation.delete("a") + with pytest.raises(ValueError, match="or pass a SearchResult as 'anchor'"): + EditOperation.insert_before("a", "x") + + class TestStructuredBatchOperationError: def test_predispatch_missing_paragraph_has_operation_index(self, multi_para_doc): doc, _ = multi_para_doc @@ -1053,11 +1195,11 @@ def test_empty_paragraph_ref_rejected(self): def test_none_paragraph_ref_rejected(self): """paragraph=None gets the field-specific ValueError, not a raw regex TypeError.""" with pytest.raises(ValueError, match="Invalid paragraph reference None"): - EditOperation.replace("a", "b", paragraph=None) # type: ignore[arg-type] + EditOperation.replace("a", "b", paragraph=None) def test_delete_none_paragraph_ref_rejected(self): with pytest.raises(ValueError, match="Invalid paragraph reference None"): - EditOperation.delete("x", paragraph=None) # type: ignore[arg-type] + EditOperation.delete("x", paragraph=None) def test_non_string_paragraph_ref_names_the_type(self): with pytest.raises(ValueError, match="expected a string like 'P3#a7b2', got int"): diff --git a/tests/test_comments.py b/tests/test_comments.py index 4500615..f893ed4 100644 --- a/tests/test_comments.py +++ b/tests/test_comments.py @@ -1,6 +1,9 @@ """Tests for comment functionality.""" +from typing import Any + import pytest +from conftest import match_for from docx_editor import ( CommentError, @@ -1337,3 +1340,78 @@ def test_scoped_occurrence_when_anchor_absent(self, clean_workspace): assert exc_info.value.total_occurrences is None finally: doc.close() + + +class TestAddCommentSearchResultAnchor: + """``add_comment`` accepts a SearchResult as its anchor, which pins the + paragraph and occurrence for it (ISSUES.md #52). + """ + + def test_comment_anchored_from_a_match(self, clean_workspace): + doc = Document.open(clean_workspace) + try: + match = match_for(doc, "brown fox") + comment_id = doc.add_comment(match, "from a search") + + comment = doc.list_comments()[0] + assert comment.id == comment_id + assert comment.text == "from a search" + finally: + doc.close() + + def test_matches_the_explicit_spelling(self, clean_workspace): + """Same markers as passing text/paragraph/occurrence by hand.""" + doc = Document.open(clean_workspace) + try: + match = match_for(doc, "lazy dog") + by_object = doc.add_comment(match, "note A") + by_fields = doc.add_comment( + match.text, + "note B", + paragraph=match.paragraph_ref, + occurrence=match.paragraph_occurrence, + ) + + starts = doc._document_editor.dom.getElementsByTagName("w:commentRangeStart") + ids = [s.getAttribute("w:id") for s in starts] + assert str(by_object) in ids and str(by_fields) in ids + finally: + doc.close() + + def test_second_occurrence_is_targeted_without_bookkeeping(self, clean_workspace): + """The 2nd "he" in P2 gets the comment, not the 1st.""" + doc = Document.open(clean_workspace) + try: + second = [m for m in doc.find_all("he") if m.paragraph_index == 2][1] + assert second.paragraph_occurrence == 1 + doc.add_comment(second, "second only") + + # Everything before the marker must already carry the sentence up to + # the SECOND "he" — anchoring to the first would cut after "T". + para = _find_paragraph_with_text(doc, "quick brown fox") + before_marker = para.toxml().split(" dict[str, ParagraphInfo]: + return {info.text: info for info in doc.list_paragraphs_structured(limit=None)} + + def test_heading_styles_and_outline_levels(self, styled_docx): + with Document.open(styled_docx) as doc: + infos = self._by_text(doc) + + assert (infos["Chapter one"].style, infos["Chapter one"].outline_level) == ("Heading1", 0) + assert (infos["Termination"].style, infos["Termination"].outline_level) == ("Heading2", 1) + + def test_plain_body_paragraph_has_neither(self, styled_docx): + with Document.open(styled_docx) as doc: + info = self._by_text(doc)["Preamble text"] + assert info.style is None + assert info.outline_level is None + assert info.in_table is False + + def test_direct_outline_level_without_a_style(self, styled_docx): + with Document.open(styled_docx) as doc: + info = self._by_text(doc)["Direct outline"] + assert info.style is None + assert info.outline_level == 3 + + def test_style_defined_level_resolves_through_styles_xml(self, styled_docx): + """Heading1/Heading2 carry no direct w:outlineLvl — the level comes from + word/styles.xml, so this pins that the style map is actually wired in.""" + with Document.open(styled_docx) as doc: + assert self._by_text(doc)["Chapter two"].outline_level == 0 + + def test_unknown_style_is_kept_raw_with_no_level(self, styled_docx): + with Document.open(styled_docx) as doc: + info = self._by_text(doc)["Unknown style"] + assert info.style == "NoSuchStyle" + assert info.outline_level is None + + def test_body_text_marker_reports_no_level(self, styled_docx): + """TOCHeading is basedOn Heading1 but sets w:val="9" (no outline).""" + with Document.open(styled_docx) as doc: + info = self._by_text(doc)["Toc heading"] + assert info.style == "TOCHeading" + assert info.outline_level is None + + def test_pprchange_decoy_is_ignored(self, styled_docx): + """A style/level that only exists inside a w:pPrChange revision record + must not be read as current formatting.""" + with Document.open(styled_docx) as doc: + info = self._by_text(doc)["Decoy paragraph"] + assert info.style is None + assert info.outline_level is None + + def test_table_cell_paragraph_reports_in_table(self, styled_docx): + with Document.open(styled_docx) as doc: + infos = self._by_text(doc) + assert infos["Cell under h2"].in_table is True + assert infos["Body under h2"].in_table is False + + def test_toc_from_one_call(self, styled_docx): + """The consumer recipe the fields exist for.""" + with Document.open(styled_docx) as doc: + toc = [ + (info.outline_level, info.text) + for info in doc.list_paragraphs_structured(limit=None) + if info.outline_level is not None + ] + assert toc == [(0, "Chapter one"), (1, "Termination"), (3, "Direct outline"), (0, "Chapter two")] + + def test_get_paragraph_agrees_with_the_listing(self, styled_docx): + """Both producers must emit identical records, fields included.""" + with Document.open(styled_docx) as doc: + listed = doc.list_paragraphs_structured(limit=None) + assert [doc.get_paragraph(info.index) for info in listed] == listed + + def test_context_records_carry_the_fields(self, styled_docx): + with Document.open(styled_docx) as doc: + ref = _ref_for_text(doc, "Body under h2") + window = doc.context(ref, window=2) + + by_text = {info.text: info for info in window} + assert by_text["Termination"].outline_level == 1 + assert by_text["Cell under h2"].in_table is True + + def test_fields_agree_with_get_paragraph_location(self, styled_docx): + """One semantics, two carriers: never let the pair drift.""" + with Document.open(styled_docx) as doc: + for info in doc.list_paragraphs_structured(limit=None): + loc = doc.get_paragraph_location(info.ref) + assert (info.style, info.outline_level, info.in_table) == ( + loc.style, + loc.outline_level, + loc.in_table, + ) + + def test_document_without_styles_part_degrades(self, simple_docx, tmp_path): + """No word/styles.xml: direct levels still resolve, style-defined ones + become None, and nothing raises.""" + dest = tmp_path / "nostyles.docx" + replace_docx_parts( + simple_docx, + dest, + {"word/document.xml": _build_styled_document_xml(), "word/styles.xml": None}, + ) + with Document.open(dest) as doc: + infos = self._by_text(doc) + assert infos["Chapter one"].style == "Heading1" + assert infos["Chapter one"].outline_level is None # no styles.xml to resolve it + assert infos["Direct outline"].outline_level == 3 # direct w:outlineLvl still wins diff --git a/tests/test_session.py b/tests/test_session.py index da4c558..70de8d6 100644 --- a/tests/test_session.py +++ b/tests/test_session.py @@ -105,10 +105,136 @@ def test_exec_timeout(tmp_path): try: res = exec_code("import time; time.sleep(30)", connection_file=conn, timeout=2.0) assert res.status == "timeout" + assert res.started is True # our own code ran and overran finally: stop_session(conn) +def test_exec_ok_reports_started(session_conn): + assert exec_code("1 + 1", connection_file=session_conn).started is True + + +def test_exec_error_reports_started(session_conn): + """An exception still means the code executed.""" + assert exec_code("1 / 0", connection_file=session_conn).started is True + + +def test_exec_timeout_while_queued_reports_not_started(tmp_path): + """A timeout waiting in the QUEUE is distinguishable from a timeout of your + own running code (ISSUES.md #52): nothing of ours executed. + + Own kernel, occupied by a fire-and-forget sleep sent on a separate client, + so our request cannot leave the queue before the clock runs out. + """ + from docx_editor.session import _client + + conn = tmp_path / "kernel.json" + start_session(conn) + hog = _client(conn) + try: + hog.execute("import time; time.sleep(30)") # never waited on + time.sleep(1.0) # let the kernel pick it up and go busy + + res = exec_code("1 + 1", connection_file=conn, timeout=2.0) + assert res.status == "timeout" + assert res.started is False # never left the queue + assert res.result is None + finally: + hog.stop_channels() + stop_session(conn) + + +def test_cli_distinguishes_the_two_timeouts(tmp_path, capsys): + """Each timeout flavour gets its own stderr advice; exit code stays 2.""" + from docx_editor.session import EXIT_TIMEOUT + + conn = tmp_path / "kernel.json" + start_session(conn) + try: + code = main(["exec", "import time; time.sleep(30)", "--session-file", str(conn), "--timeout", "2"]) + assert code == EXIT_TIMEOUT + assert "kernel still running" in capsys.readouterr().err + + code = main(["exec", "1 + 1", "--session-file", str(conn), "--timeout", "2"]) + assert code == EXIT_TIMEOUT + err = capsys.readouterr().err + assert "still queued" in err + assert "never started" in err + finally: + stop_session(conn) + + +def test_request_discarded_behind_a_raising_command(tmp_path): + """An "ok" result with started=False ran NOTHING. + + ipykernel aborts everything still queued behind a command that raises, so a + request sent while an earlier one was failing comes back clean and empty + having done nothing. Without ``started`` that is indistinguishable from a + successful silent statement — the caller would believe its edit applied. + """ + from docx_editor.session import _client + + conn = tmp_path / "kernel.json" + start_session(conn) + hog = _client(conn) + try: + hog.execute("import time; time.sleep(3); raise RuntimeError('boom')") + time.sleep(0.5) # let the kernel pick it up and go busy + + res = exec_code("discarded = 'I RAN'", connection_file=conn, timeout=30) + assert res.status == "ok" + assert res.started is False # the tell: never dequeued + assert res.stdout == "" + + time.sleep(4) # let the failing command finish and the kernel settle + assert eval_code("globals().get('discarded')", connection_file=conn).value is None + finally: + hog.stop_channels() + stop_session(conn) + + +def test_cli_warns_when_the_kernel_discards_the_request(tmp_path, capsys): + """Exit code stays 0 (contract), so the warning has to carry the signal.""" + from docx_editor.session import EXIT_OK, _client + + conn = tmp_path / "kernel.json" + start_session(conn) + hog = _client(conn) + try: + hog.execute("import time; time.sleep(3); raise RuntimeError('boom')") + time.sleep(0.5) + + code = main(["exec", "discarded = 1", "--session-file", str(conn), "--timeout", "30"]) + assert code == EXIT_OK + assert "discarded this request" in capsys.readouterr().err + finally: + hog.stop_channels() + stop_session(conn) + + +def test_eval_names_the_discard_instead_of_blaming_the_transport(tmp_path, capsys): + """A discarded eval has no reply to decode — but that is not a transport + fault, and saying so sends the caller debugging the wrong thing.""" + from docx_editor.session import EXIT_ERROR, _client + + conn = tmp_path / "kernel.json" + start_session(conn) + hog = _client(conn) + try: + hog.execute("import time; time.sleep(3); raise RuntimeError('boom')") + time.sleep(0.5) + + code = main(["eval", "1 + 1", "--session-file", str(conn), "--timeout", "30"]) + captured = capsys.readouterr() + assert code == EXIT_ERROR + assert "discarded this request" in captured.err + assert "transport failed" not in captured.err + assert captured.out == "" # no envelope: nothing was evaluated + finally: + hog.stop_channels() + stop_session(conn) + + def test_exec_docx_editing_workflow(session_conn, temp_docx): """End-to-end: a document stays open across separate exec calls.""" r1 = exec_code( @@ -198,10 +324,18 @@ def test_main_eval_without_session(self, tmp_path, capsys): assert "docx-session start" in captured.err def test_eval_transport_no_result_raises(self, tmp_path, monkeypatch): - """A kernel reply with no execute_result is a transport bug, not a user error.""" + """A kernel reply with no execute_result is a transport bug, not a user error. + + ``started=True`` is what makes it one: the wrapper ran and still produced + nothing. The same shape with ``started=False`` means the kernel discarded + the request, which gets its own message (see + test_eval_names_the_discard_instead_of_blaming_the_transport). + """ from docx_editor import session as session_mod - monkeypatch.setattr(session_mod, "exec_code", lambda *a, **k: ExecResult(status="ok", result=None)) + monkeypatch.setattr( + session_mod, "exec_code", lambda *a, **k: ExecResult(status="ok", result=None, started=True) + ) with pytest.raises(SessionError, match="returned no result"): eval_code("1 + 1", connection_file=tmp_path / "unused.json") diff --git a/tests/test_track_changes.py b/tests/test_track_changes.py index 29b4ecf..5b62e7a 100644 --- a/tests/test_track_changes.py +++ b/tests/test_track_changes.py @@ -1,12 +1,14 @@ """Tests for track changes functionality.""" +import shutil from datetime import datetime, timezone +from typing import Any from unittest.mock import MagicMock import pytest -from conftest import find_ref +from conftest import find_ref, match_for -from docx_editor import Document, TextNotFoundError +from docx_editor import Document, HashMismatchError, TextNotFoundError from docx_editor.track_changes import Revision, RevisionManager, _escape_xml, _trim_replace_affixes from docx_editor.xml_editor import DocxXMLEditor, build_text_map @@ -1609,3 +1611,254 @@ def test_unicode_smart_quote_tokens(self): replace_with = "“term” of y" prefix, suffix = _trim_replace_affixes(find, replace_with) assert (prefix, suffix) == (len("“term” of "), 0) + + +class TestTrimReplaceAffixesWhitespaceOnly: + """Whitespace-only spans trim character-wise, so a spacing fix is a pure + insert/delete instead of an invisible del+ins pair (ISSUES.md #60). + + Every case asserts the resulting (del, ins) split rather than raw offsets: + that is what decides which revisions get written. + """ + + @staticmethod + def _split(find: str, replace_with: str) -> tuple[str, str]: + prefix, suffix = _trim_replace_affixes(find, replace_with) + # An affix pair that overlaps would silently corrupt the split. + assert prefix + suffix <= min(len(find), len(replace_with)) + return find[prefix : len(find) - suffix], replace_with[prefix : len(replace_with) - suffix] + + def test_single_to_double_space_is_pure_insert(self): + assert self._split(" ", " ") == ("", " ") + + def test_double_to_single_space_is_pure_delete(self): + assert self._split(" ", " ") == (" ", "") + + def test_three_to_one_space_deletes_two(self): + assert self._split(" ", " ") == (" ", "") + + def test_space_to_tab_stays_a_replacement(self): + """Different whitespace characters share no affix — still del+ins.""" + assert self._split(" ", "\t") == (" ", "\t") + + def test_trim_applies_after_word_level_trim(self): + """Words trim first; only the leftover whitespace trims by character.""" + assert self._split("a b", "a b") == ("", " ") + assert self._split("thirty days", "thirty days") == ("", " ") + + def test_shared_characters_never_consumed_twice(self): + """Prefix and suffix both grow, bounded by the shorter side.""" + assert self._split("\t \t", "\t\t") == (" ", "") + + def test_mixed_whitespace_and_words_is_left_alone(self): + """A span that is not whitespace-only keeps whole-span replacement.""" + assert self._split(" x ", " x ") == (" x ", " x ") + + def test_word_replacement_unaffected(self): + """Regression canary: character trimming must never reach word spans.""" + assert self._split("30 days", "60 days") == ("30", "60") + assert self._split("net", "gross") == ("net", "gross") + + +class TestWhitespaceReplaceRevisions: + """End-to-end: the whitespace-only trim reaches the written revisions.""" + + def test_single_to_double_space_writes_only_an_insertion(self, clean_workspace): + doc = Document.open(clean_workspace) + ref = find_ref(doc, "quick brown fox") + doc.replace("quick brown", "quick brown", paragraph=ref) + + kinds = [r.type for r in doc.list_revisions()] + assert kinds == ["insertion"], kinds + assert doc.get_visible_text().splitlines()[1] == "The quick brown fox jumps over the lazy dog." + doc.close() + + def test_double_to_single_space_writes_only_a_deletion(self, clean_workspace): + doc = Document.open(clean_workspace) + ref = find_ref(doc, "quick brown fox") + doc.replace("quick brown", "quick brown", paragraph=ref) + doc.accept_all() + + ref = find_ref(doc, "quick brown fox") + doc.replace("quick brown", "quick brown", paragraph=ref) + kinds = [r.type for r in doc.list_revisions()] + assert kinds == ["deletion"], kinds + assert doc.get_visible_text().splitlines()[1] == "The quick brown fox jumps over the lazy dog." + doc.close() + + def test_word_replace_still_writes_both_halves(self, clean_workspace): + """The canary at the revision level: words stay a deletion+insertion.""" + doc = Document.open(clean_workspace) + ref = find_ref(doc, "quick brown fox") + doc.replace("quick", "swift", paragraph=ref) + + assert sorted(r.type for r in doc.list_revisions()) == ["deletion", "insertion"] + doc.close() + + def test_whitespace_deletion_is_preserved_for_reject(self, clean_workspace): + """The deleted whitespace must survive a round-trip through a conforming + consumer, or rejecting the fix would silently restore the WRONG spacing. + + A whitespace-only ``w:delText`` without ``xml:space="preserve"`` gets its + content trimmed away by Word, and ``_restore_deletion`` copies that + element's attributes onto the ``w:t`` it rebuilds — so both halves of the + undo path need the attribute. + """ + doc = Document.open(clean_workspace) + ref = find_ref(doc, "quick brown fox") + doc.replace("quick brown", "quick brown", paragraph=ref) + doc.accept_all() + + ref = find_ref(doc, "quick brown fox") + result = doc.replace("quick brown", "quick brown", paragraph=ref) + deleted = doc._document_editor.dom.getElementsByTagName("w:delText") + assert [d.firstChild.data for d in deleted] == [" "] + assert deleted[0].getAttribute("xml:space") == "preserve" + + assert result.group_id is not None # a real edit, so a real group to reject + doc.reject_group(result.group_id) + restored = [t for t in doc._document_editor.dom.getElementsByTagName("w:t") if t.firstChild.data == " "] + assert restored, "the deleted space should be back as a w:t" + assert all(t.getAttribute("xml:space") == "preserve" for t in restored) + assert doc.get_visible_text().splitlines()[1] == "The quick brown fox jumps over the lazy dog." + doc.close() + + def test_deleted_edge_whitespace_carries_preserve(self, clean_workspace): + """Same rule for any deletion whose text has a whitespace edge, not just + the whitespace-only ones the trim newly produces.""" + doc = Document.open(clean_workspace) + ref = find_ref(doc, "quick brown fox") + doc.delete("brown ", paragraph=ref) + + deleted = doc._document_editor.dom.getElementsByTagName("w:delText") + assert [d.firstChild.data for d in deleted] == ["brown "] + assert deleted[0].getAttribute("xml:space") == "preserve" + doc.close() + + +class TestSearchResultAsEditTarget: + """A SearchResult can stand in for (text, paragraph, occurrence) on every + edit method — the find→edit double-typing papercut (ISSUES.md #52). + """ + + def test_replace_matches_the_explicit_spelling(self, clean_workspace): + doc = Document.open(clean_workspace) + match = match_for(doc, "quick") + result = doc.replace(match, "swift") + + assert result == match_for(doc, "swift").paragraph_ref + assert doc.get_visible_text().splitlines()[1] == "The swift brown fox jumps over the lazy dog." + doc.close() + + def test_delete_from_a_match(self, clean_workspace): + doc = Document.open(clean_workspace) + doc.delete(match_for(doc, "brown ")) + + assert doc.get_visible_text().splitlines()[1] == "The quick fox jumps over the lazy dog." + doc.close() + + def test_insert_after_and_before_from_a_match(self, clean_workspace): + doc = Document.open(clean_workspace) + doc.insert_after(match_for(doc, "lazy dog"), " (allegedly)") + doc.insert_before(match_for(doc, "The quick"), "Note: ") + + # The anchor is "lazy dog", so the insertion lands before the period. + assert doc.get_visible_text().splitlines()[1] == ( + "Note: The quick brown fox jumps over the lazy dog (allegedly)." + ) + doc.close() + + def test_occurrence_is_carried_from_the_match(self, clean_workspace): + """The 2nd "he" in P2 ("tHE lazy") is edited with no occurrence bookkeeping.""" + doc = Document.open(clean_workspace) + second = [m for m in doc.find_all("he") if m.paragraph_index == 2][1] + assert second.paragraph_occurrence == 1 + + doc.replace(second, "HE") + assert doc.get_visible_text().splitlines()[1] == "The quick brown fox jumps over tHE lazy dog." + doc.close() + + def test_equivalent_to_spelling_the_fields_out(self, clean_workspace, temp_dir): + """Same edit, two spellings, byte-identical document XML.""" + second_copy = temp_dir / "second.docx" + shutil.copy(clean_workspace, second_copy) + + doc = Document.open(clean_workspace) + match = match_for(doc, "sample document") + doc.replace(match, "example document") + via_object = doc.get_markup_text() + doc.close() + + other = Document.open(second_copy) + m2 = match_for(other, "sample document") + other.replace( + m2.text, + "example document", + paragraph=m2.paragraph_ref, + occurrence=m2.paragraph_occurrence, + ) + via_fields = other.get_markup_text() + other.close() + + assert via_object == via_fields + + def test_stale_match_raises_hash_mismatch(self, clean_workspace): + """A SearchResult is a ref like any other: it goes stale when edited.""" + doc = Document.open(clean_workspace) + match = match_for(doc, "quick") + doc.replace("brown", "red", paragraph=match.paragraph_ref) + + with pytest.raises(HashMismatchError): + doc.replace(match, "swift") + doc.close() + + def test_paragraph_and_occurrence_are_refused_with_a_match(self, clean_workspace): + doc = Document.open(clean_workspace) + match = match_for(doc, "quick") + + extras: list[dict[str, Any]] = [{"paragraph": match.paragraph_ref}, {"occurrence": 0}, {"occurrence": 3}] + for kwargs in extras: + with pytest.raises(ValueError, match="already pins the paragraph"): + doc.replace(match, "swift", **kwargs) + with pytest.raises(ValueError, match="drop paragraph= and occurrence="): + doc.replace(match, "swift", paragraph=match.paragraph_ref, occurrence=0) + assert doc.list_revisions() == [] + doc.close() + + def test_every_method_refuses_the_double_spelling(self, clean_workspace): + doc = Document.open(clean_workspace) + match = match_for(doc, "quick") + calls = [ + lambda: doc.replace(match, "x", occurrence=0), + lambda: doc.delete(match, occurrence=0), + lambda: doc.insert_after(match, "x", occurrence=0), + lambda: doc.insert_before(match, "x", occurrence=0), + lambda: doc.add_comment(match, "note", occurrence=0), + ] + for call in calls: + with pytest.raises(ValueError, match="already pins the paragraph"): + call() + doc.close() + + def test_plain_text_still_requires_a_paragraph(self, clean_workspace): + """The doc-wide RevisionManager branch stays unreachable, and the error + teaches the SearchResult form.""" + doc = Document.open(clean_workspace) + with pytest.raises(ValueError, match="must be a paragraph ref string"): + doc.replace("quick", "swift") + with pytest.raises(ValueError, match="or pass a SearchResult as 'find'"): + doc.replace("quick", "swift") + with pytest.raises(ValueError, match="or pass a SearchResult as 'text'"): + doc.delete("quick") + with pytest.raises(ValueError, match="or pass a SearchResult as 'anchor'"): + doc.insert_after("quick", "x") + assert doc.list_revisions() == [] + doc.close() + + def test_find_text_returning_none_is_not_a_silent_edit(self, clean_workspace): + """Passing an unchecked find_text() result through is a clear error.""" + doc = Document.open(clean_workspace) + assert doc.find_text("no such text") is None + with pytest.raises(ValueError, match="must be a paragraph ref string"): + doc.replace(doc.find_text("no such text"), "x") # type: ignore[arg-type] + doc.close() diff --git a/tests/test_workspace.py b/tests/test_workspace.py index dab442e..2a2abed 100644 --- a/tests/test_workspace.py +++ b/tests/test_workspace.py @@ -950,6 +950,74 @@ def test_pre_upgrade_meta_without_dirty_key_adopts(self, temp_docx): ws2.close() +class TestDiscardWorkspace: + """``Document.discard_workspace()`` — the crashed-script reset that used to + need a deep ``Workspace`` import or a hand-deleted cache dir (ISSUES.md #60). + """ + + def test_returns_true_then_false(self, temp_docx): + Document.open(temp_docx).close(cleanup=False) + + assert Document.discard_workspace(temp_docx) is True + assert Document.discard_workspace(temp_docx) is False # idempotent + + def test_no_workspace_at_all_is_not_an_error(self, temp_docx): + """Safe to call unconditionally at the top of a script.""" + assert Document.discard_workspace(temp_docx) is False + + def test_unblocks_open_after_a_crashed_session(self, temp_docx, temp_dir): + """The friction being removed: a session that saved elsewhere and never + closed makes every later open() raise until the workspace is dealt with.""" + doc = Document.open(temp_docx) + ref = find_ref(doc, "fox") + doc.replace("fox", "cat", paragraph=ref) + doc.save(temp_dir / "elsewhere.docx") + doc.close(cleanup=False) # stands in for a crash: workspace left dirty + + with pytest.raises(WorkspaceSyncError): + Document.open(temp_docx) + + assert Document.discard_workspace(temp_docx) is True + reopened = Document.open(temp_docx) # no force_recreate needed + try: + assert "fox" in reopened.get_visible_text() # fresh from the source + finally: + reopened.close() + + def test_clears_a_leftover_lock(self, temp_docx): + """A lock from a wedged session must not keep the document unopenable.""" + doc = Document.open(temp_docx) + lock_path = doc.workspace_path.with_name(doc.workspace_path.name + ".lock") + assert lock_path.exists() + + with pytest.raises(WorkspaceLockedError): + Document.open(temp_docx) + + assert Document.discard_workspace(temp_docx) is True + assert not lock_path.exists() + assert not doc.workspace_path.exists() + + replacement = Document.open(temp_docx) + replacement.close() + + def test_honours_workspace_dir(self, temp_docx, temp_dir): + """Targets the same workspace ``open(workspace_dir=...)`` would.""" + override = temp_dir / "ws_override" + Document.open(temp_docx, workspace_dir=override).close(cleanup=False) + + # The default location holds nothing, so a bare call finds nothing. + assert Document.discard_workspace(temp_docx) is False + assert Document.discard_workspace(temp_docx, workspace_dir=override) is True + assert Document.discard_workspace(temp_docx, workspace_dir=override) is False + + def test_source_need_not_exist(self, temp_docx): + """A workspace outlives its source, so cleanup after a deleted file works.""" + Document.open(temp_docx).close(cleanup=False) + temp_docx.unlink() + + assert Document.discard_workspace(temp_docx) is True + + class TestWorkspaceLock: """Advisory per-workspace lock against concurrent opens (issue #24)."""