Repository navigation
Fix verse range marker placement bug - #371
Conversation
A verse range in the USFM being updated can be matched by more than one row. The updater concatenated all of their texts but kept only the last row's metadata, so the place markers handler received alignment info that did not describe the text it was given. Extracting a verse range puts all of its text on the first verse and leaves the rest empty, so the common shape is one row with text followed by empty rows. The empty row's metadata won, its alignment matrix was empty, and the handler silently placed no markers at all for that range. The block now carries every matched row's metadata alongside the existing single dict, which is left as it was so that existing handlers reading block.metadata are unaffected. The place markers handler ignores rows that have no alignment info or an empty alignment matrix, since they contributed no text to align. If more than one row is left it warns, as only one row's alignment can be used and it describes only part of the block's text. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. |
Enkidu93
left a comment
Enkidu93
left a comment
There was a problem hiding this comment.
@Enkidu93 reviewed 4 files and all commit messages, and made 1 comment.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on ddaspit).
ddaspit
left a comment
ddaspit
left a comment
There was a problem hiding this comment.
@ddaspit reviewed 4 files and all commit messages, and made 2 comments.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on benjaminking).
machine/corpora/place_markers_usfm_update_block_handler.py line 45 at r1 (raw file):
len(infos), ) return infos[-1] if len(infos) > 0 else None
Unless I'm missing something, this seems to contradict the comments above.
machine/corpora/usfm_update_block.py line 32 at r1 (raw file):
@property def metadata(self) -> dict[str, object]:
Nit: maybe we should remove this property altogether and just use row_metadata. It is unclear what the difference is between the two properties.
|
@benjaminking No rush, but if you could wrap this up when you get a chance, that would be great! |
…ces within a single update block.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #371 +/- ##
==========================================
+ Coverage 92.18% 92.27% +0.08%
==========================================
Files 398 398
Lines 25267 25461 +194
==========================================
+ Hits 23293 23494 +201
+ Misses 1974 1967 -7 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
This comment has been minimized.
This comment has been minimized.
Each row's alignment info describes only that row's text, so it is now always merged, even when another row has the same info. Deduplicating by identity broke rows that held equal copies, and deduplicating by equality would drop a repeated verse whose text and alignment are identical. Aligned word pairs outside a row's tokens are now rejected for every row, not only when rows are merged. A single row with such a pair used to place markers silently or fail with an IndexError that bypassed the error handler. Blocks with no markers to place return before their alignment is read, so they no longer log warnings about missing alignment info. The rows argument of UsfmUpdateBlock is keyword-only, so a metadata dict passed where it used to go raises a TypeError instead of being taken as rows. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
This comment has been minimized.
This comment has been minimized.
A block whose alignment info strips every marker it has is left alone, so its alignment is never used. Checking the aligned word pairs while reading the alignment info, before the info's behaviors were known, let such a block fail the whole update. The pairs are now checked only once there are markers to place, and still before rows are merged. Also trim the comments added on this branch to the non-obvious reasons behind the code. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
benjaminking
left a comment
benjaminking
left a comment
There was a problem hiding this comment.
@benjaminking partially reviewed 5 files, made 5 comments, and resolved 4 discussions.
Reviewable status: 0 of 5 files reviewed, 1 unresolved discussion (waiting on ddaspit and Enkidu93).
machine/corpora/place_markers_usfm_update_block_handler.py line 45 at r1 (raw file):
Previously, ddaspit (Damien Daspit) wrote…
Unless I'm missing something, this seems to contradict the comments above.
Yes, it was a confusing way of writing it. Logically that line could only be reached if there was exactly one item in infos, so this was a way of selecting the only item. It should be more clear now.
machine/corpora/usfm_update_block.py line 32 at r1 (raw file):
Previously, ddaspit (Damien Daspit) wrote…
Nit: maybe we should remove this property altogether and just use
row_metadata. It is unclear what the difference is between the two properties.
Done.
This comment has been minimized.
This comment has been minimized.
Enkidu93
left a comment
Enkidu93
left a comment
There was a problem hiding this comment.
@Enkidu93 reviewed 5 files and all commit messages, and dismissed @claude[bot] from 2 discussions.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on ddaspit).
ddaspit
left a comment
ddaspit
left a comment
There was a problem hiding this comment.
@ddaspit reviewed 5 files and all commit messages, made 1 comment, and resolved 1 discussion.
Reviewable status:
Review
Public API and parity: Findings: F1 addressed, F2 accepted, F3 addressed, F4 withdrawn, F5 addressed, F6 addressed, F7 addressed. Reviewed at 3db18e0 |
This PR fixes #370, which is caused by verse ranges containing empty rows with empty alignment matrices, resulting in no markers being placed for the verse range. This changes the marker placement algorithm to ignore any empty rows with an empty alignment matrix.
This change is