diff --git a/.github/workflows/redline-action-test.yml b/.github/workflows/redline-action-test.yml index 0e41fe3..1f3efd3 100644 --- a/.github/workflows/redline-action-test.yml +++ b/.github/workflows/redline-action-test.yml @@ -44,7 +44,11 @@ jobs: import json, sys [record] = json.load(sys.stdin) assert record["status"] == "explicit", record - assert record["revisions"] == 9, record + # Not an exact count: this workflow installs python-redlines from PyPI, + # so the number tracks whatever version is published, not this branch. + # The exact count is pinned in tests/test_docxodus_engine.py, which runs + # against the working tree. + assert isinstance(record["revisions"], int) and record["revisions"] > 0, record ' test -s "redlines/tests/fixtures/modified.redline.docx" @@ -87,6 +91,10 @@ jobs: [record] = json.load(sys.stdin) assert record["path"] == "contracts/agreement.docx", record assert record["status"] == "modified", record - assert record["revisions"] == 9, record + # Not an exact count: this workflow installs python-redlines from PyPI, + # so the number tracks whatever version is published, not this branch. + # The exact count is pinned in tests/test_docxodus_engine.py, which runs + # against the working tree. + assert isinstance(record["revisions"], int) and record["revisions"] > 0, record ' test -s "redlines/contracts/agreement.redline.docx" diff --git a/CLAUDE.md b/CLAUDE.md index c0ce19a..d6f678e 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -7,9 +7,10 @@ This file provides guidance to Claude Code (claude.ai/code) when working with co Python-Redlines generates `.docx` redline/tracked-changes documents by comparing two Word files. A pure-Python wrapper drives compiled C# (.NET 10) engine binaries; the Python layer handles platform detection, binary extraction, temp file management, and subprocess execution. Two comparison engines are available: -- **XmlPowerToolsEngine** — wraps Open-XML-PowerTools WmlComparer (original engine) +- **XmlPowerToolsEngine** — wraps Open-XML-PowerTools WmlComparer (original engine). + Deprecated as of 1.0.0: instantiating it emits a `DeprecationWarning`. Still published. - **DocxodusEngine** — wraps Docxodus, a modernized .NET 10.0 fork with better move detection. - Takes `engine="wmlcomparer"` (default) or `engine="docxdiff"` to pick the comparison algorithm. + Runs `DocxDiff`, its only algorithm — Docxodus v11.0.0 deleted `WmlComparer`. ## Monorepo structure — three published packages @@ -78,10 +79,18 @@ python -m build --wheel packages/docxodus # needs an archive in _binaries/ package is missing, with the `pip install` command to fix it. Both engines expose `run_redline(author_tag, original, modified, **kwargs)`. - `DocxodusEngine` overrides `_build_command()` to translate kwargs (e.g. `engine`, - `detect_moves`, `detail_threshold`) into CLI flags, and raises `ValueError` when a - WmlComparer-only kwarg is combined with `engine="docxdiff"`. `XmlPowerToolsEngine` uses - the legacy 4-positional-arg format and ignores kwargs. + `DocxodusEngine` overrides `_build_command()` to translate kwargs (e.g. `detect_moves`, + `case_insensitive`) into CLI flags. It raises `ValueError` for unknown kwargs and for the + three removed with WmlComparer in Docxodus v11.0.0 — `engine`, `detail_threshold`, + `simplify_move_markup` — rather than dropping them: the CLI warn-and-ignores two of them, + so passing them through would report a setting that did nothing, and silently dropping + `engine="wmlcomparer"` would return DocxDiff output to a caller who asked for something + else. `XmlPowerToolsEngine` uses the legacy 4-positional-arg format and ignores kwargs. + + `BaseEngine.run_redline` registers **only files it created** for cleanup. Adding a + caller-supplied path there deletes the user's own document (fixed in 1.0.0), and the + output scratch file uses `mkstemp`, not `NamedTemporaryFile(delete=False).name`, which + leaked an unclosed file object (issue #30). 2. **Binary packages** ship one platform archive under `src//_binaries/.tar.gz` (or `.zip` for Windows). The archive is @@ -119,10 +128,15 @@ so all three always share one version. Bump only that file. Tests live in repo-root `tests/` and must be run from the repo root (fixtures use relative paths like `tests/fixtures/original.docx`). They require all three packages installed and the binaries built for the current platform. The XmlPowerToolsEngine -integration test validates exactly 9 revisions on the fixture documents. +integration test validates exactly 9 revisions on the fixture documents; the +DocxodusEngine one validates 10. ## Stdout Format Differences - **XmlPowerToolsEngine**: `"Revisions found: 9"` -- **DocxodusEngine**, default / `engine="wmlcomparer"`: `"Redline complete: 9 revision(s) found"` -- **DocxodusEngine**, `engine="docxdiff"`: `"Redline complete: 11 revision(s) found"` +- **DocxodusEngine**: `"Redline complete: 10 revision(s) found"` + +The Docxodus count is measured, not derived — re-measure it after any submodule bump and +re-pin `EXPECTED_REVISIONS` in `tests/test_docxodus_engine.py`. Clear +`~/.cache/python-redlines` first: the extraction cache keys on the installed binary +*package* version, so a rebuilt binary at an unchanged version is otherwise ignored. diff --git a/README.md b/README.md index 5d7bf70..4e48886 100644 --- a/README.md +++ b/README.md @@ -45,31 +45,29 @@ with open("redline.docx", "wb") as f: That's the whole thing. The rest of this README covers the other engines, comparison settings, and how the packages are built and distributed. -### 🆕 New in 0.3.0: `docxdiff`, an optional next-generation engine +### ⚠️ Breaking in 1.0.0: `WmlComparer` is gone; DocxDiff is the only algorithm -Docxodus now ships a **second comparison algorithm** alongside the classic one. `docxdiff` is a -structure-aware engine that models the document as a tree rather than a stream of runs, so it -produces finer-grained redlines and tracks structural edits — table cell and row properties, section -properties, header and footer content — that the classic algorithm reports coarsely or not at all. +Docxodus v11.0.0 **removed `WmlComparer`**, the algorithm this library ran by default for its +entire life. `DocxDiff` — introduced as an opt-in in 0.3.0, upstream's default since v8.0.0, and +now mature — replaces it. There is no flag to bring the old engine back: it no longer exists in +the binary. -**It is off by default, so nothing about your existing calls changes engines.** Opting in is one -keyword argument: +If you never passed comparison settings, **your code needs no change**. Output will differ, because +the algorithm differs; see [Upgrading to 1.0.0](#upgrading-to-100). + +Three keyword arguments were removed and now raise `ValueError`: ```python -engine.run_redline("Reviewer", original, modified, engine="docxdiff") +engine.run_redline("Reviewer", original, modified, engine="docxdiff") # ValueError +engine.run_redline("Reviewer", original, modified, detail_threshold=0.3) # ValueError +engine.run_redline("Reviewer", original, modified, simplify_move_markup=True) # ValueError ``` -**Please try it and tell us what you find.** It is new, and the two algorithms legitimately disagree -about how to describe the same edit — on this project's own test fixtures `docxdiff` reports 11 -revisions where the classic engine reports 9. Neither is wrong; they segment the same changes -differently. Before adopting it for production redlines, compare its output against your own -documents. See [Choosing an engine](#choosing-an-engine) for the trade-offs and the settings it does -not support. - -> **Note:** the default algorithm is unchanged, but the Docxodus binary behind it moved from v5.4.2 -> to v7.0.0 in this release and carries upstream `WmlComparer` fixes of its own (header references, -> table anchoring). Redline output on the default path can therefore differ from 0.2.1 independently -> of this new flag. Diff a representative document if byte-level stability matters to you. +They raise rather than being ignored on purpose. Two of them are still *accepted* by the +underlying CLI, which warns and does nothing; `--engine` is rejected outright. Silently dropping +`engine="wmlcomparer"` would have handed you DocxDiff output while you believed you had selected +something else — a wrong answer, not a breaking change. Unknown keyword arguments are rejected on +the same reasoning: a typo used to vanish in silence. ## GitHub Action @@ -123,13 +121,13 @@ You can also compare an explicit pair of files instead of auto-detecting: | `base-ref` / `head-ref` | event-derived | Commits to compare. Defaults: PR base (merge-base) → head on `pull_request`, `before` → `after` on `push`, else `HEAD~1` → `HEAD`. | | `author` | `python-redlines` | Author recorded on the tracked changes. | | `engine` | `docxodus` | `docxodus` or `xmlpowertools`. | -| `comparison` | engine default | `wmlcomparer` or `docxdiff` (docxodus engine only). | +| `comparison` | — | **Removed in 1.0.0.** Setting it fails the run; there is no longer an algorithm to select. | | `detect-moves` | `false` | Move detection (docxodus engine only). | | `output-dir` | `redlines` | Where outputs are written (mirrors the source tree). | | `html-preview` | `auto` | `auto` (render when the Docx2Html tool supports `--track-changes`, else warn and skip), `true` (require), `false` (skip — no .NET needed). | | `summary` | `true` | Write the job-summary table. | | `upload-artifact` / `artifact-name` | `true` / `docx-redlines` | Artifact upload controls. | -| `package-version` | latest | pip pin for python-redlines, e.g. `==0.3.0`. | +| `package-version` | latest | pip pin for python-redlines, e.g. `==1.0.0`. | | `docx2html-version` | latest | NuGet pin for the Docx2Html preview tool. | Outputs: `count` (redlines generated), `any-changes`, `redlines` (a JSON array of @@ -147,32 +145,31 @@ Notes: ## Comparison Engines -Python-Redlines gives you **three ways to compare**, across two engine classes. `DocxodusEngine` -carries two interchangeable algorithms in one binary; `XmlPowerToolsEngine` is a separate, legacy -package. +Python-Redlines ships **two engine classes**, each wrapping one algorithm. | # | Choice | How to select it | Algorithm | Status | |---|---|---|---|---| -| 1 | **Docxodus · `wmlcomparer`** | `DocxodusEngine()` | Modernized `WmlComparer` | ✅ **Default.** Stable, recommended | -| 2 | **Docxodus · `docxdiff`** | `DocxodusEngine()` + `engine="docxdiff"` | Structure-aware IR diff | 🆕 New in 0.3.0. Opt-in, seeking feedback | -| 3 | **Open-XML-PowerTools** | `XmlPowerToolsEngine()` | Original `WmlComparer` | 🗄️ Legacy. Upstream archived | - -Choices 1 and 3 are cousins: both descend from Microsoft's `WmlComparer`, which is why choice 1 is -named `wmlcomparer`. Choice 1 is Docxodus's actively-maintained fork of it; choice 3 is the original, -unmaintained code. Choice 2 shares nothing with either but the output format — it is a new engine. +| 1 | **Docxodus** | `DocxodusEngine()` | `DocxDiff` — structure-aware IR diff | ✅ **Default.** Actively maintained | +| 2 | **Open-XML-PowerTools** | `XmlPowerToolsEngine()` | Original `WmlComparer` | ⚠️ Deprecated. Upstream archived | **If you are unsure, use choice 1.** It is the default and requires no arguments. -### 1. `DocxodusEngine` with `wmlcomparer` — the default +Until 1.0.0 there was a third choice — Docxodus running a modernized `WmlComparer`, selected with +`engine="wmlcomparer"`. Docxodus v11.0.0 deleted it, so that choice is gone and the keyword +argument raises `ValueError`. + +### 1. `DocxodusEngine` — the default -**[Docxodus](https://github.com/JSv4/Docxodus)** is a modernized .NET 10.0 fork of Open-XML-PowerTools with -significant improvements: +**[Docxodus](https://github.com/JSv4/Docxodus)** is an actively-maintained .NET 10.0 document +toolchain. Its `DocxDiff` algorithm models the document as an intermediate representation rather +than a stream of runs, which lets it attribute a change to the exact paragraph, cell, row, or +section it touched: -- **Move detection** — identifies content that was moved rather than deleted and re-inserted +- **Structure-aware** — tracks table cell and row properties, section properties, and header and + footer content that a run-stream diff reports coarsely or not at all +- **Native move detection** — identifies content that moved rather than deleting and re-inserting it - **Format change detection** — detects changes to bold, italic, font size, and other run properties -- **Better table handling** — LCS-based row matching for large tables - **Actively maintained** — regular bug fixes and new features -- **Open XML SDK 3.x compatible** — uses the latest SDK version ```python from python_redlines import DocxodusEngine @@ -181,40 +178,24 @@ engine = DocxodusEngine() redline_bytes, stdout, stderr = engine.run_redline("AuthorName", original_bytes, modified_bytes) ``` -### 2. `DocxodusEngine` with `docxdiff` — new, opt-in +### 2. `XmlPowerToolsEngine` — deprecated -The same class and the same binary, selected per call. `docxdiff` models the document as an -intermediate representation, which lets it attribute a change to the exact paragraph, cell, row, or -section it touched. - -```python -from python_redlines import DocxodusEngine - -engine = DocxodusEngine() -redline_bytes, stdout, stderr = engine.run_redline( - "AuthorName", original_bytes, modified_bytes, engine="docxdiff", -) -``` - -Three settings do not exist for this algorithm and raise `ValueError` rather than being silently -ignored — see [Choosing an engine](#choosing-an-engine). - -### 3. `XmlPowerToolsEngine` — legacy - -Wraps the original [Open-XML-PowerTools](https://github.com/OpenXmlDev/Open-Xml-PowerTools) `WmlComparer`. This -engine remains available for backward compatibility and for users who prefer the original comparison behavior. +Wraps the original [Open-XML-PowerTools](https://github.com/OpenXmlDev/Open-Xml-PowerTools) +`WmlComparer`. Instantiating it emits a `DeprecationWarning`. ```python from python_redlines import XmlPowerToolsEngine -engine = XmlPowerToolsEngine() +engine = XmlPowerToolsEngine() # DeprecationWarning redline_bytes, stdout, stderr = engine.run_redline("AuthorName", original_bytes, modified_bytes) ``` -> **Note:** Open-XML-PowerTools was archived by Microsoft and is no longer maintained. It uses an older -> version of the Open XML SDK. While it works for many purposes, Docxodus is the recommended engine going forward. +> **Note:** Open-XML-PowerTools was archived by Microsoft and is no longer maintained. This class +> and its `python-redlines-ooxmlpowertools` wheel still ship, and still work, so that anyone who +> needs the original algorithm's output has somewhere to stand. It will be removed in a future +> major release — move to `DocxodusEngine` when you can. -All three share the same call signature — `run_redline(author, original, modified)` returning +Both share the same call signature — `run_redline(author, original, modified)` returning `(bytes, stdout, stderr)`. They differ in the class you instantiate, which keyword arguments they accept, and their stdout format (see [Stdout Differences](#stdout-differences) below). @@ -230,8 +211,8 @@ Each engine ships in its own optional companion package. Install the engine(s) y as extras: ```commandline -pip install python-redlines[docxodus] # Docxodus engine -pip install python-redlines[ooxmlpowertools] # Open-XML-PowerTools engine +pip install python-redlines[docxodus] # Docxodus engine (recommended) +pip install python-redlines[ooxmlpowertools] # Open-XML-PowerTools engine (deprecated) pip install python-redlines[all] # both engine packages ``` @@ -248,8 +229,7 @@ See the [Quick Start](#quick-start) above for a minimal example, or the ## Comparison Settings (DocxodusEngine only) `DocxodusEngine` supports fine-grained control over the comparison via keyword arguments to -`run_redline()`. Which arguments are available depends on the algorithm you select — the second -table below is the authoritative matrix. `XmlPowerToolsEngine` accepts none of them. +`run_redline()`. `XmlPowerToolsEngine` accepts none of them. ```python from python_redlines import DocxodusEngine @@ -258,8 +238,6 @@ engine = DocxodusEngine() redline_bytes, stdout, stderr = engine.run_redline( "Reviewer", original, modified, detect_moves=True, - simplify_move_markup=True, - detail_threshold=0.3, case_insensitive=True, ) ``` @@ -268,82 +246,58 @@ redline_bytes, stdout, stderr = engine.run_redline( | Setting | Type | Default | Description | |---|---|---|---| -| `engine` | str | `"wmlcomparer"` | Comparison algorithm: `"wmlcomparer"` or `"docxdiff"` | -| `detail_threshold` | float | `0.0` | Comparison granularity (0.0–1.0, lower = more detailed) | | `case_insensitive` | bool | `False` | Ignore case differences | | `detect_moves` | bool | `False` | Enable move detection | -| `simplify_move_markup` | bool | `False` | Convert moves to del/ins for Word compatibility | | `move_similarity_threshold` | float | `0.8` | Jaccard threshold for move matching (0.0–1.0) | | `move_minimum_word_count` | int | `3` | Minimum words for move detection | -| `detect_format_changes` | bool | `True` | Detect formatting-only changes | +| `detect_format_changes` | bool | `True` | Detect block-level formatting changes | | `conflate_spaces` | bool | `True` | Treat breaking/non-breaking spaces the same | | `date_time` | str | now | Custom ISO 8601 timestamp for revisions | -### Which engine accepts which setting - -| Setting | Docxodus · `wmlcomparer` | Docxodus · `docxdiff` | `XmlPowerToolsEngine` | -|---|:---:|:---:|:---:| -| `engine` | ✅ | ✅ | — ignored | -| `detail_threshold` | ✅ | ❌ `ValueError` | — ignored | -| `case_insensitive` | ✅ | ✅ | — ignored | -| `detect_moves` | ✅ | ✅ | — ignored | -| `simplify_move_markup` | ✅ | ❌ `ValueError` | — ignored | -| `move_similarity_threshold` | ✅ | ✅ | — ignored | -| `move_minimum_word_count` | ✅ | ✅ | — ignored | -| `detect_format_changes` | ✅ | ❌ `ValueError` | — ignored | -| `conflate_spaces` | ✅ | ✅ | — ignored | -| `date_time` | ✅ | ✅ | — ignored | - -**❌ `ValueError`** — `docxdiff` has no equivalent of these three settings. The underlying CLI accepts -and silently discards them, so Python rejects them up front rather than let you believe a setting took -effect when it did not. The check is on the *keyword being present*, whatever its value: pass -`detect_format_changes=True` (its default) with `engine="docxdiff"` and you still get a `ValueError`. -Drop the keyword, or use `engine="wmlcomparer"`. - -**— ignored** — `XmlPowerToolsEngine` silently discards every keyword argument, including `engine`. -This is long-standing behavior, not new. Passing `engine="docxdiff"` to it does nothing. - -> **Warning:** (`wmlcomparer` only) Move detection can cause Word to display "unreadable content" warnings due to a known -> ID collision bug. When using `detect_moves=True`, always set `simplify_move_markup=True` as well. -> This converts move markup to regular del/ins (loses green move styling but ensures Word compatibility). +Anything else raises `ValueError`, including a misspelled setting name. Passing a setting to +`XmlPowerToolsEngine` is still silently ignored — it has never accepted any. -### Choosing an engine +### Settings removed in 1.0.0 -`DocxodusEngine` wraps two comparison algorithms in one binary, selected per call: - -```python -engine.run_redline("Reviewer", original, modified) # wmlcomparer (default) -engine.run_redline("Reviewer", original, modified, engine="docxdiff") # opt in -``` - -| | Reach for `wmlcomparer` | Reach for `docxdiff` | -|---|---|---| -| **When** | You want the long-established algorithm | You want finer-grained, structure-aware redlines | -| **Maturity** | Years of production use | New in 0.3.0 — evaluate on your documents first | -| **Granularity knob** | `detail_threshold` tunes it | Not applicable; granularity is structural | -| **Moves** | Can be lowered to del/ins via `simplify_move_markup` | Rendered natively; cannot be lowered | -| **Structural edits** | Reported coarsely | Attributed to the paragraph, cell, row, or section | - -**The two disagree about revision counts, and that is expected.** On this project's own fixtures, -`wmlcomparer` reports 9 revisions and `docxdiff` reports 11 for the same pair of documents. They -segment the same edits differently — a single reworded sentence may be one revision to one engine and -two to the other. Do not treat a changed count as a defect; do compare the rendered redline against -your own documents before switching. - -**Move markup differs.** `docxdiff` renders moves natively and rejects `simplify_move_markup`, so the -Word-compatibility mitigation in the warning above is unavailable there. Whether Word's ID-collision -warning affects `docxdiff`'s native move markup is untested. If you need moves lowered to plain -del/ins for maximum Word compatibility, use `engine="wmlcomparer"` with `simplify_move_markup=True`. - -**Feedback wanted.** `docxdiff` stays off by default precisely so that adopting 0.3.0 cannot change -your output. If you try it, please -[open an issue](https://github.com/JSv4/Python-Redlines/issues) with what you found — especially -documents where its redline reads worse than `wmlcomparer`'s. +| Setting | Why it is gone | +|---|---| +| `engine` | Selected between `wmlcomparer` and `docxdiff`. Docxodus v11.0.0 deleted `WmlComparer`, so there is nothing to select. | +| `detail_threshold` | Tuned `WmlComparer`'s LCS granularity. `DocxDiff`'s granularity is structural and has no equivalent knob. | +| `simplify_move_markup` | Worked around `WmlComparer`'s move markup. `DocxDiff` renders moves natively. | + +All three raise `ValueError` naming the removal and what to do instead. The check is on the +*keyword being present*, whatever its value: `simplify_move_markup=False` raises too. + +The 0.3.0-era warning that `detect_moves=True` needed `simplify_move_markup=True` to avoid Word's +"unreadable content" dialog applied to `WmlComparer`'s move markup. It does not apply here — +`DocxDiff` emits move markup natively and has no such mitigation, because it needs none. + +## Upgrading to 1.0.0 + +**If you called `run_redline` with no keyword arguments, nothing in your code changes.** Your +output will change, because the algorithm changed. + +1. **Remove `engine=`, `detail_threshold=` and `simplify_move_markup=`.** They raise `ValueError`. + If you were passing `engine="docxdiff"`, delete the argument — you now get it by default. +2. **Re-baseline anything that asserts on revision counts.** On this project's own fixtures the + count moved from 9 (`wmlcomparer`) and 11 (`docxdiff` in 0.3.0) to **10**. Upstream v11 and v12 + changed region arrangement, surplus table cells and section defaults, so the 0.3.0 `docxdiff` + count is not the 1.0.0 count either. +3. **Check documents that already carry tracked changes.** Docxodus v11 dropped + `PreserveInputRevisions` from `DocxCompare.Compare`'s front door to match Word's own Compare + behaviour. Revision-bearing inputs therefore produce different output than in 0.3.0. This + project's fixtures carry no input revisions and will not warn you; diff a representative + document if this is your workload. +4. **Move off `XmlPowerToolsEngine`** when you can — it now warns, and will be removed in a future + major release. +5. **In the GitHub Action, remove the `comparison:` input.** It fails the run. + +Pin `python-redlines==0.3.0` if you need the old `wmlcomparer` output while you migrate. ## Architecture Overview Both engine classes follow the same pattern: a Python wrapper class invokes a self-contained C# binary -via subprocess. `DocxodusEngine`'s two algorithms are one binary selected by a CLI flag, not two binaries. +via subprocess. The repository is a **monorepo of three separately-published packages**: @@ -388,11 +342,9 @@ The engines produce slightly different stdout messages: | Engine | Example stdout | |---|---| | `XmlPowerToolsEngine` | `Revisions found: 9` | -| `DocxodusEngine` (default / `engine="wmlcomparer"`) | `Redline complete: 9 revision(s) found` | -| `DocxodusEngine` (`engine="docxdiff"`) | `Redline complete: 11 revision(s) found` | +| `DocxodusEngine` | `Redline complete: 10 revision(s) found` | -The revision counts differ between the two Docxodus engines because the algorithms differ, -not because either is wrong. +The counts differ because the algorithms differ, not because either is wrong. ## Python-Redlines vs. Commercial Alternatives diff --git a/action.yml b/action.yml index 3a69de0..23f9e7f 100644 --- a/action.yml +++ b/action.yml @@ -47,8 +47,10 @@ inputs: default: 'docxodus' comparison: description: >- - Comparison algorithm for the docxodus engine: 'wmlcomparer' (default) or - 'docxdiff'. + REMOVED. Docxodus v11.0.0 deleted WmlComparer, so there is no longer an + algorithm to select; DocxDiff is the only one. Setting this fails the run + rather than silently comparing with something other than what was asked + for. Remove the input. required: false default: '' detect-moves: diff --git a/action/redline_changed.py b/action/redline_changed.py index 4c71b1a..33c7b80 100644 --- a/action/redline_changed.py +++ b/action/redline_changed.py @@ -31,7 +31,16 @@ # kwargs only DocxodusEngine understands; XmlPowerToolsEngine would silently # ignore them, so requesting one with engine=xmlpowertools is a config error. -DOCXODUS_ONLY_INPUTS = ('comparison', 'detect-moves') +DOCXODUS_ONLY_INPUTS = ('detect-moves',) + +# Docxodus v11.0.0 deleted WmlComparer, and with it the engine selector this +# input mapped onto. Rejected outright rather than ignored: a workflow that +# pinned 'wmlcomparer' would otherwise keep running and quietly produce +# DocxDiff output. +COMPARISON_REMOVED = ( + "Input 'comparison' is no longer supported: the comparison-engine selector was " + "removed in Docxodus v11.0.0, which deleted WmlComparer. DocxDiff is now the only " + "algorithm — remove the input.") class ConfigError(Exception): @@ -116,18 +125,15 @@ def validate(self) -> None: raise ConfigError( "Inputs 'original' and 'modified' must be provided together " "(explicit-pair mode) or both left empty (auto-detect mode).") + if self.comparison: + raise ConfigError(COMPARISON_REMOVED) if self.engine != 'docxodus': - if self.comparison: - raise ConfigError( - "Input 'comparison' is only supported by the docxodus engine.") if self.detect_moves: raise ConfigError( "Input 'detect-moves' is only supported by the docxodus engine.") def engine_kwargs(self) -> Dict: kwargs: Dict = {} - if self.comparison: - kwargs['engine'] = self.comparison if self.detect_moves: kwargs['detect_moves'] = True return kwargs diff --git a/docs/alternatives.md b/docs/alternatives.md index fae6000..24a4903 100644 --- a/docs/alternatives.md +++ b/docs/alternatives.md @@ -66,10 +66,9 @@ processing API, including a document comparison endpoint, billed per API call. keeps document bytes in your own process the entire time — nothing to configure for data residency because nothing leaves. - **Redline fidelity.** Python-Redlines' default engine (Docxodus) supports move - detection, format-change detection, and structure-aware diffing (via the optional - `docxdiff` algorithm) — producing native Word tracked-changes output tuned - specifically for `.docx`, rather than a generic document-diff endpoint shared across - many file formats. + detection, format-change detection, and structure-aware diffing — producing native + Word tracked-changes output tuned specifically for `.docx`, rather than a generic + document-diff endpoint shared across many file formats. - **No vendor lock-in.** Cloudmersive's comparison logic is closed and proprietary. Python-Redlines is MIT-licensed open source: inspect the C# comparison engines, build them yourself, or contribute a fix upstream. @@ -98,9 +97,9 @@ launching Word, and without the TOS risk. ## High-performance, cross-platform document diffing -Python-Redlines' default engine, [Docxodus](https://github.com/JSv4/Docxodus), is a -modernized, actively-maintained .NET 10 fork of Open-XML-PowerTools' `WmlComparer` — -a high-performance document diffing engine purpose-built for cross-platform Word +Python-Redlines' default engine, [Docxodus](https://github.com/JSv4/Docxodus), is an +actively-maintained .NET 10 document toolchain whose `DocxDiff` algorithm is a +structure-aware, high-performance diffing engine purpose-built for cross-platform Word document comparison. It ships as a prebuilt, self-contained binary embedded directly in the Python wheel for Linux, macOS, and Windows (x64 and arm64), so there's no .NET SDK to install and no compilation step for end users — just `pip install diff --git a/docs/index.md b/docs/index.md index 194a0ab..7b18165 100644 --- a/docs/index.md +++ b/docs/index.md @@ -47,8 +47,8 @@ required. - **Native tracked-changes output** — the redline `.docx` opens in Word with real insertions, deletions, and moves, attributable to an author tag. - **Cross-platform, high-performance diffing engine** — the default - [Docxodus](https://github.com/JSv4/Docxodus) engine is a modernized .NET 10 - fork of Open-XML-PowerTools' `WmlComparer`, shipped as a prebuilt, self-contained + [Docxodus](https://github.com/JSv4/Docxodus) engine runs `DocxDiff`, a + structure-aware .NET 10 comparison algorithm, shipped as a prebuilt, self-contained binary embedded in the wheel for Linux, macOS, and Windows (x64/arm64) — nothing to compile. diff --git a/docs/quickstart.md b/docs/quickstart.md index 592cbb0..f14ba0c 100644 --- a/docs/quickstart.md +++ b/docs/quickstart.md @@ -2,7 +2,7 @@ `python-redlines` wraps a C# comparison engine to produce tracked-change redline `.docx` files. This guide uses `DocxodusEngine` — the default and recommended engine. -`XmlPowerToolsEngine` (legacy) shares the same call signature; the only behavioural +`XmlPowerToolsEngine` (deprecated) shares the same call signature; the only behavioural difference is that it silently ignores the keyword arguments shown in Step 4. ### Step 0: Install @@ -14,8 +14,8 @@ the engine binary is prebuilt and embedded in the wheel. pip install python-redlines[docxodus] ``` -Use `python-redlines[ooxmlpowertools]` for the legacy engine, or `python-redlines[all]` -for both. +Use `python-redlines[ooxmlpowertools]` for the deprecated engine, or +`python-redlines[all]` for both. ### Step 1: Import and Initialize the Wrapper @@ -58,19 +58,23 @@ with open('/path/to/redline_output.docx', 'wb') as f: ### Step 4: Tune the Comparison (optional, DocxodusEngine only) -`DocxodusEngine` accepts keyword arguments to control move detection, granularity, and -more. See the [main README](https://github.com/JSv4/Python-Redlines#comparison-settings-docxodusengine-only) for +`DocxodusEngine` accepts keyword arguments to control move detection, case sensitivity, +and more. See the [main README](https://github.com/JSv4/Python-Redlines#comparison-settings-docxodusengine-only) for the full table. ```python output = wrapper.run_redline( 'AuthorTag', original_bytes, modified_bytes, detect_moves=True, - simplify_move_markup=True, # required with detect_moves for Word compatibility - detail_threshold=0.3, + case_insensitive=True, ) ``` +An unrecognised keyword argument raises `ValueError`. The `engine`, `detail_threshold` +and `simplify_move_markup` arguments were removed in 1.0.0 along with the `WmlComparer` +algorithm they configured — see +[Upgrading to 1.0.0](https://github.com/JSv4/Python-Redlines#upgrading-to-100). + `XmlPowerToolsEngine` silently ignores these kwargs — switch engines if you need them. ### See also diff --git a/docs/tutorials/how-to-compare-word-documents-python.md b/docs/tutorials/how-to-compare-word-documents-python.md index 78bd594..3583c6c 100644 --- a/docs/tutorials/how-to-compare-word-documents-python.md +++ b/docs/tutorials/how-to-compare-word-documents-python.md @@ -93,26 +93,27 @@ redline_bytes, stdout, stderr = engine.run_redline("Reviewer", original_bytes, m with open("redline.docx", "wb") as f: f.write(redline_bytes) -print(stdout) # e.g. "Redline complete: 9 revision(s) found" +print(stdout) # e.g. "Redline complete: 10 revision(s) found" ``` ## Tuning the comparison -`DocxodusEngine` accepts keyword arguments for move detection, comparison -granularity, and more: +`DocxodusEngine` accepts keyword arguments for move detection, case sensitivity, +and more: ```python redline_bytes, stdout, stderr = engine.run_redline( "Reviewer", original_bytes, modified_bytes, detect_moves=True, - simplify_move_markup=True, # required alongside detect_moves for Word compatibility - detail_threshold=0.3, # lower = more detailed diff case_insensitive=True, ) ``` +An unrecognised setting raises `ValueError` rather than being ignored, so a typo +tells you about itself. + See the [comparison settings reference](https://github.com/JSv4/Python-Redlines#comparison-settings-docxodusengine-only) -for every option and which engine supports it. +for every option. ## Why not automate MS Word instead? @@ -129,7 +130,7 @@ more on this and other trade-offs (Draftable API, Cloudmersive, cloud data priva - [Quickstart guide](../quickstart.md) — the same walkthrough with more detail on engine choice -- [Comparison engines](https://github.com/JSv4/Python-Redlines#comparison-engines) — `wmlcomparer` vs - `docxdiff` vs the legacy Open-XML-PowerTools engine +- [Comparison engines](https://github.com/JSv4/Python-Redlines#comparison-engines) — Docxodus's + `DocxDiff` vs the deprecated Open-XML-PowerTools engine - [Live demo](https://redlines.opensource.legal) — try a comparison in your browser first diff --git a/docxodus b/docxodus index 47a543a..5ab5ac8 160000 --- a/docxodus +++ b/docxodus @@ -1 +1 @@ -Subproject commit 47a543a0a60a0c7be0b01dd3d1b6eec88af24d8a +Subproject commit 5ab5ac87bbc81d5348ea2f2de87f6237d479d4b2 diff --git a/packages/core/pyproject.toml b/packages/core/pyproject.toml index b0ba2b7..dcc7efc 100644 --- a/packages/core/pyproject.toml +++ b/packages/core/pyproject.toml @@ -27,7 +27,7 @@ authors = [ { name = "John Scrudato IV" }, ] classifiers = [ - "Development Status :: 4 - Beta", + "Development Status :: 5 - Production/Stable", "License :: OSI Approved :: MIT License", "Programming Language :: Python", "Programming Language :: Python :: 3.9", @@ -44,6 +44,9 @@ dependencies = [ # its extra, e.g. `pip install python-redlines[docxodus]`. All three packages # are released together from the same repository on each tagged release. [project.optional-dependencies] +# Deprecated: wraps the archived Open-XML-PowerTools WmlComparer. Kept so that +# anyone who needs that algorithm's output has somewhere to stand; `all` keeps +# installing it so the extra does not change meaning under existing pins. ooxmlpowertools = ["python-redlines-ooxmlpowertools"] docxodus = ["python-redlines-docxodus"] all = ["python-redlines-ooxmlpowertools", "python-redlines-docxodus"] diff --git a/packages/core/src/python_redlines/__about__.py b/packages/core/src/python_redlines/__about__.py index d769a50..f58919b 100644 --- a/packages/core/src/python_redlines/__about__.py +++ b/packages/core/src/python_redlines/__about__.py @@ -1,4 +1,4 @@ # SPDX-FileCopyrightText: 2024-present U.N. Owen # # SPDX-License-Identifier: MIT -__version__ = "0.3.0" +__version__ = "1.0.0" diff --git a/packages/core/src/python_redlines/engines.py b/packages/core/src/python_redlines/engines.py index 898ca42..3cf3cfe 100644 --- a/packages/core/src/python_redlines/engines.py +++ b/packages/core/src/python_redlines/engines.py @@ -6,6 +6,7 @@ import subprocess import tarfile import tempfile +import warnings import zipfile from pathlib import Path from typing import Optional, Tuple, Union @@ -138,23 +139,26 @@ def run_redline(self, author_tag: str, original: Union[str, bytes, Path], modifi Runs the redline binary. The 'original' and 'modified' arguments can be either bytes or file paths (as ``str`` or ``pathlib.Path``). Returns the redline output as bytes. - Additional keyword arguments are passed to _build_command() for engine-specific options. - DocxodusEngine supports: engine, detail_threshold, case_insensitive, detect_moves, - simplify_move_markup, move_similarity_threshold, move_minimum_word_count, - detect_format_changes, conflate_spaces, date_time. + A path the caller supplies is never deleted; only scratch files this + method creates are cleaned up. - DocxodusEngine's engine kwarg selects the comparison algorithm: 'wmlcomparer' - (the default) or 'docxdiff'. The docxdiff engine ignores detail_threshold, - simplify_move_markup, and detect_format_changes, so passing them alongside - engine='docxdiff' raises ValueError rather than silently changing nothing. + Additional keyword arguments are passed to _build_command() for engine-specific + options. DocxodusEngine supports: case_insensitive, detect_moves, + move_similarity_threshold, move_minimum_word_count, detect_format_changes, + conflate_spaces, date_time. It raises ValueError for unrecognised settings and + for the WmlComparer-era settings removed in Docxodus v11.0.0 (engine, + detail_threshold, simplify_move_markup). XmlPowerToolsEngine ignores kwargs. """ - temp_files = [] + scratch_files = [] try: + # mkstemp, not NamedTemporaryFile: we want a path, and NamedTemporaryFile + # hands back an open file object that nothing here would close (issue #30). + handle, target_path = tempfile.mkstemp(suffix='.docx') + os.close(handle) + scratch_files.append(target_path) - target_path = tempfile.NamedTemporaryFile(delete=False).name - original_path = self._write_to_temp_file(original) if isinstance(original, bytes) else original - modified_path = self._write_to_temp_file(modified) if isinstance(modified, bytes) else modified - temp_files.extend([target_path, original_path, modified_path]) + original_path = self._as_path(original, scratch_files) + modified_path = self._as_path(modified, scratch_files) command = self._build_command(author_tag, original_path, modified_path, target_path, **kwargs) @@ -169,14 +173,27 @@ def run_redline(self, author_tag: str, original: Union[str, bytes, Path], modifi return redline_output, stdout_output, stderr_output finally: - self._cleanup_temp_files(temp_files) + self._cleanup_temp_files(scratch_files) + + def _as_path(self, document, scratch_files): + """Return a filesystem path for *document*, writing bytes out if needed. + + Only a path this method creates is appended to *scratch_files*. A path + the caller passed in is the caller's own document, and registering it + for cleanup would delete the file they asked us to compare. + """ + if isinstance(document, bytes): + path = self._write_to_temp_file(document) + scratch_files.append(path) + return path + return os.fspath(document) def _cleanup_temp_files(self, temp_files): for file_path in temp_files: try: os.remove(file_path) except OSError as e: - print(f"Error deleting temp file {file_path}: {e}") + logger.warning("Error deleting temp file %s: %s", file_path, e) def _write_to_temp_file(self, data): """ @@ -193,24 +210,49 @@ class XmlPowerToolsEngine(BaseEngine): BINARY_BASE_NAME = 'redlines' EXTRA_NAME = 'ooxmlpowertools' + def __init__(self, target_path: Optional[str] = None): + warnings.warn( + "XmlPowerToolsEngine wraps the original, unmaintained Open-XML-PowerTools " + "WmlComparer and is deprecated; it will be removed in a future major " + "release. Use DocxodusEngine, which is actively maintained and runs the " + "DocxDiff algorithm.", + DeprecationWarning, + stacklevel=2, + ) + super().__init__(target_path) + class DocxodusEngine(BaseEngine): BINARY_PACKAGE = 'python_redlines_docxodus' BINARY_BASE_NAME = 'redline' EXTRA_NAME = 'docxodus' - # Comparison engines accepted by the redline CLI's --engine flag. - ENGINES = ('wmlcomparer', 'docxdiff') - - # DocxCompare.ToDocxDiffSettings drops these on the docxdiff branch, and the CLI - # accepts them there without complaint, so reject them before we shell out. - _WMLCOMPARER_ONLY = ('detail_threshold', 'simplify_move_markup', 'detect_format_changes') + # Settings that died with WmlComparer in Docxodus v11.0.0. They are rejected + # rather than dropped, because dropping them would not be a breaking change + # but a wrong-answer one: the v12 CLI rejects --engine as an unknown flag, + # and merely warns-and-ignores the other two, so a caller who passed + # engine='wmlcomparer' would silently receive DocxDiff output believing they + # had selected something else. + _REMOVED_KWARGS = { + 'engine': ( + "the comparison-engine selector was removed in Docxodus v11.0.0, which " + "deleted WmlComparer. DocxDiff is now the only algorithm — drop the argument" + ), + 'detail_threshold': ( + "it tuned WmlComparer's LCS granularity and went with it in Docxodus " + "v11.0.0. DocxDiff's granularity is structural, with no equivalent knob — " + "drop the argument" + ), + 'simplify_move_markup': ( + "it worked around WmlComparer's move markup and went with it in Docxodus " + "v11.0.0. DocxDiff renders moves natively — drop the argument" + ), + } # Boolean flags (default False — presence enables) _BOOL_FLAGS = [ ('case_insensitive', '--case-insensitive'), ('detect_moves', '--detect-moves'), - ('simplify_move_markup', '--simplify-move-markup'), ] # Negatable flags (default True — --no- prefix disables) @@ -221,43 +263,31 @@ class DocxodusEngine(BaseEngine): # Value flags _VALUE_FLAGS = [ - ('detail_threshold', '--detail-threshold'), ('move_similarity_threshold', '--move-similarity-threshold'), ('move_minimum_word_count', '--move-minimum-word-count'), ('date_time', '--date-time'), ] @classmethod - def _normalize_engine(cls, kwargs): - """The chosen engine, lowercased and stripped, or None if the caller didn't pick one.""" - if 'engine' not in kwargs: - return None - - engine = kwargs['engine'] - if not isinstance(engine, str): - raise ValueError(f"engine must be a string, got {engine!r}") - - normalized = engine.strip().lower() - if normalized not in cls.ENGINES: - raise ValueError( - f"engine must be one of {', '.join(cls.ENGINES)}, got {engine!r}" - ) - return normalized + def _supported_kwargs(cls): + """Every comparison setting this engine still understands.""" + return {name for name, _ in (*cls._BOOL_FLAGS, *cls._NEG_FLAGS, *cls._VALUE_FLAGS)} @classmethod def _validate_kwargs(cls, kwargs): - if cls._normalize_engine(kwargs) == 'docxdiff': - for name in cls._WMLCOMPARER_ONLY: - if name in kwargs: - raise ValueError( - f"{name} is not supported by the 'docxdiff' engine " - f"(WmlComparer-only). Remove it or use engine='wmlcomparer'." - ) - - if 'detail_threshold' in kwargs: - val = kwargs['detail_threshold'] - if not isinstance(val, (int, float)) or val < 0.0 or val > 1.0: - raise ValueError(f"detail_threshold must be a float between 0.0 and 1.0, got {val!r}") + # Removed settings first, so they get their specific explanation rather + # than being lumped in with typos below. + for name, reason in cls._REMOVED_KWARGS.items(): + if name in kwargs: + raise ValueError(f"{name} is no longer supported: {reason}.") + + supported = cls._supported_kwargs() + unknown = sorted(set(kwargs) - supported) + if unknown: + raise ValueError( + f"Unknown comparison setting(s): {', '.join(unknown)}. " + f"Supported settings: {', '.join(sorted(supported))}." + ) if 'move_similarity_threshold' in kwargs: val = kwargs['move_similarity_threshold'] @@ -271,14 +301,10 @@ def _validate_kwargs(cls, kwargs): def _build_command(self, author_tag, original_path, modified_path, target_path, **kwargs): self._validate_kwargs(kwargs) - engine = self._normalize_engine(kwargs) cmd = [self.extracted_binaries_path, original_path, modified_path, target_path, f'--author={author_tag}'] - if engine is not None: - cmd.append(f'--engine={engine}') - for kwarg, flag in self._BOOL_FLAGS: if kwargs.get(kwarg): cmd.append(flag) diff --git a/tests/test_action_script.py b/tests/test_action_script.py index 4ac2ad9..79acb4a 100644 --- a/tests/test_action_script.py +++ b/tests/test_action_script.py @@ -59,18 +59,34 @@ def test_inputs_defaults(): def test_inputs_engine_kwargs(): - inputs = ra.Inputs.from_env({ - 'INPUT_COMPARISON': 'docxdiff', - 'INPUT_DETECT_MOVES': 'true', - }) - assert inputs.engine_kwargs() == {'engine': 'docxdiff', 'detect_moves': True} + inputs = ra.Inputs.from_env({'INPUT_DETECT_MOVES': 'true'}) + assert inputs.engine_kwargs() == {'detect_moves': True} + + +@pytest.mark.parametrize('value', ['wmlcomparer', 'docxdiff']) +def test_comparison_input_is_rejected(value): + """Docxodus v11.0.0 deleted the engine selector the input mapped onto. + + Accepting it would either crash in the CLI or, worse, quietly produce + DocxDiff output for a workflow that asked for WmlComparer. + """ + with pytest.raises(ra.ConfigError, match='comparison'): + ra.Inputs.from_env({'INPUT_COMPARISON': value}) + + +def test_comparison_rejection_explains_the_removal(): + with pytest.raises(ra.ConfigError) as excinfo: + ra.Inputs.from_env({'INPUT_COMPARISON': 'wmlcomparer'}) + + message = str(excinfo.value) + assert 'v11.0.0' in message + assert 'DocxDiff' in message @pytest.mark.parametrize('env', [ {'INPUT_ENGINE': 'wordperfect'}, {'INPUT_HTML_PREVIEW': 'maybe'}, {'INPUT_ORIGINAL': 'a.docx'}, # original without modified - {'INPUT_ENGINE': 'xmlpowertools', 'INPUT_COMPARISON': 'docxdiff'}, {'INPUT_ENGINE': 'xmlpowertools', 'INPUT_DETECT_MOVES': 'true'}, {'INPUT_DETECT_MOVES': 'yes'}, # not a bool ]) @@ -268,7 +284,7 @@ def test_main_explicit_pair(tmp_path, monkeypatch): assert 'count=1\n' in text payload = [line for line in text.splitlines() if line.startswith('redlines=')][0] record = json.loads(payload[len('redlines='):])[0] - assert record['revisions'] == 9 + assert record['revisions'] == 10 redline = Path(record['redline']) assert redline.is_file() and redline.stat().st_size > 0 # absolute source paths must not escape the requested output directory @@ -299,6 +315,6 @@ def test_main_auto_detect_over_git_history(repo, tmp_path, monkeypatch): record = json.loads(payload[len('redlines='):])[0] assert record['path'] == 'contracts/agreement.docx' assert record['status'] == 'modified' - assert record['revisions'] == 9 + assert record['revisions'] == 10 assert Path(record['redline']).is_file() assert 'contracts/agreement.docx' in summary_file.read_text() diff --git a/tests/test_docxodus_engine.py b/tests/test_docxodus_engine.py index fa1c4a9..abb961a 100644 --- a/tests/test_docxodus_engine.py +++ b/tests/test_docxodus_engine.py @@ -1,9 +1,16 @@ +import io import re +import zipfile import pytest from python_redlines.engines import DocxodusEngine +# Measured against the Docxodus v12.1.0 binary on tests/fixtures/. Docxodus +# v11.0.0 removed WmlComparer, so DocxDiff is the only algorithm; the count is +# neither the 9 the old default reported nor the 11 the old opt-in reported. +EXPECTED_REVISIONS = 10 + def load_docx_bytes(file_path): with open(file_path, 'rb') as file: @@ -41,25 +48,36 @@ def test_run_docxodus_with_real_files(original_docx, modified_docx): assert "revision(s) found" in stdout -# --- Integration tests for comparison settings --- - -def test_docxodus_with_detect_moves(original_docx, modified_docx): +def test_docxodus_revision_count_is_pinned(original_docx, modified_docx): + """The regression anchor for the DocxDiff-only engine.""" engine = DocxodusEngine() redline_output, stdout, stderr = engine.run_redline( "TestAuthor", original_docx, modified_docx, - detect_moves=True, simplify_move_markup=True, ) - assert redline_output is not None - assert len(redline_output) > 0 assert stderr is None - assert "revision(s) found" in stdout + assert revision_count(stdout) == EXPECTED_REVISIONS + assert redline_output[:2] == b"PK" -def test_docxodus_with_detail_threshold(original_docx, modified_docx): +def test_docxodus_output_is_a_valid_docx_with_tracked_changes(original_docx, modified_docx): + engine = DocxodusEngine() + redline_output, _, _ = engine.run_redline("TestAuthor", original_docx, modified_docx) + + with zipfile.ZipFile(io.BytesIO(redline_output)) as archive: + assert archive.testzip() is None + document_xml = archive.read("word/document.xml").decode("utf-8") + + assert " 0 @@ -92,13 +110,12 @@ def test_docxodus_with_no_format_changes(original_docx, modified_docx): def test_docxodus_with_all_options(original_docx, modified_docx): + """Every surviving setting at once, and the CLI stays quiet on stderr.""" engine = DocxodusEngine() redline_output, stdout, stderr = engine.run_redline( "TestAuthor", original_docx, modified_docx, - detail_threshold=0.3, case_insensitive=True, detect_moves=True, - simplify_move_markup=True, move_similarity_threshold=0.7, move_minimum_word_count=2, detect_format_changes=False, @@ -111,13 +128,86 @@ def test_docxodus_with_all_options(original_docx, modified_docx): assert "revision(s) found" in stdout -# --- Validation tests --- +# --- Settings removed with WmlComparer (Docxodus v11.0.0) --- +# +# These must raise rather than be dropped. Two of them are still *accepted* by +# the v12 CLI, which warns on stderr and ignores them; `engine` is rejected as +# an unknown flag. Silently discarding any of them would hand the caller +# DocxDiff output while they believe they configured something else. + +@pytest.mark.parametrize("value", ["wmlcomparer", "docxdiff", "WmlComparer", " docxdiff "]) +def test_engine_kwarg_is_rejected(value): + engine = DocxodusEngine() + with pytest.raises(ValueError, match=r"engine .*no longer"): + engine._build_command("Author", "orig", "mod", "out", engine=value) + + +def test_detail_threshold_is_rejected(): + engine = DocxodusEngine() + with pytest.raises(ValueError, match=r"detail_threshold .*no longer"): + engine._build_command("Author", "orig", "mod", "out", detail_threshold=0.5) + + +def test_simplify_move_markup_is_rejected(): + engine = DocxodusEngine() + with pytest.raises(ValueError, match=r"simplify_move_markup .*no longer"): + engine._build_command("Author", "orig", "mod", "out", simplify_move_markup=True) + -def test_docxodus_invalid_detail_threshold(): +@pytest.mark.parametrize("kwarg", ["engine", "detail_threshold", "simplify_move_markup"]) +def test_removed_kwarg_error_names_the_replacement(kwarg): + """The message has to tell the reader what to do, not just that they are wrong.""" + values = {"engine": "docxdiff", "detail_threshold": 0.5, "simplify_move_markup": True} engine = DocxodusEngine() - with pytest.raises(ValueError, match="detail_threshold must be a float between 0.0 and 1.0"): - engine._build_command("Author", "orig", "mod", "out", detail_threshold=1.5) + with pytest.raises(ValueError) as excinfo: + engine._build_command("Author", "orig", "mod", "out", **{kwarg: values[kwarg]}) + message = str(excinfo.value) + assert "v11.0.0" in message + assert "DocxDiff" in message + + +def test_removed_kwarg_is_rejected_even_when_false(): + """The check is on the keyword being present, whatever its value.""" + engine = DocxodusEngine() + with pytest.raises(ValueError, match="no longer"): + engine._build_command("Author", "orig", "mod", "out", simplify_move_markup=False) + + +def test_removed_kwarg_rejection_reaches_run_redline(original_docx, modified_docx): + """The guard is on the public call, not only on the private builder.""" + engine = DocxodusEngine() + with pytest.raises(ValueError, match="no longer"): + engine.run_redline("TestAuthor", original_docx, modified_docx, engine="docxdiff") + + +# --- Unknown settings --- + +def test_unknown_kwarg_is_rejected(): + """A typo used to be discarded in silence, which reads as 'the setting did nothing'.""" + engine = DocxodusEngine() + with pytest.raises(ValueError, match="detial_threshold"): + engine._build_command("Author", "orig", "mod", "out", detial_threshold=0.5) + + +def test_unknown_kwarg_error_lists_the_supported_settings(): + engine = DocxodusEngine() + with pytest.raises(ValueError) as excinfo: + engine._build_command("Author", "orig", "mod", "out", nonsense=True) + + message = str(excinfo.value) + assert "case_insensitive" in message + assert "detect_moves" in message + + +def test_removed_kwarg_beats_unknown_kwarg_reporting(): + """A removed setting gets its specific message, not the generic 'unknown' one.""" + engine = DocxodusEngine() + with pytest.raises(ValueError, match="no longer"): + engine._build_command("Author", "orig", "mod", "out", engine="docxdiff", nonsense=True) + + +# --- Validation of surviving settings --- def test_docxodus_invalid_move_similarity_threshold(): engine = DocxodusEngine() @@ -137,7 +227,7 @@ def test_docxodus_invalid_move_minimum_word_count_type(): engine._build_command("Author", "orig", "mod", "out", move_minimum_word_count=2.5) -# --- Unit test for _build_command flag construction --- +# --- Unit tests for _build_command flag construction --- def test_build_command_default(): engine = DocxodusEngine() @@ -149,14 +239,19 @@ def test_build_command_default(): assert len(cmd) == 5 # binary + 3 positional + --author +def test_build_command_never_emits_an_engine_flag(): + """--engine= was removed from the CLI in v11.0.0; emitting it exits 1.""" + engine = DocxodusEngine() + cmd = engine._build_command("Author", "/tmp/o.docx", "/tmp/m.docx", "/tmp/out.docx") + assert not any(str(arg).startswith("--engine") for arg in cmd) + + def test_build_command_with_all_flags(): engine = DocxodusEngine() cmd = engine._build_command( "Author", "/tmp/orig.docx", "/tmp/mod.docx", "/tmp/out.docx", - detail_threshold=0.5, case_insensitive=True, detect_moves=True, - simplify_move_markup=True, move_similarity_threshold=0.7, move_minimum_word_count=2, detect_format_changes=False, @@ -166,10 +261,8 @@ def test_build_command_with_all_flags(): assert "--author=Author" in cmd assert "--case-insensitive" in cmd assert "--detect-moves" in cmd - assert "--simplify-move-markup" in cmd assert "--no-detect-format-changes" in cmd assert "--no-conflate-spaces" in cmd - assert "--detail-threshold=0.5" in cmd assert "--move-similarity-threshold=0.7" in cmd assert "--move-minimum-word-count=2" in cmd assert "--date-time=2025-01-01T00:00:00Z" in cmd @@ -197,149 +290,3 @@ def test_build_command_negatable_true_not_added(): ) assert "--no-detect-format-changes" not in cmd assert "--no-conflate-spaces" not in cmd - - -# --- Engine selection (Docxodus v7.0.0 --engine flag) --- - -def test_build_command_engine_omitted_by_default(): - """No engine= kwarg means no --engine flag: the argv stays as it was pre-v7.""" - engine = DocxodusEngine() - cmd = engine._build_command("Author", "/tmp/o.docx", "/tmp/m.docx", "/tmp/out.docx") - assert not any(arg.startswith("--engine") for arg in cmd) - - -def test_build_command_engine_docxdiff(): - engine = DocxodusEngine() - cmd = engine._build_command( - "Author", "/tmp/o.docx", "/tmp/m.docx", "/tmp/out.docx", engine="docxdiff", - ) - assert "--engine=docxdiff" in cmd - - -def test_build_command_engine_explicit_wmlcomparer(): - engine = DocxodusEngine() - cmd = engine._build_command( - "Author", "/tmp/o.docx", "/tmp/m.docx", "/tmp/out.docx", engine="wmlcomparer", - ) - assert "--engine=wmlcomparer" in cmd - - -def test_build_command_engine_is_normalized(): - engine = DocxodusEngine() - cmd = engine._build_command( - "Author", "/tmp/o.docx", "/tmp/m.docx", "/tmp/out.docx", engine=" DocxDiff ", - ) - assert "--engine=docxdiff" in cmd - - -def test_build_command_unknown_engine(): - engine = DocxodusEngine() - with pytest.raises(ValueError, match="engine must be one of"): - engine._build_command("Author", "orig", "mod", "out", engine="bogus") - - -def test_build_command_non_string_engine(): - engine = DocxodusEngine() - with pytest.raises(ValueError, match="engine must be a string"): - engine._build_command("Author", "orig", "mod", "out", engine=1) - - -@pytest.mark.parametrize("kwarg, value", [ - ("detail_threshold", 0.5), - ("detail_threshold", 0.0), - ("simplify_move_markup", True), - ("simplify_move_markup", False), - ("detect_format_changes", True), - ("detect_format_changes", False), -]) -def test_docxdiff_rejects_wmlcomparer_only_kwargs(kwarg, value): - """docxdiff silently ignores these in C#; reject on key presence, whatever the value.""" - engine = DocxodusEngine() - expected = f"{kwarg} is not supported by the 'docxdiff' engine" - with pytest.raises(ValueError, match=expected): - engine._build_command("Author", "orig", "mod", "out", engine="docxdiff", **{kwarg: value}) - - -def test_wmlcomparer_still_allows_its_own_kwargs(): - engine = DocxodusEngine() - cmd = engine._build_command( - "Author", "orig", "mod", "out", - engine="wmlcomparer", detail_threshold=0.5, simplify_move_markup=True, - ) - assert "--engine=wmlcomparer" in cmd - assert "--detail-threshold=0.5" in cmd - assert "--simplify-move-markup" in cmd - - -def test_docxdiff_allows_the_kwargs_it_honours(): - engine = DocxodusEngine() - cmd = engine._build_command( - "Author", "orig", "mod", "out", - engine="docxdiff", detect_moves=True, case_insensitive=True, - conflate_spaces=False, move_similarity_threshold=0.7, move_minimum_word_count=2, - ) - assert "--engine=docxdiff" in cmd - assert "--detect-moves" in cmd - assert "--case-insensitive" in cmd - assert "--no-conflate-spaces" in cmd - assert "--move-similarity-threshold=0.7" in cmd - assert "--move-minimum-word-count=2" in cmd - - -def test_docxdiff_engine_check_precedes_range_check(): - """engine='docxdiff' + an out-of-range detail_threshold reports the engine problem.""" - engine = DocxodusEngine() - with pytest.raises(ValueError, match="not supported by the 'docxdiff' engine"): - engine._build_command("Author", "orig", "mod", "out", engine="docxdiff", detail_threshold=1.5) - - -# --- Engine selection, end to end --- - -def test_docxodus_default_engine_is_wmlcomparer(original_docx, modified_docx): - """The default path is the regression anchor: 9 revisions, exactly as before v7.""" - engine = DocxodusEngine() - redline_output, stdout, stderr = engine.run_redline( - "TestAuthor", original_docx, modified_docx, - ) - assert stderr is None - assert "Redline complete: 9 revision(s) found" in stdout - assert redline_output[:2] == b"PK" - - -def test_docxodus_docxdiff_engine(original_docx, modified_docx): - """docxdiff is a different algorithm and finds a different number of revisions.""" - engine = DocxodusEngine() - redline_output, stdout, stderr = engine.run_redline( - "TestAuthor", original_docx, modified_docx, engine="docxdiff", - ) - assert stderr is None - assert "Redline complete: 11 revision(s) found" in stdout - assert redline_output[:2] == b"PK" - - -def test_docxodus_explicit_wmlcomparer_matches_default(original_docx, modified_docx): - engine = DocxodusEngine() - _, default_stdout, _ = engine.run_redline("TestAuthor", original_docx, modified_docx) - _, explicit_stdout, _ = engine.run_redline( - "TestAuthor", original_docx, modified_docx, engine="wmlcomparer", - ) - default_count = revision_count(default_stdout) - explicit_count = revision_count(explicit_stdout) - assert explicit_count == default_count - assert default_count == 9 - - -def test_docxdiff_output_is_a_valid_docx_with_tracked_changes(original_docx, modified_docx): - import io - import zipfile - - engine = DocxodusEngine() - redline_output, _, _ = engine.run_redline( - "TestAuthor", original_docx, modified_docx, engine="docxdiff", - ) - with zipfile.ZipFile(io.BytesIO(redline_output)) as archive: - assert archive.testzip() is None - document_xml = archive.read("word/document.xml").decode("utf-8") - - assert " 0 assert stderr is None assert "Revisions found: 9" in stdout + + +def test_xmlpowertools_engine_is_deprecated(): + """It wraps the original, unmaintained Open-XML-PowerTools WmlComparer. + + The package keeps shipping and the class keeps working; instantiating it + has to say that it is on the way out and name what to use instead. + """ + with pytest.warns(DeprecationWarning) as record: + XmlPowerToolsEngine() + + message = str(record[0].message) + assert "DocxodusEngine" in message + + +def test_xmlpowertools_deprecation_points_at_the_caller(): + """stacklevel must blame the user's construction site, not engines.py.""" + with pytest.warns(DeprecationWarning) as record: + XmlPowerToolsEngine() + + assert record[0].filename == __file__ diff --git a/tests/test_temp_file_handling.py b/tests/test_temp_file_handling.py new file mode 100644 index 0000000..faf21e9 --- /dev/null +++ b/tests/test_temp_file_handling.py @@ -0,0 +1,117 @@ +"""Temp-file handling in BaseEngine.run_redline. + +Two bugs live in the same handful of lines, so they are tested together: + +- Issue #30: the output scratch file was created as + ``NamedTemporaryFile(delete=False).name``, which drops the only reference to + the file object. Its finalizer then reports a ResourceWarning and the + descriptor stays open until collection. +- Path inputs were registered for deletion alongside the engine's own scratch + files, so passing a path — a documented input mode — deleted the caller's + source documents. +""" +import gc +import shutil +import sys +import tempfile +import warnings + +import pytest + +from python_redlines.engines import DocxodusEngine + + +@pytest.fixture +def engine(): + return DocxodusEngine() + + +@pytest.fixture +def docs(tmp_path): + """A private copy of the fixtures, so a destructive bug cannot damage them.""" + original = tmp_path / 'original.docx' + modified = tmp_path / 'modified.docx' + shutil.copy('tests/fixtures/original.docx', original) + shutil.copy('tests/fixtures/modified.docx', modified) + return original, modified + + +def test_run_redline_closes_every_temp_file_it_opens(engine, docs, monkeypatch): + """Issue #30, in a form that is observable on every CPython version. + + The ResourceWarning the issue reports is only emitted on 3.13+, so this + asserts the underlying defect instead: a temp file object that run_redline + creates must be closed by the time the call returns. Holding a reference in + the spy is what makes the leak visible — it is precisely the reference the + buggy code drops, letting the finalizer close the file and hide the fault. + """ + original, modified = docs + created = [] + real_named_temp_file = tempfile.NamedTemporaryFile + + def spy(*args, **kwargs): + handle = real_named_temp_file(*args, **kwargs) + created.append(handle) + return handle + + monkeypatch.setattr(tempfile, 'NamedTemporaryFile', spy) + engine.run_redline('Author', original.read_bytes(), modified.read_bytes()) + + unclosed = [handle for handle in created if not handle.closed] + assert not unclosed, ( + f'run_redline abandoned {len(unclosed)} unclosed temp file object(s); ' + 'this is what raises ResourceWarning on CPython 3.13+ (issue #30)' + ) + + +@pytest.mark.skipif( + sys.version_info < (3, 13), + reason="tempfile's finalizer only emits ResourceWarning on CPython 3.13+", +) +def test_run_redline_emits_no_resource_warning(engine, docs): + """The exact symptom issue #30 reports, on the versions that can show it.""" + original, modified = docs + + with warnings.catch_warnings(): + warnings.simplefilter('error', ResourceWarning) + engine.run_redline('Author', original.read_bytes(), modified.read_bytes()) + gc.collect() + + +def test_run_redline_keeps_path_inputs_on_disk(engine, docs): + """Passing paths must not delete the caller's documents.""" + original, modified = docs + + redline_bytes, _, _ = engine.run_redline('Author', str(original), str(modified)) + + assert original.exists(), 'run_redline deleted the original document it was given' + assert modified.exists(), 'run_redline deleted the modified document it was given' + assert redline_bytes[:2] == b'PK' + + +def test_run_redline_accepts_pathlib_paths(engine, docs): + """The docstring and type hint promise pathlib.Path works, not just str.""" + original, modified = docs + + redline_bytes, _, _ = engine.run_redline('Author', original, modified) + + assert original.exists() + assert modified.exists() + assert redline_bytes[:2] == b'PK' + + +def test_run_redline_cleans_up_its_own_scratch_files(engine, docs, tmp_path, monkeypatch): + """The fix must not overshoot: engine-created temp files still get removed. + + tempfile is pointed at a private directory so the assertion sees only what + this call created, not whatever else is using the system temp directory. + """ + original, modified = docs + scratch = tmp_path / 'scratch' + scratch.mkdir() + monkeypatch.setattr(tempfile, 'tempdir', str(scratch)) + + engine.run_redline('Author', original.read_bytes(), modified.read_bytes()) + + leaked = sorted(path.name for path in scratch.iterdir()) + assert not leaked, f'run_redline left scratch files behind: {leaked}'