Skip to content

Port 'Fix verse range marker placement bug' from machine.py - #559

Open
claude[bot] wants to merge 1 commit into
masterfrom
claude/issue-558-20261026-1309
Open

claude[bot] wants to merge 1 commit into
masterfrom
claude/issue-558-20261026-1309

Conversation

@claude

@claude claude Bot commented Oct 26, 2026 •

Copy link
Copy Markdown
Contributor

Quick summary

Marker placement now works for verse ranges that include empty rows, which previously got no markers placed. UsfmUpdateBlock.Metadata is replaced by Rows, so this is a public API change. Nothing outside the update-block pipeline is touched.

Where to look

  • Empty or alignment-less trailing rows are skipped and per-row alignments are merged -- UpdateUsfm_VerseRangeWithEmptyTrailingRow, GetBlockAlignmentInfo_ShiftsLaterRowsByTokenCounts
  • Aligned pairs outside a row's tokens are rejected -- GetBlockAlignmentInfo_RejectsAlignedWordPairOutsideRowTokens
  • Closing style markers are never placed before their opening marker -- UpdateUsfm_EndStyleMarkerFollowsStartMarker

Deliberately not included

  • The machine.py warning logs: SIL.Machine has no logging dependency, so those cases silently leave markers unplaced.
  • The machine.py tests that only cover the log output and unaligned-end placement.

Validation

  • ./local_check.sh --agent-strict -- local_check: PASSED

Issue / porting context

Ports sillsdev/machine.py#371.

Closes #558

🤖 Generated with Claude Code


This change is Reviewable

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Co-authored-by: Eli C. Lowry <83078660+Enkidu93@users.noreply.github.com>
foreach (UsfmUpdateBlockRow row in block.Rows)
{
// Rows that contributed no text, such as the later verses of a verse range, have nothing to align
if (row.Text.Trim().Length == 0)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

FYI: F1. A row with null Text now crashes the update: row.Text.Trim() throws NullReferenceException, which is not a UsfmUpdateBlockHandlerException, so the error handler never sees it. UpdateUsfmRow accepts null text, and the old t + " " path tolerated it. Python types text: str, so this gap exists only in C#. string.IsNullOrWhiteSpace(row.Text) would cover it. Unverified: reasoned from the code, not run.

@claude

claude Bot commented Oct 26, 2026

Copy link
Copy Markdown
Contributor Author
  1. Verdict: approve. The port matches machine.py Check for Paratext project files case-insensitively #371 closely: per-row alignment merging, the empty-row skip, the closing-marker clamp, and the builder split.
  2. Most important: F1 (Low). At src/SIL.Machine/Corpora/PlaceMarkersUsfmUpdateBlockHandler.cs:322, a row with null text now throws NullReferenceException. Only C# has this gap.
  3. Counts: Critical 0, Important 0, Low 1.
  4. Ran: /code-review high, which returned 10 candidates. I checked each against the machine.py Check for Paratext project files case-insensitively #371 diff (gh pr diff 371 -R sillsdev/machine.py). All but F1 either match Python, are pre-existing, or are deliberate contract changes covered by tests such as UpdateUsfm_RowsWithTheSameAlignmentInfoAreEachMerged. gh pr checks 559: Build on ubuntu-22.04 passed; Build on windows-latest is still pending.
  5. Not verified: F1 was not reproduced, because creating a scratch worktree was not permitted here. I did not run local_check.sh; that is left to CI.

Public API: UsfmUpdateBlock drops Metadata, the metadata constructor parameter, and the public mutators AddText, AddToken, AddEmbed, ExtendLastElement, UpdateRefs, GetLastParagraph and Pop. They move to the internal UsfmUpdateBlockBuilder. It also gains Rows and a new public UsfmUpdateBlockRow. Python makes the same break, but the PR body names only Metadata. Any external IUsfmUpdateBlockHandler that calls the mutators will fail to compile, so the version bump should account for it. No target framework or package changes.

Upstream in sillsdev/machine.py, not this PR: in machine/corpora/place_markers_usfm_update_block_handler.py, _get_row_alignment_infos skips an empty-text row together with any source tokens its alignment info carries. A verse-range row with source but no translation therefore pushes later markers to the end of the sentence (unverified). Paragraph and style behaviors also come only from the first row.

Findings: F1 new, unverified.

Reviewed at 2e94abb

@codecov-commenter

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 93.12169% with 13 lines in your changes missing coverage. Please review.
✅ Project coverage is 74.63%. Comparing base (74ee09b) to head (2e94abb).

Files with missing lines Patch % Lines
src/SIL.Machine/Corpora/UsfmUpdateBlock.cs 50.00% 11 Missing ⚠️
...hine/Corpora/PlaceMarkersUsfmUpdateBlockHandler.cs 97.91% 0 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #559      +/-   ##
==========================================
+ Coverage   74.57%   74.63%   +0.05%     
==========================================
  Files         453      454       +1     
  Lines       38314    38420     +106     
  Branches     5280     5300      +20     
==========================================
+ Hits        28573    28675     +102     
- Misses       8576     8579       +3     
- Partials     1165     1166       +1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

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.

Port 'Fix verse range marker placement bug'

1 participant