Repository navigation
fix: keep run formatting across empty split segments (ROADMAP.md #79) - #95
Merged
Merged
Conversation
_apply_paragraph_splits reset the boundary rPr fallback on every iteration, so a paragraph left runless by an empty segment cleared it and the next segment dropped to document default. Initialize the fallback once and update it only when the paragraph being split has runs.
|
Warning Review limit reachedNext included review available in 59 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #95 +/- ##
=====================================
Coverage 97.9% 97.9%
=====================================
Files 23 23
Lines 5136 5138 +2
Branches 991 992 +1
=====================================
+ Hits 5033 5035 +2
Misses 49 49
Partials 54 54 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…ests The callee docstring still described fallback_rPr_xml as the previous paragraph's formatting; with the value loop-carried it can come from several paragraphs back. Make the test helper's assertion explicit in its name and take w:t text through the codebase's usual TEXT_NODE guard.
pablospe
added a commit
that referenced
this pull request
Aug 30, 2026
…ed items docs/api.md promised an author= call "resolves that author's own elements and no one else's", which overstates a data-loss guarantee: the scoping decides which elements are targeted, not what survives, and rejecting an insertion still carries away revisions another author nested inside it. api.md's own rationale-notes section already said so. UnhandledRevision's docstring claimed its rows "carry no adjudicable id, so handing them to accept_revision() could only fail". An unhandled mark often does carry a numeric w:id, and where it collides with a handled revision's the call resolves that other element: verified, reject_revision(row.id) on a w:rPrChange id=7 deleted a w:ins id=7's text and left the rPrChange standing. Filed as #87. main added its own #83 (PR #95) after this branch forked, so the items filed here move to #84/#85/#86 and #75's cross-reference follows them.
pablospe
added a commit
that referenced
this pull request
Aug 31, 2026
…#96) * fix: author-filtered accept_all/reject_all resolve only that author's elements (ROADMAP.md #75) accept_all(author=A) and reject_all(author=A) selected revisions by author but resolved them by w:id against an index holding every author's elements. Where a producer repeats a w:id across authors (LibreOffice does, and the corpus has such files), the id lookup returned the other author's element: reject_all(author="A") deleted B's inserted text and accept_all(author="A") made it permanent, both reporting 2. _revision_element_index gains an optional author parameter and _resolve_all passes it, so a filtered call has no other author's element to land on. The filter uses list_revisions' own author expression -- a missing w:author reads as "Unknown" -- so the index and the listing cannot disagree about who owns an element, which is what would otherwise turn the fix into a silent no-op. The group/changeset path (_resolve_ids) needs no author scoping: a member id is never a duplicated one, since _reconstruct_groups bars every occurrence of a repeated w:id from every group and _seed_max_change_id keeps a freshly allocated id above every id already in the file. * test: pin author-filtered resolution against duplicate w:ids across authors (ROADMAP.md #75) Flips the pin that documented the defect (accept_all's filtered pass no longer resolves another author's same-id revision as collateral) and adds the exact ROADMAP shape -- B's w:ins w:id="7" before A's w:del w:id="7" -- with the data-loss case, the accept counterpart, both unfiltered paths (so the scoping cannot regress into over-filtering), and a count-equals-listed-rows check. test_unattributed_revision_resolves_under_the_unknown_author covers the other half of the index's author expression: dropping the "Unknown" fallback leaves accept_all(author="Unknown") returning 0 on an unattributed revision, with the revision still pending and no warning raised, and passed every other test. TestGroupResolutionWithDuplicateIds pins why the group/changeset path needs no author scoping: both duplicate rows report group_id and changeset_id None, the neighbouring real group resolves exactly its own member, and the duplicated id is not a group or changeset id either. * docs: state that author= resolution holds across duplicate w:ids (ROADMAP.md #75) The "author= filters process each author's changes independently" bullet now says the guarantee survives a producer repeating a w:id across authors. * fix: key the w:id index on the id the listing reports (ROADMAP.md #75) The index keyed on the raw w:id attribute while every lookup asks for str(int(w:id)) and the listing reports int(w:id), so a nonconforming w:id="007" listed as id 7 and was reachable by nothing: accept_all() returned 0, left it pending, and reported a clean document. Nothing warned, because a mark with a numeric id is not part of the unhandled honesty floor -- and groups_are_dead() read the live group as dead, which reaps its note comment. Both sides now normalize on _adjudicable_id: the index keys on str of it and drops elements that have none (nothing id-keyed can reach those, and _unhandled_elements already reports them), and the fresh scan in _find_revision_element compares the parsed id instead of the raw string. Every consumer already looked ids up in the normalized form. _revision_author joins _adjudicable_id in dom.py as the one definition of the other half of a revision's identity, replacing five copies of `getAttribute("w:author") or "Unknown"` in listing.py. That makes the index-and-listing agreement this fix rests on structural rather than a docstring claim backed by one regression test. * test: ask for an unallocated group id rather than a revision id (ROADMAP.md #75) test_unknown_id_still_raises passed for the wrong reason: it asked accept_group/accept_changeset for id 7 -- a w:id -- and group and changeset ids come from their own counters, so it raised only because this fixture's counters never reach 7, and would have started testing the opposite thing had they. It now asks for an id past the one actually allocated. DUPLICATE_ID_BODY builds its runs with the file's own _ins_xml/_del_xml helpers, which emit byte-identical XML, instead of hand-writing them. * docs: note the per-id duplicate-w:id caveat and file the adjacent defects The duplicate-w:id guarantee belongs to accept_all/reject_all alone: accept_revision(id)/reject_revision(id) take only the id, so a duplicated one resolves whichever element comes first in the document. SKILL.md recommended the per-id pattern with no caveat; it now points at the bulk call or the row's group_id when an id is shared. Files three defects the PR #96 multi-review turned up, all pre-existing and each needing its own change and regression test: - #83 the per-id half of #75, with a zero-cost ambiguity test the manager already computes - #84 _rejoin_paragraph drops the tail paragraph's w:pPr, destroying another author's paragraph-mark revisions uncounted - #85 marks recorded inside a change record list as live revisions * fix: resolving revision id None must match nothing (ROADMAP.md #75) Keying the fresh scan on _adjudicable_id made None match: the parse returns None for exactly the marks no id-keyed call can reach, so an unguarded comparison handed back the first of them. list_unhandled_revisions() reports id=None for such a row, so this was reachable from the library's own output -- reject_revision(None) returned True and destroyed the mark's text, where before it returned False. The index path never matched (no "None" key), so the two lookup paths disagreed as well. get_markup_text renders the adjudicable id too, so the one view that names revision ids in text names the id accept_revision actually takes ("?" when there is none) rather than the raw attribute. Tests: id None resolves nothing through either verb and leaves the mark and its text in place; and author scoping still holds for ids that collide only once parsed (B's w:id="007" against A's w:id="7"), the duplicate-id class that normalizing the key widened. * docs: correct the duplicate-w:id guidance and sync docs/api.md The per-id caveat advised falling back to the row's group_id, which is exactly what a duplicated id does not have: _reconstruct_groups bars every occurrence from grouping, as this PR's own TestGroupResolutionWithDuplicateIds asserts. Two rows sharing an id in list_revisions() is the signal instead, and the author-filtered bulk calls are the way to resolve them. docs/api.md carried neither the author-scoping guarantee nor the per-id caveat; its accept_all/reject_all/accept_revision/reject_revision sections now match SKILL.md, as every prior change to this prose has kept them. ROADMAP #75 no longer describes the fixed behaviour as current, and records why the w:id index was scoped by author instead of the filed element-identity suggestion. #83's fix direction named _reconstruct_groups as the sole writer of an explicit None group; _adopt_split_tail also writes one for an unambiguous split tail, so it points at the duplicate_ids set instead. has_own_revisions says why it keeps the raw w:author rather than _revision_author: an editor named "Unknown" must not claim unattributed marks. * fix: read a revision id the same way on both lookup paths (ROADMAP.md #75) Keying the index on the parsed id left the two paths comparing different things: the fresh scan compared _adjudicable_id's int, the index looked up str(id). Anything int-equal but not an int then resolved through one and not the other -- accept_revision(True) destroyed revision 1 on the scan path (bool subclasses int) while .get("True") found nothing, and None, which list_unhandled_revisions() reports for a mark with no numeric id, matched every unreachable mark through == while missing the index entirely. Both sides now compare str of the adjudicable id. That restores what every id type did before this PR -- including "7", which both paths accepted and which narrowing to int would have broken for a caller round-tripping ids through JSON -- while keeping w:id="007" reachable. The None special case goes away with it, being just one value that now misses on both paths. get_markup_text's id is pinned: w:id="007" renders [ins#7:A] and a mark with no numeric id renders #?, the rows list_unhandled_revisions() reports. * refactor: parse revision ids through _adjudicable_id in the group registry The registry hand-rolled int(elem.getAttribute("w:id")) with a ValueError guard in four places. Identical in behaviour -- int("") raises ValueError too -- but it was the last place that answered "how is a revision id read?" on its own, after the listing, the index and the honesty floor were folded onto one definition. * docs: scope the author-exact claim to targeting, and renumber the filed items docs/api.md promised an author= call "resolves that author's own elements and no one else's", which overstates a data-loss guarantee: the scoping decides which elements are targeted, not what survives, and rejecting an insertion still carries away revisions another author nested inside it. api.md's own rationale-notes section already said so. UnhandledRevision's docstring claimed its rows "carry no adjudicable id, so handing them to accept_revision() could only fail". An unhandled mark often does carry a numeric w:id, and where it collides with a handled revision's the call resolves that other element: verified, reject_revision(row.id) on a w:rPrChange id=7 deleted a w:ins id=7's text and left the rPrChange standing. Filed as #87. main added its own #83 (PR #95) after this branch forked, so the items filed here move to #84/#85/#86 and #75's cross-reference follows them. * docs: say what author= guarantees, and what it does not (ROADMAP.md #75) The accept_all/reject_all docstrings and SKILL.md promised a filtered call "resolves only that author's own elements". Scoping decides which elements are targeted, not what survives: rejecting an insertion still carries away revisions another author nested inside it, reject_all(author=...) included. docs/api.md was corrected already; these are the sources it mirrors. _resolve_all's index bullet claimed the call "resolves exactly the rows it listed". It can resolve fewer -- a listed row nested inside a rejected insertion goes with its host -- so the honest claim is the negative one: it cannot resolve a row it did not list. The test named for the false generalization is renamed and says why equality holds in its own fixture. _revision_element_index's "a duplicated id resolves the same element whichever path finds it" predates the author parameter and is now false by design: the scan hands back the first element carrying the id, a scoped index the first one that author wrote. It is scoped to the unscoped case. accept_revision/reject_revision gain the duplicate-w:id caveat that SKILL.md and docs/api.md already carry. * test: pin group liveness for a non-canonical member id (ROADMAP.md #75) groups_are_dead answers through the same w:id index resolution uses, so a group whose member carries w:id="007" was reported dead while it was live -- and a dead group has nothing left to explain, which is what deletes its note= comment. Nothing covered it; reverting the index key to the raw attribute now fails this test. _adopt_split_tail reads its origin id through _adjudicable_id like the rest of the registry. ROADMAP #75 keeps the one-line record of the fix and leaves the design rationale to the PR. * docs: keep the targeting/survival distinction in _resolve_all (ROADMAP.md #75) "never someone else's" still read as a survival guarantee where its siblings had already been qualified: another author's revision nested inside a rejected insertion does go with it, uncounted. test_accept_all_author_filter_resolves_every_row_it_listed credited itself with covering the author expression, which its fixture cannot -- both rows carry an explicit w:author, so the "Unknown" fallback is pinned by test_unattributed_revision_resolves_under_the_unknown_author instead. It now says which half it covers and points at the other.
pablospe
added a commit
that referenced
this pull request
Aug 31, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Splitting a paragraph on
\nused to drop the surrounding run formatting forevery segment after an empty one: on a bold paragraph,
insert_after(..., "A\nC")kept bold on
C, but"A\n\nC"renderedCat document default. Now theboundary formatting propagates through any run of empty segments.
Why
insert_after/replace/rewrite_paragraphwith a blank line betweensegments is the ordinary way to append a paragraph break, and it silently
de-formatted the inserted text. The loop comment already claimed the
propagation worked — PR #72 merged the comment without the one-line fix.
How
_apply_paragraph_splitsindocx_editor/track_changes/insert.pyrecomputedfallback_rPron every iteration and reset it to""whenever the currentparagraph had no runs. A split at a paragraph end leaves an empty tail, so the
next iteration saw a runless paragraph and cleared the fallback.
The fallback is now initialized once above the loop and updated only when the
paragraph being split actually has runs. The guard keys on presence of runs,
not on a non-empty rPr: a runless paragraph carries no formatting of its own and
must keep the last known boundary, while a paragraph that has runs is a genuine
boundary and replaces it — including when its last run carries no
w:rPr, sounformatted text stays unformatted.
_split_replace,_split_insertandbatch.py's_rewrite_insert_atall funnel through this helper, so one fixcovers all three entry points.
_insert_ins_at_paragraph_start's docstring isupdated to match: the fallback it receives is now loop-carried, so it can come
from several paragraphs back.
ROADMAP.md gains #83, which records a defect this code sits next to but does not
introduce: the boundary run is picked from raw document order, so it can be a
run Word never shows (inside
w:del,w:moveFrom, or a text box), and a copiedrPrcarries a foreign author'sw:rPrChangealong with it. Both reproducewith a plain
"A\nC"split, before this change, and both span three call sites.Testing
TestSplitFormattingAcrossEmptySegmentsintests/test_paragraph_split.py— 8cases: zero/one/two empty segments via
insert_after, thereplaceandrewrite_paragraphentry points, an unformatted-source guard against theopposite over-reach, and reject/accept resolution of a group spanning empty
segments. With the one-line fix reverted, 5 of the 8 fail.
Full suite: 1865 passed, 8 skipped, 2 xfailed in 5:18.
ruff checkandruff format --checkclean;ty checkat baseline (7 pre-existing diagnostics,none in the changed files).