Skip to content

Remove the unreachable reversal-index delete command - #1188

Merged
mark-sil merged 1 commit into
mainfrom
claude/remove-dead-reversal-index-delete-313b52
Oct 15, 2026
Merged

mark-sil merged 1 commit into
mainfrom
claude/remove-dead-reversal-index-delete-313b52

Conversation

@mark-sil

@mark-sil mark-sil commented Oct 15, 2026 •

Copy link
Copy Markdown
Contributor

Start here: Src/LexText/Lexicon/ReversalListener.cs, the deleted block at the end of ReversalClerk.

Deletes ReversalClerk's handlers for the InsertReversalIndex and DeleteReversalIndex xCore messages, along with the two LexEdStrings entries that only the delete confirmation used (ksDeletingThisRevIndex, ksReallyWantToDeleteRevIndex). Pure deletion: 169 lines out, none in.

What breaks? Nothing that can run. LT-16385 (2161fc2, 2015) removed CmdInsertReversalIndex and CmdDeleteReversalIndex from the Lexicon areaConfiguration.xml when reversal indexes became one per analysis writing system, created on demand by IReversalIndexRepository.FindOrCreateIndexForWs. The handlers stayed behind. No configuration XML in DistFiles defines a command with either message, and no code in Src calls the removed members, tests included.

Where to look

  • Removed members: OnDisplayInsertReversalIndex, OnDisplayDeleteReversalIndex, OnDeleteReversalIndex, DeleteReversalIndex, ReallyDeleteReversalIndex, ReversalIndexAfterDeletion, and ReversalClerk's private SetReversalIndexGuid. ReversalListener's own SetReversalIndexGuid stays; Init uses it.
  • Three usings only that code needed: System.Diagnostics, SIL.LCModel.Infrastructure, SIL.FieldWorks.FdoUi.Dialogs.
  • ReallyDeleteReversalIndex held one MasterRefresh publish; it was on the unreachable path.

Not here: no change to ReversalListener.Init or ReversalClerk.ChangeOwningObject, which #1187 edits; the two PRs touch different hunks. No translated .resx copies exist in the tree; Crowdin drops the two keys on its next sync.

Verification: build.ps1 -CommentHygiene -TokenHygiene clean, 0 warnings. LexEdDllTests 19/19 passed; xWorksTests 1789 passed, 2 skipped, 0 failed. No manual testing: nothing reachable changes. Developer-only, no Jira ticket.

Next: approve.

Preflight review details

Code Review Summary

Branch: claude/remove-dead-reversal-index-delete-313b52
Base: main
Date: 2026-10-15
Review model: Claude Opus 5.5
Files changed: 3

Overview

Removes ReversalClerk's handlers for the InsertReversalIndex and
DeleteReversalIndex xCore messages, plus the two LexEdStrings entries only
the delete confirmation used. LT-16385 (2161fc2, 2015) removed the commands
that sent those messages from the Lexicon areaConfiguration.xml; nothing in
Src or DistFiles has sent them since. Developer-only, no ticket needed.

Contract/API Changes

ReversalClerk loses public virtual OnDisplayInsertReversalIndex,
OnDisplayDeleteReversalIndex and OnDeleteReversalIndex, public
DeleteReversalIndex, protected virtual ReallyDeleteReversalIndex and internal
static ReversalIndexAfterDeletion. No overrides or callers exist in the repo.

Findings

Critical - Must address before merge

None.

Important - Should address before merge

None.

Minor - Consider

None.

Required Validation / Evidence

  • build.ps1 -CommentHygiene -TokenHygiene: 0 warnings, 0 errors; both gates clean.
  • test.ps1 -TestProject LexEdDllTests: 19/19 passed.
  • test.ps1 -TestProject xWorksTests: 1789 passed, 2 skipped, 0 failed.
  • Manual: none; no reachable behaviour changes.

Positive Observations

ReversalListener.Init and ReversalClerk.ChangeOwningObject are untouched, so
the branch does not overlap PR #1187.

Interview Notes

Author decided (2026-10-14) to remove the feature rather than revive it.
Author approved the code and commit message before commit.

Suggested Review Focus

  • Confirm no message name in DistFiles configuration reaches the removed handlers.

🤖 Generated with Claude Code


This change is Reviewable

LT-16385 (2161fc2, 2015) removed CmdInsertReversalIndex and
CmdDeleteReversalIndex from the Lexicon areaConfiguration.xml, since
reversal indexes became one per analysis writing system, created on
demand by IReversalIndexRepository.FindOrCreateIndexForWs. Nothing has
sent the InsertReversalIndex or DeleteReversalIndex messages since, but
ReversalClerk kept their handlers.

Nothing reachable changes.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@github-actions

Copy link
Copy Markdown

NUnit Tests

    1 files  ±0      1 suites  ±0   13m 52s ⏱️ + 2m 49s
6 659 tests ±0  6 574 ✅ ±0  85 💤 ±0  0 ❌ ±0 
6 668 runs  ±0  6 583 ✅ ±0  85 💤 ±0  0 ❌ ±0 

Results for commit a7825b6. ± Comparison against base commit e8487e6.

@codecov-commenter

codecov-commenter commented Oct 15, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 39.64%. Comparing base (e8487e6) to head (a7825b6).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1188      +/-   ##
==========================================
+ Coverage   39.63%   39.64%   +0.01%     
==========================================
  Files        1541     1541              
  Lines      354882   354790      -92     
  Branches    41176    41169       -7     
==========================================
+ Hits       140662   140663       +1     
+ Misses     184831   184746      -85     
+ Partials    29389    29381       -8     
Files with missing lines Coverage Δ
Src/LexText/Lexicon/ReversalListener.cs 29.41% <ø> (+6.79%) ⬆️

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@thejambi thejambi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

:lgtm:

@thejambi reviewed 3 files and all commit messages, and made 1 comment.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on mark-sil).

@mark-sil
mark-sil merged commit b1d2482 into main Oct 15, 2026
9 checks passed
@mark-sil
mark-sil deleted the claude/remove-dead-reversal-index-delete-313b52 branch October 15, 2026 18:02
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.

3 participants