Skip to content

Preserve localised _toc.yml part captions on sync - #291

Open
kp992 wants to merge 4 commits into
QuantEcon:mainfrom
kp992:issue-254
Open

Preserve localised _toc.yml part captions on sync#291
kp992 wants to merge 4 commits into
QuantEcon:mainfrom
kp992:issue-254

Conversation

@kp992

@kp992 kp992 commented Aug 25, 2026

Copy link
Copy Markdown

When a sync run regenerated _toc.yml, it mirrored the source directly and silently overwrote any translated part captions in the target.

Two-part fix: fetch the existing target TOC in index.ts so it is available as targetContent, then in processTocFile call mergeTargetCaptions which matches source and target parts by file-set membership and carries the target's caption forward for unchanged parts. New parts with no target equivalent keep the source caption.

Part of #254 — the interim caption-preservation step; the structured TOC merge in #259 and the shared deterministic localisation check supersede it, so this PR does not close #254.

mmcky and others added 2 commits September 1, 2026 17:21
…only target fetch (QuantEcon#254)

Review follow-up on the _toc.yml caption preservation:

- Matching by identical file set lost the caption of exactly the part a
  lecture-adding sync touches (measured on the real python/zh-cn pair: append
  one lecture to the first part and 基础工具 reverted to English). Parts are
  now paired by largest shared membership, each target part used once, ties
  to the nearest position. A part with no shared files keeps the source
  caption and is named in a warning; a matched part whose caption is already
  byte-identical to the source is logged as not localised. No positional
  guessing.
- The merge no longer round-trips through yaml.dump (201 of 204 lines
  changed on lecture-python's TOC, comments dropped, re-dumped even when
  nothing was preserved). Caption values are substituted into the source
  text and the result is verified by parse against the intended document;
  noArrayIndent re-serialisation is the fallback only.
- The rebase replay's `type: 'toc'` site in index.ts now fetches
  targetContent too, so a rebase no longer re-copies English captions.
- Both target-TOC fetches swallow only a 404; any other status fails the
  fetch instead of silently dropping the captions on a transient error.
- Code moved to src/toc-captions.ts (new module, new test file) so this PR
  and QuantEcon#289 — which inserts at the same point in sync-orchestrator.ts and
  adds the same js-yaml import — merge cleanly in either order.
- Tests: byte-identity on a QuantEcon-style fixture with a comment, quoted
  caption and trailing whitespace; gain/lose/reorder a lecture; parts moved;
  new part; no-overlap part not guessed; split part; target-only parts;
  never-localised no-op by reference; no parts; malformed target; quoting;
  commented-out caption line; the block-scalar fallback; matcher unit tests.
  Orchestrator test asserts bytes. CHANGELOG, FAQ and architecture map
  updated. Merged current main.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@mmcky

mmcky commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Thanks @kp992 — this is the right interim step for #254: fetch the target's TOC alongside the source and carry its captions forward, with the source staying the structural authority. The fetch is done the same way as the other target reads and the tests are genuine assertions. As with #289, maintainerCanModify was on, so I pushed the follow-ups straight to this branch — two commits on top of yours: a merge of current main (only the committed bundle conflicted) and bb7cc4b with the changes below. Nothing you wrote was thrown away; your two orchestrator-level tests are still there in tightened form, and the unit tests moved with the code.

What the review found

# Finding Severity Status
1 Exact file-set matching loses the caption of exactly the part a lecture-adding sync touches. A part that gains or loses one file gets a new partKey, misses the map, and reverts to English. Measured on the real python/zh-cn pair: with no structural change 19/19 captions preserved; append one lecture to the first part → 18/19 and 基础工具 reverts to Tools and Techniques. Both field instances in #254 were lecture-adding syncs, so this is the common case, and it fails silently — the #254 failure again, one part per sync. blocker fixed
2 Unconditional yaml.dump reformats the whole file: 201 of 204 lines differ on the real lecture-python TOC even when every caption is preserved, comments are dropped, quoting is normalised, and it re-dumps even when preserved === 0. Every TOC-touching sync PR becomes a full-file diff that hides the actual change. should-fix fixed
3 The rebase replay's second type: 'toc' construction site in index.ts was not updated, so a rebase still re-copied the English TOC verbatim — the #254 thread assumed rebase "inherits the fix for free"; it did not. should-fix fixed
4 The target-TOC fetch swallowed every error as 404: a 403/rate-limit/network failure yields targetContent = undefined → merge skipped → captions overwritten. Fail-open on the exact failure being fixed. should-fix fixed
5 Two target parts with identical membership resolved last-wins; target-only parts ignored (correct); malformed/missing target and flat chapters: layout return source unchanged (correct). nit superseded by the new matcher
6 Tests generated every input with yaml.dump, so the reformat problem was invisible by construction; the only byte-equality test exercised the early-return path; no gain-one-file case. The dist-action bundle was built from the old base (no cache_control from #293). test-coverage fixed; bundle rebuilt on current main

What changed in bb7cc4b

Matching by membership overlap. Each source part is paired with the unclaimed target part sharing the most files (greedy on overlap, ties to the nearest position, each target used once). Gain a lecture, lose one, reorder within a part, move parts around — the caption follows the lectures. A part with no shared files keeps the source caption and is named in a run-log warning (has no counterpart in the target TOC); I deliberately did not add a positional fallback for that case, because same-count/same-index/zero-overlap is just as likely a wholly rewritten part as a renamed single-file one, and attaching a stale translation to the wrong topic is the same silent class we are fixing — English is the honest state, and it is what the shared deterministic check in the #254 design (a caption byte-identical to source) will gate on. A matched part whose caption is already identical to the source is logged as not localised — that is the four never-localised programming editions, surfaced on every sync until someone fixes them once.

Byte-preserving output. The result is the source text with only the caption values substituted, verified by parse against the intended document (parsed source with the captions changed); a noArrayIndent re-serialisation is the fallback only when the substitution cannot express the change (e.g. a block-scalar caption — covered by a test). Same reference returned when nothing changes. Validated on the real lecture-python.mystlecture-python.zh-cn pair (202 lines, 19 source parts, 14 in the target): 14 captions preserved, exactly 14 lines changed, all of them caption lines — and the same numbers after appending a lecture to the first part and after appending a whole new part. The five warnings name the five parts zh-cn has genuinely never received.

Rebase path and fetch semantics. The rebase replay site now passes targetContent too. Both target-TOC fetches swallow only a genuine 404 (first delivery); any other status throws into the existing fetch-error path, so the run fails loudly rather than shipping English captions on a transient error — the same rule the bibliography and config reads already follow.

Layout, to compose with #289. The code lives in a new src/toc-captions.ts with its own src/__tests__/toc-captions.test.ts, and the js-yaml import is gone from sync-orchestrator.ts. #289 inserts its section at the same point in sync-orchestrator.ts and adds the same js-yaml import there, so as originally written the two PRs conflicted (and git's auto-merge produced a duplicate import that tsc rejects). Now they merge cleanly in either order; the only shared touch is processTocFile, which #289 does not modify.

Tests (suite 1,538 → 1,562, all green; lint and format:check clean): byte-identity on a QuantEcon-style fixture carrying a comment, a quoted caption and trailing whitespace; the lecture-adding case; lecture removed; lectures reordered; parts moved; a new part with the warning; no positional guessing on zero overlap; a split part; target-only parts; never-localised no-op by reference; no parts on either side; malformed target; YAML quoting of substituted captions; a commented-out caption line; the block-scalar fallback; the matcher on its own. CHANGELOG entry, a FAQ entry on why captions used to revert and the two cases that still land in English, and the architecture module map.

Scope note

Per the 2026-08-21 design on #254, the end state is W1's structured merge (#259) where captions are never taken from the source at all, plus the shared deterministic localisation check. This PR is the interim preservation step and should not close #254, so I have changed Fixes #254 in the description to a plain reference — shout if you'd rather word it differently. From my side this is ready to merge once you have had a look; if anything in the follow-up reads wrong, say so and I will adjust rather than you needing to rework it.

@kp992 kp992 left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM @mmcky. Thanks for the review and fixing.

mmcky added a commit that referenced this pull request Sep 3, 2026
… v0.27.0 shipped), resume pointer

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
mmcky added a commit that referenced this pull request Sep 3, 2026
… v0.27.0 shipped), resume pointer (#302)

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Sync overwrites localised _toc.yml part captions with English, and nothing detects it

2 participants